Fix memory leak with duplicated fields and PB_ENABLE_MALLOC.

If a required or optional field appeared twice in the message data,
pb_decode will overwrite the old data with new one. That is fine, but
with submessage fields, it didn't release the allocated subfields before
overwriting.

This bug can manifest if all of the following conditions are true:

1. There is a message with a "optional" or "required" submessage field
   that has type:FT_POINTER.

2. The submessage contains atleast one field with type:FT_POINTER.

3. The message data to be decoded has the submessage field twice in it.

diff --git a/pb_decode.c b/pb_decode.c
index ecd46dc..1fbc65c 100644 (file)
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -44,6 +44,10 @@ static bool checkreturn pb_dec_submessage(pb_istream_t *stream, const pb_field_t
 static bool checkreturn pb_skip_varint(pb_istream_t *stream);
 static bool checkreturn pb_skip_string(pb_istream_t *stream);
 
+#ifdef PB_ENABLE_MALLOC
+static void pb_release_single_field(const pb_field_iter_t *iter);
+#endif
+
 /* --- Function pointers to field decoders ---
  * Order in the array must match pb_action_t LTYPE numbering.
  */
@@ -458,6 +462,13 @@ static bool checkreturn decode_pointer_field(pb_istream_t *stream, pb_wire_type_
     {
         case PB_HTYPE_REQUIRED:
         case PB_HTYPE_OPTIONAL:
+            if (PB_LTYPE(type) == PB_LTYPE_SUBMESSAGE &&
+                *(void**)iter->pData != NULL)
+            {
+                /* Duplicate field, have to release the old allocation first. */
+                pb_release_single_field(iter);
+            }
+        
             if (PB_LTYPE(type) == PB_LTYPE_STRING ||
                 PB_LTYPE(type) == PB_LTYPE_BYTES)
             {
@@ -869,60 +880,65 @@ bool pb_decode_delimited(pb_istream_t *stream, const pb_field_t fields[], void *
 }
 
 #ifdef PB_ENABLE_MALLOC
-void pb_release(const pb_field_t fields[], void *dest_struct)
+static void pb_release_single_field(const pb_field_iter_t *iter)
 {
-    pb_field_iter_t iter;
-    
-    if (!pb_field_iter_begin(&iter, fields, dest_struct))
-        return; /* Empty message type */
-    
-    do
+    pb_type_t type;
+    type = iter->pos->type;
+
+    if (PB_ATYPE(type) == PB_ATYPE_POINTER)
     {
-        pb_type_t type;
-        type = iter.pos->type;
-    
-        if (PB_ATYPE(type) == PB_ATYPE_POINTER)
+        if (PB_HTYPE(type) == PB_HTYPE_REPEATED &&
+            (PB_LTYPE(type) == PB_LTYPE_STRING ||
+             PB_LTYPE(type) == PB_LTYPE_BYTES))
         {
-            if (PB_HTYPE(type) == PB_HTYPE_REPEATED &&
-                (PB_LTYPE(type) == PB_LTYPE_STRING ||
-                 PB_LTYPE(type) == PB_LTYPE_BYTES))
+            /* Release entries in repeated string or bytes array */
+            void **pItem = *(void***)iter->pData;
+            pb_size_t count = *(pb_size_t*)iter->pSize;
+            while (count--)
             {
-                /* Release entries in repeated string or bytes array */
-                void **pItem = *(void***)iter.pData;
-                pb_size_t count = *(pb_size_t*)iter.pSize;
-                while (count--)
-                {
-                    pb_free(*pItem);
-                    *pItem++ = NULL;
-                }
-                *(pb_size_t*)iter.pSize = 0;
+                pb_free(*pItem);
+                *pItem++ = NULL;
             }
-            else if (PB_LTYPE(type) == PB_LTYPE_SUBMESSAGE)
+            *(pb_size_t*)iter->pSize = 0;
+        }
+        else if (PB_LTYPE(type) == PB_LTYPE_SUBMESSAGE)
+        {
+            /* Release fields in submessages */
+            void *pItem = *(void**)iter->pData;
+            if (pItem)
             {
-                /* Release fields in submessages */
-                void *pItem = *(void**)iter.pData;
-                if (pItem)
+                pb_size_t count = 1;
+                
+                if (PB_HTYPE(type) == PB_HTYPE_REPEATED)
                 {
-                    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;
-                    }
+                    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 */
-            pb_free(*(void**)iter.pData);
-            *(void**)iter.pData = NULL;
         }
+        
+        /* Release main item */
+        pb_free(*(void**)iter->pData);
+        *(void**)iter->pData = NULL;
+    }
+}
+
+void pb_release(const pb_field_t fields[], void *dest_struct)
+{
+    pb_field_iter_t iter;
+    
+    if (!pb_field_iter_begin(&iter, fields, dest_struct))
+        return; /* Empty message type */
+    
+    do
+    {
+        pb_release_single_field(&iter);
     } while (pb_field_iter_next(&iter));
 }
 #endif