From 33585924de9bf2e77a9e7b5b9d3b9b241b3c8442 Mon Sep 17 00:00:00 2001
From: Petteri Aimonen <jpa@git.mail.kapsi.fi>
Date: Sat, 6 Sep 2014 18:56:34 +0300
Subject: [PATCH] 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.
---
 pb_decode.c | 114 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 65 insertions(+), 49 deletions(-)

diff --git a/pb_decode.c b/pb_decode.c
index 0e8b597..e90fd1b 100644
--- a/pb_decode.c
+++ b/pb_decode.c
@@ -57,6 +57,11 @@ 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 bool checkreturn allocate_field(pb_istream_t *stream, void *pData, size_t data_size, size_t array_size);
+static void pb_release_single_field(const pb_field_iterator_t *iter);
+#endif
+
 /* --- Function pointers to field decoders ---
  * Order in the array must match pb_action_t LTYPE numbering.
  */
@@ -535,6 +540,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)
             {
@@ -934,62 +946,66 @@ bool pb_decode_delimited(pb_istream_t *stream, const pb_field_t fields[], void *
 }
 
 #ifdef PB_ENABLE_MALLOC
+static void pb_release_single_field(const pb_field_iterator_t *iter)
+{
+    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))
+        {
+            /* 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;
+        }
+        else if (PB_LTYPE(type) == PB_LTYPE_SUBMESSAGE)
+        {
+            /* Release fields in submessages */
+            void *pItem = *(void**)iter->pData;
+            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 */
+        pb_free(*(void**)iter->pData);
+        *(void**)iter->pData = NULL;
+    }
+}
+
 void pb_release(const pb_field_t fields[], void *dest_struct)
 {
     pb_field_iterator_t iter;
     pb_field_init(&iter, fields, dest_struct);
+
+    if (iter.pos->tag == 0)
+        return; /* Empty message type */
     
     do
     {
-        pb_type_t type;
-        type = iter.pos->type;
-    
-        /* Avoid crash on empty message types (zero fields) */
-        if (iter.pos->tag == 0)
-            continue;
-        
-        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))
-            {
-                /* Release entries in repeated string or bytes array */
-                void **pItem = *(void***)iter.pData;
-                size_t count = *(size_t*)iter.pSize;
-                while (count--)
-                {
-                    pb_free(*pItem);
-                    *pItem++ = NULL;
-                }
-                *(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)
-                {
-                    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 */
-            pb_free(*(void**)iter.pData);
-            *(void**)iter.pData = NULL;
-        }
+        pb_release_single_field(&iter);
     } while (pb_field_next(&iter));
 }
 #endif