Publishing nanopb-0.2.9.2

Also backports cdbf51 generator bugfix.

CHANGELOG.txt

@@ -1,3 +1,7 @@
+nanopb-0.2.9.2 (2015-01-24)
+ Fix memory leaks with PB_ENABLE_MALLOC with some submessage hierarchies (issue 138)
+ Fix compilation error with generated initializers for repeated pointer fields
+
 nanopb-0.2.9.1 (2014-09-11)
  Fix security issue due to size_t overflows. (issue 132)
  Fix memory leak with duplicated fields and PB_ENABLE_MALLOC

generator/nanopb_generator.py

@@ -1,7 +1,7 @@
 #!/usr/bin/python
 
 '''Generate header file for nanopb from a ProtoBuf FileDescriptorSet.'''
-nanopb_version = "nanopb-0.2.9.1"
+nanopb_version = "nanopb-0.2.9.2"
 
 import sys
 
@@ -599,6 +599,8 @@ class Message:
                 else:
                     parts.append(field.get_initializer(null_init))
             elif field.allocation == 'POINTER':
+                if field.rules == 'REPEATED':
+                    parts.append('0')
                 parts.append('NULL')
             elif field.allocation == 'CALLBACK':
                 if field.pbtype == 'EXTENSION':

pb.h

@@ -46,7 +46,7 @@
 
 /* Version of the nanopb library. Just in case you want to check it in
  * your own program. */
