Fix memory leaks (#2422)

* fix memory leak in test only - invalid api call

Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>

* simplify string init

Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>

* refactor part memory handling for parts

failing to add a part was not reverting cleanly. However, when
reverting, the array storing the list of the parts was not being updated
correctly, causing more potential memory leaks with lots of failures.

Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>

---------

Signed-off-by: Kimball Thurston <kdt3rd@gmail.com>

Author: kdt3rd

src/lib/OpenEXRCore/internal_structs.c

@@ -162,26 +162,22 @@ internal_exr_destroy_part (exr_context_t ctxt, exr_priv_part_t cur)
     atomic_store (&(cur->chunk_table), (uintptr_t) (0));
 #endif
     if (ctable && ((uintptr_t) ctable) != UINTPTR_MAX) dofree (ctable);
+
+    /* we avoid malloc on one part files, but need to check */
+    if (cur != &(ctxt->first_part)) { dofree (cur); }
+    else { memset (cur, 0, sizeof (struct _priv_exr_part_t)); }
 }
 
 /**************************************/
 
 static void
 internal_exr_destroy_parts (exr_context_t ctxt)
 {
-    exr_memory_free_func_t dofree = ctxt->free_fn;
     for (int p = 0; p < ctxt->num_parts; ++p)
-    {
-        exr_priv_part_t cur = ctxt->parts[p];
-
-        internal_exr_destroy_part (ctxt, cur);
-
-        /* the first one is always the one that is part of the file */
-        if (cur != &(ctxt->first_part)) { dofree (cur); }
-        else { memset (cur, 0, sizeof (struct _priv_exr_part_t)); }
-    }
+        internal_exr_destroy_part (ctxt, ctxt->parts[p]);
 
-    if (ctxt->num_parts > 1) dofree (ctxt->parts);
+    if (ctxt->parts != &(ctxt->init_part)) ctxt->free_fn (ctxt->parts);
+    ctxt->init_part = NULL;
     ctxt->parts     = NULL;
     ctxt->num_parts = 0;
 }
@@ -200,15 +196,14 @@ internal_exr_add_part (
 
     if (ncount == 1)
     {
-        /* no need to zilch, the parent struct will have already been zero'ed */
+        /* avoid an initial malloc by embedding a part in the struct... */
         part         = &(f->first_part);
+        /* also need a pointer to pointer... */
         f->init_part = part;
         nptrs        = &(f->init_part);
     }
     else
     {
-        struct _priv_exr_part_t nil = {0};
-
         part = f->alloc_fn (sizeof (struct _priv_exr_part_t));
         if (!part) return f->standard_error (f, EXR_ERR_OUT_OF_MEMORY);
 
@@ -218,8 +213,8 @@ internal_exr_add_part (
             f->free_fn (part);
             return f->standard_error (f, EXR_ERR_OUT_OF_MEMORY);
         }
-        *part = nil;
     }
+    memset (part, 0, sizeof (struct _priv_exr_part_t));
 
     /* assign appropriately invalid values */
     part->storage_mode         = EXR_STORAGE_LAST_TYPE;
@@ -234,16 +229,13 @@ internal_exr_add_part (
     part->dwa_compression_level = f->default_dwa_quality;
 
     /* put it into the part table */
-    if (ncount > 1)
+    for (int p = 0; p < f->num_parts; ++p)
     {
-        for (int p = 0; p < f->num_parts; ++p)
-        {
-            nptrs[p] = f->parts[p];
-        }
-        nptrs[ncount - 1] = part;
+        nptrs[p] = f->parts[p];
     }
+    nptrs[f->num_parts] = part;
 
-    if (f->num_parts > 1) { f->free_fn (f->parts); }
+    if (f->parts && f->parts != &(f->init_part)) { f->free_fn (f->parts); }
     f->parts     = nptrs;
     f->num_parts = ncount;
     if (outpart) *outpart = part;
@@ -264,20 +256,8 @@ internal_exr_revert_add_part (
     *new_index = -1;
 
     internal_exr_destroy_part (ctxt, part);
-    if (ncount == 0)
-    {
-        ctxt->num_parts = 0;
-        ctxt->init_part = NULL;
-        ctxt->parts     = NULL;
-    }
-    else if (ncount == 1)
-    {
-        if (part == &(ctxt->first_part)) ctxt->first_part = *(ctxt->parts[1]);
-        ctxt->init_part = &(ctxt->first_part);
-        ctxt->free_fn (ctxt->parts);
-        ctxt->parts = &(ctxt->init_part);
-    }
-    else
+
+    if ( ncount > 0 )
     {
         int np = 0;
         for (int p = 0; p < ctxt->num_parts; ++p)
@@ -287,6 +267,13 @@ internal_exr_revert_add_part (
             ++np;
         }
     }
+    else
+    {
+        if (ctxt->parts && ctxt->parts != &(ctxt->init_part))
+            ctxt->free_fn (ctxt->parts);
+
+        ctxt->parts = NULL;
+    }
     ctxt->num_parts = ncount;
 }
 

src/lib/OpenEXRCore/part.c

@@ -83,20 +83,16 @@ exr_add_part (
             (uint64_t) pnamelen));
     }
 
