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 <ot@cisco.com>
Change-Id: I21cffa8b01e4f72f10501f202f6a762ae300a941
Signed-off-by: Ole Troan <ot@cisco.com>

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 <vppinfra/cJSON.h>\n\n')
         write('#include <vat2/jsonconvert.h>\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 $<TARGET_FILE:${TARGET_NAME}>
+    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 $<TARGET_FILE:${TARGET_NAME}> -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 $<TARGET_FILE:${TARGET_NAME}> -instr-profile=${TARGET_NAME}.profdata ${COV_SOURCES}
+    DEPENDS ${TARGET_NAME}-ccov-preprocessing)
+
+  add_custom_target(${TARGET_NAME}-ccov
+    COMMAND llvm-cov show $<TARGET_FILE:${TARGET_NAME}> -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)