From fb0afab7f539f1e28fc01d98b446e3ce1e9812d0 Mon Sep 17 00:00:00 2001 From: Ole Troan Date: Thu, 11 Feb 2021 11:13:46 +0100 Subject: [PATCH] vppapigen: fix fromjson coverity errors in generation Fix memory leak coverity errors where free was not called on error conditions. Or called twice. Type: fix Signed-off-by: Ole Troan Change-Id: I21cffa8b01e4f72f10501f202f6a762ae300a941 Signed-off-by: Ole Troan --- src/tools/vppapigen/vppapigen_c.py | 57 ++++++++++++++++++++------------------ src/vat2/CMakeLists.txt | 37 +++++++++++++++++++++++++ src/vat2/test/vat2_test.api | 11 ++++++++ src/vat2/test/vat2_test.c | 44 +++++++++++++++++++---------- 4 files changed, 107 insertions(+), 42 deletions(-) diff --git a/src/tools/vppapigen/vppapigen_c.py b/src/tools/vppapigen/vppapigen_c.py index 1886c98d6be..e7045db6797 100644 --- a/src/tools/vppapigen/vppapigen_c.py +++ b/src/tools/vppapigen/vppapigen_c.py @@ -313,6 +313,7 @@ class FromJSON(): write('#define included_{}_api_fromjson_h\n'.format(self.module)) write('#include \n\n') write('#include \n\n') + write('#pragma GCC diagnostic ignored "-Wunused-label"\n') def is_base_type(self, t): '''Check if a type is one of the VPP API base types''' @@ -331,7 +332,7 @@ class FromJSON(): '''Convert JSON string to vl_api_string_t''' write = self.stream.write - msgvar = "a" if toplevel else "mp" + msgvar = "a" if toplevel else "*mp" msgsize = "l" if toplevel else "*len" if o.modern_vla: @@ -339,6 +340,7 @@ class FromJSON(): write(' size_t plen = strlen(p);\n') write(' {msgvar} = realloc({msgvar}, {msgsize} + plen);\n' .format(msgvar=msgvar, msgsize=msgsize)) + write(' if ({msgvar} == 0) goto error;\n'.format(msgvar=msgvar)) write(' vl_api_c_string_to_api_string(p, (void *){msgvar} + ' '{msgsize} - sizeof(vl_api_string_t));\n' .format(msgvar=msgvar, msgsize=msgsize)) @@ -351,25 +353,21 @@ class FromJSON(): def print_field(self, o, toplevel=False): '''Called for every field in a typedef or define.''' write = self.stream.write - write(' // start field {}\n'.format(o.fieldname)) if o.fieldname in self.noprint_fields: return is_bt = self.is_base_type(o.fieldtype) t = 'vl_api_{}'.format(o.fieldtype) if is_bt else o.fieldtype - msgvar = "a" if toplevel else "mp" + msgvar = "(void **)&a" if toplevel else "mp" msgsize = "&l" if toplevel else "len" if is_bt: write(' vl_api_{t}_fromjson(item, &a->{n});\n' .format(t=o.fieldtype, n=o.fieldname)) else: - write(' {msgvar} = {t}_fromjson({msgvar}, ' - '{msgsize}, item, &a->{n});\n' + write(' if ({t}_fromjson({msgvar}, ' + '{msgsize}, item, &a->{n}) < 0) goto error;\n' .format(t=t, n=o.fieldname, msgvar=msgvar, msgsize=msgsize)) - write(' if (!{msgvar}) {{ free(a); return 0;}} \n'.format(msgvar=msgvar)) - - write(' // end field {}\n'.format(o.fieldname)) _dispatch['Field'] = print_field @@ -395,7 +393,7 @@ class FromJSON(): cJSON *array = cJSON_GetObjectItem(o, "{n}"); int size = cJSON_GetArraySize(array); {lfield} = size; - {msgvar} = realloc({msgvar}, {msgsize} + sizeof({t}) * size); + *{msgvar} = realloc({msgvar}, {msgsize} + sizeof({t}) * size); {t} *d = (void *){msgvar} + {msgsize}; {msgsize} += sizeof({t}) * size; for (i = 0; i < size; i++) {{ @@ -410,7 +408,7 @@ class FromJSON(): return lfield = 'a->' + o.lengthfield if o.lengthfield else o.length - msgvar = "a" if toplevel else "mp" + msgvar = "(void **)&a" if toplevel else "mp" msgsize = "l" if toplevel else "*len" if o.fieldtype == 'u8': @@ -420,7 +418,7 @@ class FromJSON(): write(' if (!s) return 0;\n') write(' {} = vec_len(s);\n'.format(lfield)) - write(' {msgvar} = realloc({msgvar}, {msgsize} + ' + write(' *{msgvar} = realloc({msgvar}, {msgsize} + ' 'vec_len(s));\n'.format(msgvar=msgvar, msgsize=msgsize)) write(' memcpy((void *){msgvar} + {msgsize}, s, ' 'vec_len(s));\n'.format(msgvar=msgvar, msgsize=msgsize)) @@ -452,7 +450,7 @@ class FromJSON(): call = ('vl_api_{t}_fromjson(e, &a->{n}[i]);' .format(t=t, n=o.fieldname)) else: - call = ('a = {}_fromjson({}, len, e, &a->{}[i]);' + call = ('if ({}_fromjson({}, len, e, &a->{}[i]) < 0) goto error;' .format(t, msgvar, o.fieldname)) write(forloop.format(lfield=lfield, t=t, @@ -466,14 +464,15 @@ class FromJSON(): def print_enum(self, o): '''Convert to JSON enum(string) to VPP API enum (int)''' write = self.stream.write - write('static inline void *vl_api_{n}_t_fromjson ' - '(void *mp, int *len, cJSON *o, vl_api_{n}_t *a) {{\n' + write('static inline int vl_api_{n}_t_fromjson' + '(void **mp, int *len, cJSON *o, vl_api_{n}_t *a) {{\n' .format(n=o.name)) write(' char *p = cJSON_GetStringValue(o);\n') for b in o.block: - write(' if (strcmp(p, "{}") == 0) {{*a = {}; return mp;}}\n' + write(' if (strcmp(p, "{}") == 0) {{*a = {}; return 0;}}\n' .format(b[0], b[1])) - write(' return 0;\n') + write(' *a = 0;\n') + write(' return -1;\n') write('}\n') _dispatch['Enum'] = print_enum @@ -503,7 +502,7 @@ class FromJSON(): '''Convert from JSON object to VPP API binary representation''' write = self.stream.write - write('static inline void *vl_api_{name}_t_fromjson (void *mp, ' + write('static inline int vl_api_{name}_t_fromjson (void **mp, ' 'int *len, cJSON *o, vl_api_{name}_t *a) {{\n' .format(name=o.name)) write(' cJSON *item __attribute__ ((unused));\n') @@ -511,20 +510,22 @@ class FromJSON(): for t in o.block: if t.type == 'Field' and t.is_lengthfield: continue - write(' item = cJSON_GetObjectItem(o, "{}");\n' + write('\n item = cJSON_GetObjectItem(o, "{}");\n' .format(t.fieldname)) - write(' if (!item) return 0;\n') + write(' if (!item) goto error;\n') self._dispatch[t.type](self, t) - write(' return mp;\n') + write('\n return 0;\n') + write('\n error:\n') + write(' return -1;\n') write('}\n') def print_union(self, o): '''Convert JSON object to VPP API binary union''' write = self.stream.write - write('static inline void *vl_api_{name}_t_fromjson (void *mp, ' + write('static inline int vl_api_{name}_t_fromjson (void **mp, ' 'int *len, cJSON *o, vl_api_{name}_t *a) {{\n' .format(name=o.name)) write(' cJSON *item __attribute__ ((unused));\n') @@ -537,7 +538,9 @@ class FromJSON(): write(' if (item) {\n') self._dispatch[t.type](self, t) write(' };\n') - write(' return mp;\n') + write('\n return 0;\n') + write('\n error:\n') + write(' return -1;\n') write('}\n') def print_define(self, o): @@ -549,24 +552,24 @@ class FromJSON(): write(' u8 *s __attribute__ ((unused));\n') write(' int l = sizeof(vl_api_{}_t);\n'.format(o.name)) write(' vl_api_{}_t *a = malloc(l);\n'.format(o.name)) + write('\n') for t in o.block: if t.fieldname in self.noprint_fields: continue if t.type == 'Field' and t.is_lengthfield: continue - write(' // processing {}: {} {}\n' - .format(o.name, t.fieldtype, t.fieldname)) - write(' item = cJSON_GetObjectItem(o, "{}");\n' .format(t.fieldname)) - write(' if (!item) { free(a); return 0; }\n') + write(' if (!item) goto error;\n') self._dispatch[t.type](self, t, toplevel=True) write('\n') - write('\n') write(' *len = l;\n') write(' return a;\n') + write('\n error:\n') + write(' free(a);\n') + write(' return 0;\n') write('}\n') def print_using(self, o): diff --git a/src/vat2/CMakeLists.txt b/src/vat2/CMakeLists.txt index 73538b4a2dd..9069d8f6b62 100644 --- a/src/vat2/CMakeLists.txt +++ b/src/vat2/CMakeLists.txt @@ -54,6 +54,43 @@ add_vpp_executable(test_vat2 ENABLE_EXPORTS NO_INSTALL rt m dl crypto ) #target_link_options(test_vat2 PUBLIC "LINKER:-fsanitize=address") + +if("${CMAKE_VERSION}" VERSION_GREATER_EQUAL "3.13" AND "${CMAKE_C_COMPILER_ID}" MATCHES "(Apple)?[Cc]lang") + set(TARGET_NAME test_vat2) + set(COV_SOURCES ${CMAKE_SOURCE_DIR}/vat2/jsonconvert.c) + + message("Building with llvm Code Coverage Tools ${TARGET_NAME}") + target_compile_options(${TARGET_NAME} PRIVATE -fprofile-instr-generate -fcoverage-mapping) + target_link_options(${TARGET_NAME} PRIVATE -fprofile-instr-generate -fcoverage-mapping) + target_compile_options(${TARGET_NAME} PRIVATE -fsanitize=address) + target_link_options(${TARGET_NAME} PRIVATE -fsanitize=address) + + # llvm-cov + add_custom_target(${TARGET_NAME}-ccov-preprocessing + COMMAND LLVM_PROFILE_FILE=${TARGET_NAME}.profraw $ + COMMAND llvm-profdata merge -sparse ${TARGET_NAME}.profraw -o ${TARGET_NAME}.profdata + DEPENDS ${TARGET_NAME}) + + add_custom_target(${TARGET_NAME}-ccov-show + COMMAND llvm-cov show $ -instr-profile=${TARGET_NAME}.profdata -show-line-counts-or-regions ${COV_SOURCES} + DEPENDS ${TARGET_NAME}-ccov-preprocessing) + + add_custom_target(${TARGET_NAME}-ccov-report + COMMAND llvm-cov report -show-functions $ -instr-profile=${TARGET_NAME}.profdata ${COV_SOURCES} + DEPENDS ${TARGET_NAME}-ccov-preprocessing) + + add_custom_target(${TARGET_NAME}-ccov + COMMAND llvm-cov show $ -instr-profile=${TARGET_NAME}.profdata -show-line-counts-or-regions -output-dir=${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/${TARGET_NAME}-llvm-cov -format="html" ${COV_SOURCES} + DEPENDS ${TARGET_NAME}-ccov-preprocessing) + + add_custom_command(TARGET ${TARGET_NAME}-ccov POST_BUILD + COMMAND ; + COMMENT "Open ${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/${TARGET_NAME}-llvm-cov/index.html in your browser to view the coverage report." +) +endif() + + + ############################################################################## # vat2 headers ############################################################################## diff --git a/src/vat2/test/vat2_test.api b/src/vat2/test/vat2_test.api index 6a2c94d182e..fe96dd75784 100644 --- a/src/vat2/test/vat2_test.api +++ b/src/vat2/test/vat2_test.api @@ -28,3 +28,14 @@ enumflag test_enumflags { autoreply define test_enum { vl_api_test_enumflags_t flags; }; + +typedef test_stringtype { + string str[]; +}; + +autoreply define test_string { + vl_api_test_stringtype_t str; +}; +autoreply define test_string2 { + string str[]; +}; diff --git a/src/vat2/test/vat2_test.c b/src/vat2/test/vat2_test.c index fe788f127c6..b5346eeea47 100644 --- a/src/vat2/test/vat2_test.c +++ b/src/vat2/test/vat2_test.c @@ -96,24 +96,38 @@ runtest (char *s, bool should_fail) } struct msgs msgs[] = { -{ - .name = "test_prefix", - .tojson = (tojson_fn_t)vl_api_test_prefix_t_tojson, - .fromjson = (fromjson_fn_t)vl_api_test_prefix_t_fromjson, -}, -{ - .name = "test_enum", - .tojson = (tojson_fn_t)vl_api_test_enum_t_tojson, - .fromjson = (fromjson_fn_t)vl_api_test_enum_t_fromjson, -}, + { + .name = "test_prefix", + .tojson = (tojson_fn_t) vl_api_test_prefix_t_tojson, + .fromjson = (fromjson_fn_t) vl_api_test_prefix_t_fromjson, + }, + { + .name = "test_enum", + .tojson = (tojson_fn_t) vl_api_test_enum_t_tojson, + .fromjson = (fromjson_fn_t) vl_api_test_enum_t_fromjson, + }, + { + .name = "test_string", + .tojson = (tojson_fn_t) vl_api_test_string_t_tojson, + .fromjson = (fromjson_fn_t) vl_api_test_string_t_fromjson, + }, + { + .name = "test_string2", + .tojson = (tojson_fn_t) vl_api_test_string2_t_tojson, + .fromjson = (fromjson_fn_t) vl_api_test_string2_t_fromjson, + }, }; struct tests tests[] = { - {.s = "{\"_msgname\": \"test_prefix\", \"pref\": \"2001:db8::/64\"}"}, - {.s = "{\"_msgname\": \"test_prefix\", \"pref\": \"192.168.10.0/24\"}"}, - {.s = "{\"_msgname\": \"test_enum\", \"flags\": [\"RED\", \"BLUE\"]}"}, - {.s = "{\"_msgname\": \"test_enum\", \"flags\": [\"BLACK\", \"BLUE\"]}", - .should_fail = 1}, + { .s = "{\"_msgname\": \"test_prefix\", \"pref\": \"2001:db8::/64\"}" }, + { .s = "{\"_msgname\": \"test_prefix\", \"pref\": \"192.168.10.0/24\"}" }, + { .s = "{\"_msgname\": \"test_enum\", \"flags\": [\"RED\", \"BLUE\"]}" }, + { .s = "{\"_msgname\": \"test_enum\", \"flags\": [\"BLACK\", \"BLUE\"]}", + .should_fail = 1 }, + { .s = "{\"_msgname\": \"test_string\", \"str\": {\"str\": \"Test string " + "type\"}}" }, + { .s = + "{\"_msgname\": \"test_string2\", \"str\": \"Test string toplevel\"}" }, }; int main (int argc, char **argv) -- 2.16.6