-#define NANOPB_VERSION nanopb-0.2.9.1
+#define NANOPB_VERSION nanopb-0.2.9.2
 
 /* Include all the system headers needed by nanopb. You will need the
  * definitions of the following:

pb_decode.c

@@ -42,6 +42,7 @@ static bool checkreturn pb_field_find(pb_field_iterator_t *iter, uint32_t tag);
 static bool checkreturn decode_static_field(pb_istream_t *stream, pb_wire_type_t wire_type, pb_field_iterator_t *iter);
 static bool checkreturn decode_callback_field(pb_istream_t *stream, pb_wire_type_t wire_type, pb_field_iterator_t *iter);
 static bool checkreturn decode_field(pb_istream_t *stream, pb_wire_type_t wire_type, pb_field_iterator_t *iter);
+static void iter_from_extension(pb_field_iterator_t *iter, pb_extension_t *extension);
 static bool checkreturn default_extension_decoder(pb_istream_t *stream, pb_extension_t *extension, uint32_t tag, pb_wire_type_t wire_type);
 static bool checkreturn decode_extension(pb_istream_t *stream, uint32_t tag, pb_wire_type_t wire_type, pb_field_iterator_t *iter);
 static bool checkreturn find_extension_field(pb_field_iterator_t *iter);
@@ -696,6 +697,19 @@ static bool checkreturn decode_field(pb_istream_t *stream, pb_wire_type_t wire_t
     }
 }
 
+static void iter_from_extension(pb_field_iterator_t *iter, pb_extension_t *extension)
+{
+    const pb_field_t *field = (const pb_field_t*)extension->type->arg;
+    
+    iter->start = field;
+    iter->pos = field;
+    iter->field_index = 0;
+    iter->required_field_index = 0;
+    iter->dest_struct = extension->dest;
+    iter->pData = extension->dest;
+    iter->pSize = &extension->found;
+}
+
 /* Default handler for extension fields. Expects a pb_field_t structure
  * in extension->type->arg. */
 static bool checkreturn default_extension_decoder(pb_istream_t *stream,
@@ -707,14 +721,7 @@ static bool checkreturn default_extension_decoder(pb_istream_t *stream,
     if (field->tag != tag)
         return true;
     
-    iter.start = field;
+    iter_from_extension(&iter, extension);
-    iter.pos = field;
-    iter.field_index = 0;
-    iter.required_field_index = 0;
-    iter.dest_struct = extension->dest;
-    iter.pData = extension->dest;
-    iter.pSize = &extension->found;
-    
     return decode_field(stream, wire_type, &iter);
 }
 
@@ -956,6 +963,47 @@ static void pb_release_single_field(const pb_field_iterator_t *iter)
     pb_type_t type;
     type = iter->pos->type;
 
+    /* Release anything contained inside an extension or submsg.
+     * This has to be done even if the submsg itself is statically
+     * allocated. */
+    if (PB_LTYPE(type) == PB_LTYPE_EXTENSION)
+    {
+        /* Release fields from all extensions in the linked list */
+        pb_extension_t *ext = *(pb_extension_t**)iter->pData;
+        while (ext != NULL)
+        {
+            pb_field_iterator_t ext_iter;
+            iter_from_extension(&ext_iter, ext);
+            pb_release_single_field(&ext_iter);
+            ext = ext->next;
+        }
+    }
+    else if (PB_LTYPE(type) == PB_LTYPE_SUBMESSAGE)
+    {
+        /* Release fields in submessage or submsg array */
+        void *pItem = iter->pData;
+        pb_size_t count = 1;
+        
+        if (PB_ATYPE(type) == PB_ATYPE_POINTER)
+        {
+            pItem = *(void**)iter->pData;
+        }
+        
+        if (PB_HTYPE(type) == PB_HTYPE_REPEATED)
+        {
+            count = *(pb_size_t*)iter->pSize;
+        }
+        
+        if (pItem)
+        {
+            while (count--)
+            {
+                pb_release((const pb_field_t*)iter->pos->ptr, pItem);
+                pItem = (uint8_t*)pItem + iter->pos->data_size;
+            }
+        }
+    }
+    
     if (PB_ATYPE(type) == PB_ATYPE_POINTER)
     {
         if (PB_HTYPE(type) == PB_HTYPE_REPEATED &&
@@ -970,28 +1018,12 @@ static void pb_release_single_field(const pb_field_iterator_t *iter)
                 pb_free(*pItem);
                 *pItem++ = NULL;
             }
-            *(pb_size_t*)iter->pSize = 0;
         }
-        else if (PB_LTYPE(type) == PB_LTYPE_SUBMESSAGE)
+        
+        if (PB_HTYPE(type) == PB_HTYPE_REPEATED)
         {
-            /* Release fields in submessages */
+            /* We are going to release the array, so set the size to 0 */
-            void *pItem = *(void**)iter->pData;
+            *(pb_size_t*)iter->pSize = 0;
-            if (pItem)
-            {
-                pb_size_t count = 1;
-                
-                if (PB_HTYPE(type) == PB_HTYPE_REPEATED)
-                {
-                    count = *(pb_size_t*)iter->pSize;
-                    *(pb_size_t*)iter->pSize = 0;
-                }
-                
-                while (count--)
-                {
-                    pb_release((const pb_field_t*)iter->pos->ptr, pItem);
-                    pItem = (uint8_t*)pItem + iter->pos->data_size;
-                }
-            }
         }
         
         /* Release main item */

tests/SConstruct

@@ -142,6 +142,12 @@ import os.path
 env['VARIANT_DIR'] = 'build'
 env['BUILD'] = '#' + env['VARIANT_DIR']
 env['COMMON'] = '#' + env['VARIANT_DIR'] + '/common'
+
+# Include common/SConscript first to make sure its exports are available
+# to other SConscripts.
+SConscript("common/SConscript", exports = 'env', variant_dir = env['VARIANT_DIR'] + '/common')
+
 for subdir in Glob('*/SConscript') + Glob('regression/*/SConscript'):
+    if str(subdir).startswith("common"): continue
     SConscript(subdir, exports = 'env', variant_dir = env['VARIANT_DIR'] + '/' + os.path.dirname(str(subdir)))
 

tests/alltypes_pointer/SConscript

@@ -1,30 +1,20 @@
 # Encode the AllTypes message using pointers for all fields, and verify the
 # output against the normal AllTypes test case.
 
-Import("env")
+Import("env", "malloc_env")
-
-# We need our own pb_decode.o for the malloc support
-env = env.Clone()
-env.Append(CPPDEFINES = {'PB_ENABLE_MALLOC': 1});
-
-# Disable libmudflap, because it will confuse valgrind
-# and other memory leak detection tools.
-if '-fmudflap' in env["CCFLAGS"]:
-    env["CCFLAGS"].remove("-fmudflap")
-    env["LINKFLAGS"].remove("-fmudflap")
-    env["LIBS"].remove("mudflap")
-
-strict = env.Clone()
-strict.Append(CFLAGS = strict['CORECFLAGS'])
-strict.Object("pb_decode_with_malloc.o", "$NANOPB/pb_decode.c")
-strict.Object("pb_encode_with_malloc.o", "$NANOPB/pb_encode.c")
 
 c = Copy("$TARGET", "$SOURCE")
 env.Command("alltypes.proto", "#alltypes/alltypes.proto", c)
 
 env.NanopbProto(["alltypes", "alltypes.options"])
-enc = env.Program(["encode_alltypes_pointer.c", "alltypes.pb.c", "pb_encode_with_malloc.o"])
+enc = malloc_env.Program(["encode_alltypes_pointer.c",
-dec = env.Program(["decode_alltypes_pointer.c", "alltypes.pb.c", "pb_decode_with_malloc.o"])
+                          "alltypes.pb.c",
+                          "$COMMON/pb_encode_with_malloc.o",
+                          "$COMMON/malloc_wrappers.o"])
+dec = malloc_env.Program(["decode_alltypes_pointer.c",
+                          "alltypes.pb.c",
+                          "$COMMON/pb_decode_with_malloc.o",
+                          "$COMMON/malloc_wrappers.o"])
 
 # Encode and compare results to non-pointer alltypes test case
 env.RunTest(enc)

tests/common/SConscript

@@ -8,6 +8,7 @@ env.NanopbProto("unittestproto")
 # Protocol definitions for basic_buffer/stream tests
 env.NanopbProto("person")
 
+#--------------------------------------------
 # Binaries of the pb_decode.c and pb_encode.c
 # These are built using more strict warning flags.
 strict = env.Clone()
@@ -15,3 +16,31 @@ strict.Append(CFLAGS = strict['CORECFLAGS'])
 strict.Object("pb_decode.o", "$NANOPB/pb_decode.c")
 strict.Object("pb_encode.o", "$NANOPB/pb_encode.c")
 
+#-----------------------------------------------
+# Binaries of pb_decode etc. with malloc support
+# Uses malloc_wrappers.c to count allocations.
+malloc_env = env.Clone()
+malloc_env.Append(CPPDEFINES = {'PB_ENABLE_MALLOC': 1,
+                                'PB_SYSTEM_HEADER': '\\"malloc_wrappers_syshdr.h\\"'})
+malloc_env.Append(CPPPATH = ["$COMMON"])
+
+if 'SYSHDR' in malloc_env:
+    malloc_env.Append(CPPDEFINES = {'PB_OLD_SYSHDR': malloc_env['SYSHDR']})
+
+# Disable libmudflap, because it will confuse valgrind
+# and other memory leak detection tools.
+if '-fmudflap' in env["CCFLAGS"]:
+    malloc_env["CCFLAGS"].remove("-fmudflap")
+    malloc_env["LINKFLAGS"].remove("-fmudflap")
+    malloc_env["LIBS"].remove("mudflap")
+
+malloc_strict = malloc_env.Clone()
+malloc_strict.Append(CFLAGS = malloc_strict['CORECFLAGS'])
+malloc_strict.Object("pb_decode_with_malloc.o", "$NANOPB/pb_decode.c")
+malloc_strict.Object("pb_encode_with_malloc.o", "$NANOPB/pb_encode.c")
+
+malloc_env.Object("malloc_wrappers.o", "malloc_wrappers.c")
+malloc_env.Depends("$NANOPB/pb.h", ["malloc_wrappers_syshdr.h", "malloc_wrappers.h"])
+
+Export("malloc_env")
+

tests/extensions/extensions.proto

@@ -7,7 +7,6 @@ extend AllTypes {
 message ExtensionMessage {
     extend AllTypes {
         optional ExtensionMessage AllTypes_extensionfield2 = 254;
-	required ExtensionMessage AllTypes_extensionfield3 = 253;
 	repeated ExtensionMessage AllTypes_extensionfield4 = 252;
     }
     

tests/fuzztest/SConscript

@@ -1,29 +1,9 @@
 # Run a fuzz test to verify robustness against corrupted/malicious data.
 
-Import("env")
+Import("env", "malloc_env")
-
-# We need our own pb_decode.o for the malloc support
-env = env.Clone()
-env.Append(CPPDEFINES = {'PB_ENABLE_MALLOC': 1,
-                         'PB_SYSTEM_HEADER': '\\"fuzz_syshdr.h\\"'})
-env.Append(CPPPATH = ".")
-
-if 'SYSHDR' in env:
-    env.Append(CPPDEFINES = {'PB_OLD_SYSHDR': env['SYSHDR']})
-
-# Disable libmudflap, because it will confuse valgrind
-# and other memory leak detection tools.
-if '-fmudflap' in env["CCFLAGS"]:
-    env["CCFLAGS"].remove("-fmudflap")
-    env["LINKFLAGS"].remove("-fmudflap")
-    env["LIBS"].remove("mudflap")
-
-strict = env.Clone()
-strict.Append(CFLAGS = strict['CORECFLAGS'])
-strict.Object("pb_decode_with_malloc.o", "$NANOPB/pb_decode.c")
-strict.Object("pb_encode_with_malloc.o", "$NANOPB/pb_encode.c")
 
 # We want both pointer and static versions of the AllTypes message
+# Prefix them with package name.
 env.Command("alltypes_static.proto", "#alltypes/alltypes.proto",
             lambda target, source, env:
                 open(str(target[0]), 'w').write("package alltypes_static;\n"
@@ -35,21 +15,20 @@ env.Command("alltypes_pointer.proto", "#alltypes/alltypes.proto",
 
 p1 = env.NanopbProto(["alltypes_pointer", "alltypes_pointer.options"])
 p2 = env.NanopbProto(["alltypes_static", "alltypes_static.options"])
-fuzz = env.Program(["fuzztest.c",
+fuzz = malloc_env.Program(["fuzztest.c",
                     "alltypes_pointer.pb.c",
                     "alltypes_static.pb.c",
-                    "pb_encode_with_malloc.o",
+                    "$COMMON/pb_encode_with_malloc.o",
-                    "pb_decode_with_malloc.o",
+                    "$COMMON/pb_decode_with_malloc.o",
-                    "malloc_wrappers.c"])
+                    "$COMMON/malloc_wrappers.o"])
-Depends([p1, p2, fuzz], ["fuzz_syshdr.h", "malloc_wrappers.h"])
 
 env.RunTest(fuzz)
 
-fuzzstub = env.Program(["fuzzstub.c",
+fuzzstub = malloc_env.Program(["fuzzstub.c",
                     "alltypes_pointer.pb.c",
                     "alltypes_static.pb.c",
-                    "pb_encode_with_malloc.o",
+                    "$COMMON/pb_encode_with_malloc.o",
-                    "pb_decode_with_malloc.o",
+                    "$COMMON/pb_decode_with_malloc.o",
-                    "malloc_wrappers.c"])
+                    "$COMMON/malloc_wrappers.o"])
 
 

tests/fuzztest/fuzzstub.c

@@ -10,7 +10,7 @@
 #include <string.h>
 #include <assert.h>
 #include <time.h>
-#include "malloc_wrappers.h"
+#include <malloc_wrappers.h>
 #include "alltypes_static.pb.h"
 #include "alltypes_pointer.pb.h"
 

tests/fuzztest/fuzztest.c

@@ -9,7 +9,7 @@
 #include <string.h>
 #include <assert.h>
 #include <time.h>
-#include "malloc_wrappers.h"
+#include <malloc_wrappers.h>
 #include "alltypes_static.pb.h"
 #include "alltypes_pointer.pb.h"
 

tests/mem_release/SConscript

@@ -0,0 +1,12 @@
+Import("env", "malloc_env")
+
+env.NanopbProto("mem_release.proto")
+
+test = malloc_env.Program(["mem_release.c",
+                    "mem_release.pb.c",
+                    "$COMMON/pb_encode_with_malloc.o",
+                    "$COMMON/pb_decode_with_malloc.o",
+                    "$COMMON/malloc_wrappers.o"])
+
+env.RunTest(test)
+

tests/mem_release/mem_release.c

@@ -0,0 +1,106 @@
+/* Make sure that all fields are freed in various scenarios. */
+
+#include <pb_decode.h>
+#include <pb_encode.h>
+#include <malloc_wrappers.h>
+#include <stdio.h>
+#include <test_helpers.h>
+#include "mem_release.pb.h"
+
+#define TEST(x) if (!(x)) { \
+    fprintf(stderr, "Test " #x " on line %d failed.\n", __LINE__); \
+    return false; \
+    }
+
+static char *test_str_arr[] = {"1", "2", ""};
+static SubMessage test_msg_arr[] = {SubMessage_init_zero, SubMessage_init_zero};
+
+static bool do_test()
+{
+    uint8_t buffer[256];
+    size_t msgsize;
+    
+    /* Construct a message with various fields filled in */
+    {
+        TestMessage msg = TestMessage_init_zero;
+        pb_extension_t ext2;
+        pb_ostream_t stream;
+        msg.static_req_submsg.dynamic_str = "12345";
+        msg.static_req_submsg.dynamic_str_arr_count = 3;
+        msg.static_req_submsg.dynamic_str_arr = test_str_arr;
+        msg.static_req_submsg.dynamic_submsg_count = 2;
+        msg.static_req_submsg.dynamic_submsg = test_msg_arr;
+        msg.static_req_submsg.dynamic_submsg[1].dynamic_str = "abc";
+        msg.static_opt_submsg.dynamic_str = "abc";
+        msg.has_static_opt_submsg = true;
+        msg.dynamic_submsg = &msg.static_req_submsg;
+        
+        msg.extensions = &ext2;
+        ext2.type = &static_ext;
+        ext2.dest = &msg.static_req_submsg;
+        ext2.next = NULL;
+        
+        stream = pb_ostream_from_buffer(buffer, sizeof(buffer));
+        if (!pb_encode(&stream, TestMessage_fields, &msg))
+        {
+            fprintf(stderr, "Encode failed: %s\n", PB_GET_ERROR(&stream));
+            return false;
+        }
+        msgsize = stream.bytes_written;
+    }
+    
+    /* Output encoded message for debug */
+    SET_BINARY_MODE(stdout);
+    fwrite(buffer, 1, msgsize, stdout);
+    
+    /* Decode memory using dynamic allocation */
+    {
+        TestMessage msg = TestMessage_init_zero;
+        pb_istream_t stream;
+        SubMessage ext2_dest = SubMessage_init_zero;
+        pb_extension_t ext2;
+        
+        msg.extensions = &ext2;
+        ext2.type = &static_ext;
+        ext2.dest = &ext2_dest;
+        ext2.next = NULL;
+        
+        stream = pb_istream_from_buffer(buffer, msgsize);
+        if (!pb_decode(&stream, TestMessage_fields, &msg))
+        {
+            fprintf(stderr, "Decode failed: %s\n", PB_GET_ERROR(&stream));
+            return false;
+        }
+        
+        /* Make sure it encodes back to same data */
+        {
+            uint8_t buffer2[256];
+            pb_ostream_t ostream = pb_ostream_from_buffer(buffer2, sizeof(buffer2));
+            TEST(pb_encode(&ostream, TestMessage_fields, &msg));
+            TEST(ostream.bytes_written == msgsize);
+            TEST(memcmp(buffer, buffer2, msgsize) == 0);
+        }
+        
+        /* Make sure that malloc counters work */
+        TEST(get_alloc_count() > 0);
+        
+        /* Make sure that pb_release releases everything */
+        pb_release(TestMessage_fields, &msg);
+        TEST(get_alloc_count() == 0);
+        
+        /* Check that double-free is a no-op */
+        pb_release(TestMessage_fields, &msg);
+        TEST(get_alloc_count() == 0);
+    }
+    
+    return true;
+}
+
+int main()
+{
+    if (do_test())
+        return 0;
+    else
+        return 1;
+}
+

tests/mem_release/mem_release.proto

@@ -0,0 +1,23 @@
+syntax = "proto2";
+import "nanopb.proto";
+
+message SubMessage
+{
+    optional string dynamic_str = 1 [(nanopb).type = FT_POINTER];
+    repeated string dynamic_str_arr = 2 [(nanopb).type = FT_POINTER];
+    repeated SubMessage dynamic_submsg = 3 [(nanopb).type = FT_POINTER];
+}
+
+message TestMessage
+{
+    required SubMessage static_req_submsg = 1 [(nanopb).type = FT_STATIC];
+    optional SubMessage dynamic_submsg = 2 [(nanopb).type = FT_POINTER];
+    optional SubMessage static_opt_submsg = 3 [(nanopb).type = FT_STATIC];
+    extensions 100 to 200;
+}
+
+extend TestMessage
+{
+    optional SubMessage static_ext = 101 [(nanopb).type = FT_STATIC];
+}
+