-    rv = internal_exr_add_part (ctxt, &part, new_index);
-    if (rv != EXR_ERR_SUCCESS) return EXR_UNLOCK_AND_RETURN (rv);
-
     if (ctxt->num_parts > 0)
     {
         // ensure multi part has at least some name?
         if (!partname) partname = "";
 
-        for ( int pidx = 0; pidx < ctxt->num_parts - 1; ++pidx )
+        for ( int pidx = 0; pidx < ctxt->num_parts; ++pidx )
         {
             const exr_attribute_t* pname = ctxt->parts[pidx]->name;
             if (!pname)
             {
-                internal_exr_revert_add_part (ctxt, &part, new_index);
                 return EXR_UNLOCK_AND_RETURN (
                     ctxt->print_error (
                         ctxt,
@@ -106,7 +102,6 @@ exr_add_part (
             }
             if (!strcmp (partname, pname->string->str))
             {
-                internal_exr_revert_add_part (ctxt, &part, new_index);
                 return EXR_UNLOCK_AND_RETURN (
                     ctxt->print_error (
                         ctxt,
@@ -117,6 +112,9 @@ exr_add_part (
         }
     }
 
+    rv = internal_exr_add_part (ctxt, &part, new_index);
+    if (rv != EXR_ERR_SUCCESS) return EXR_UNLOCK_AND_RETURN (rv);
+
     part->storage_mode = type;
     switch (type)
     {

src/lib/OpenEXRCore/string.c

@@ -15,7 +15,6 @@
 exr_result_t
 exr_attr_string_init (exr_context_t ctxt, exr_attr_string_t* s, int32_t len)
 {
-    exr_attr_string_t nil = {0};
     if (!ctxt) return EXR_ERR_MISSING_CONTEXT_ARG;
 
     if (len < 0)
@@ -31,10 +30,10 @@ exr_attr_string_init (exr_context_t ctxt, exr_attr_string_t* s, int32_t len)
             EXR_ERR_INVALID_ARGUMENT,
             "Invalid reference to string object to initialize");
 
-    *s     = nil;
     s->str = (char*) ctxt->alloc_fn ((size_t) (len + 1));
     if (s->str == NULL)
         return ctxt->standard_error (ctxt, EXR_ERR_OUT_OF_MEMORY);
+
     s->length     = len;
     s->alloc_size = len + 1;
     return EXR_ERR_SUCCESS;
@@ -46,7 +45,6 @@ exr_result_t
 exr_attr_string_init_static_with_length (
     exr_context_t ctxt, exr_attr_string_t* s, const char* v, int32_t len)
 {
-    exr_attr_string_t nil = {0};
     if (!ctxt) return EXR_ERR_MISSING_CONTEXT_ARG;
 
     if (len < 0)
@@ -68,9 +66,9 @@ exr_attr_string_init_static_with_length (
             EXR_ERR_INVALID_ARGUMENT,
             "Invalid reference to string object to initialize");
 
-    *s        = nil;
-    s->length = len;
-    s->str    = v;
+    s->length     = len;
+    s->alloc_size = 0;
+    s->str        = v;
     return EXR_ERR_SUCCESS;
 }
 
@@ -132,7 +130,11 @@ exr_attr_string_create_with_length (
 #    pragma GCC diagnostic ignored "-Wstringop-truncation"
 #endif
             if (d)
-                strncpy (outs, d, (size_t) len);
+            {
+                size_t slen = strnlen( d, (size_t) len);
+                memcpy (outs, d, slen);
+                memset (outs + slen, '\0', ((size_t)len - slen) + 1);
+            }
             else
                 memset (outs, 0, (size_t) len);
 #ifdef _MSC_VER
@@ -143,7 +145,7 @@ exr_attr_string_create_with_length (
 #    pragma GCC diagnostic pop
 #endif
         }
-        outs[len] = '\0';
+        else outs[0] = '\0';
     }
     return rv;
 }
@@ -210,9 +212,13 @@ exr_attr_string_set_with_length (
 #    pragma GCC diagnostic ignored "-Wstringop-truncation"
 #endif
             if (d)
-                strncpy (sstr, d, (size_t) len);
+            {
+                size_t slen = strnlen( d, (size_t) len);
+                memcpy (sstr, d, slen);
+                memset (sstr + slen, '\0', ((size_t)len - slen) + 1);
+            }
             else
-                memset (sstr, 0, (size_t) len);
+                memset (sstr, 0, (size_t) (len + 1));
 #ifdef _MSC_VER
 #    pragma warning(pop)
 #elif defined(__clang__)
@@ -221,7 +227,7 @@ exr_attr_string_set_with_length (
 #    pragma GCC diagnostic pop
 #endif
         }
-        sstr[len] = '\0';
+        else sstr[0] = '\0';
         return EXR_ERR_SUCCESS;
     }
     exr_attr_string_destroy (ctxt, s);

src/test/OpenEXRCoreTest/general_attr.cpp

@@ -1022,7 +1022,6 @@ testBytesHelper (exr_context_t f)
 
     EXRCORE_TEST_RVAL (exr_attr_bytes_init (f, &b, 0, 0));
     EXRCORE_TEST_RVAL (exr_attr_bytes_destroy (f, &b));
-    EXRCORE_TEST_RVAL (exr_attr_bytes_init (f, &b, 0, 0));
     EXRCORE_TEST_RVAL (exr_attr_bytes_create (f, &b, 0, 4, NULL, data4));
     EXRCORE_TEST_RVAL (exr_attr_bytes_copy (f, &b2, &b));
     EXRCORE_TEST_RVAL (exr_attr_bytes_destroy (f, &b2));

src/test/OpenEXRCoreTest/write.cpp

@@ -88,8 +88,20 @@ testStartWriteScan (const std::string& tempdir)
     EXRCORE_TEST_RVAL (
         exr_add_part (outf, "beauty", EXR_STORAGE_SCANLINE, &partidx));
     EXRCORE_TEST (partidx == 0);
-    EXRCORE_TEST_RVAL (exr_get_count (outf, &partidx));
+    /* dup name check */
+    EXRCORE_TEST_RVAL_FAIL (
+        EXR_ERR_INVALID_ARGUMENT,
+        exr_add_part (outf, "beauty", EXR_STORAGE_SCANLINE, &partidx));
+    /* this add should work, making 2 parts */
+    EXRCORE_TEST_RVAL (
+        exr_add_part (outf, "other", EXR_STORAGE_SCANLINE, &partidx));
     EXRCORE_TEST (partidx == 1);
+    /* test repeated add failure doesn't leak */
+    EXRCORE_TEST_RVAL_FAIL (
+        EXR_ERR_INVALID_ARGUMENT,
+        exr_add_part (outf, "beauty", EXR_STORAGE_SCANLINE, &partidx));
+    EXRCORE_TEST_RVAL (exr_get_count (outf, &partidx));
+    EXRCORE_TEST (partidx == 2);
     partidx = 0;
 
     exr_chunk_info_t cinfo;
@@ -105,7 +117,7 @@ testStartWriteScan (const std::string& tempdir)
         exr_get_name (outf, partidx - 1, &partname));
     EXRCORE_TEST_RVAL_FAIL (
         EXR_ERR_ARGUMENT_OUT_OF_RANGE,
-        exr_get_name (outf, partidx + 1, &partname));
+        exr_get_name (outf, partidx + 2, &partname));
     EXRCORE_TEST_RVAL_FAIL (
         EXR_ERR_INVALID_ARGUMENT, exr_get_name (outf, partidx, NULL));
     EXRCORE_TEST_RVAL (exr_get_name (outf, partidx, &partname));
@@ -119,7 +131,7 @@ testStartWriteScan (const std::string& tempdir)
         exr_get_storage (outf, partidx - 1, &storage));
     EXRCORE_TEST_RVAL_FAIL (
         EXR_ERR_ARGUMENT_OUT_OF_RANGE,
-        exr_get_storage (outf, partidx + 1, &storage));
+        exr_get_storage (outf, partidx + 2, &storage));
     EXRCORE_TEST_RVAL_FAIL (
         EXR_ERR_INVALID_ARGUMENT, exr_get_storage (outf, partidx, NULL));
     EXRCORE_TEST_RVAL (exr_get_storage (outf, partidx, &storage));