Re: coverity scan shows 4 problems in bundled gnulib

[Bruno Haible]

Mike FABIAN wrote in
<https://lists.gnu.org/archive/html/bug-libunistring/2021-06/msg00000.html>
> A coverity scan of libunistring showed the following 4 problems in the 
> bundled gnulib:
> 
> 
>     Error: RESOURCE_LEAK (CWE-772):
>     libunistring-0.9.10/lib/vasnprintf.c:2123: alloc_fn: Storage is returned 
> from allocation function "u8_to_u32".
>     libunistring-0.9.10/lib/vasnprintf.c:2123: var_assign: Assigning: 
> "converted" = storage returned from "u8_to_u32(arg, arg_end - arg, converted, 
> &converted_len)".
>     libunistring-0.9.10/lib/vasnprintf.c:2140: leaked_storage: Variable 
> "converted" going out of scope leaks the storage it points to.
>     # 2138|                           if (converted != result + length)
>     # 2139|                             {
>     # 2140|->                             ENSURE_ALLOCATION (xsum (length, 
> converted_len));
>     # 2141|                               DCHAR_CPY (result + length, 
> converted, converted_len);
>     # 2142|                               free (converted);
> 
> Here in the ENSURE_ALLOCATION macro, if malloc or realloc fails, the macro
> does a “goto out_of_memory;” and then “converted” goes out of scope and is 
> not freed anymore.
> 
> The other 3 reported problems are the same:
> 
>     Error: RESOURCE_LEAK (CWE-772):
>     libunistring-0.9.10/lib/vasnprintf.c:2249: alloc_fn: Storage is returned 
> from allocation function "u16_to_u32".
>     libunistring-0.9.10/lib/vasnprintf.c:2249: var_assign: Assigning: 
> "converted" = storage returned from "u16_to_u32(arg, arg_end - arg, 
> converted, &converted_len)".
>     libunistring-0.9.10/lib/vasnprintf.c:2266: leaked_storage: Variable 
> "converted" going out of scope leaks the storage it points to.
>     # 2264|                           if (converted != result + length)
>     # 2265|                             {
>     # 2266|->                             ENSURE_ALLOCATION (xsum (length, 
> converted_len));
>     # 2267|                               DCHAR_CPY (result + length, 
> converted, converted_len);
>     # 2268|                               free (converted);
>     
>     Error: RESOURCE_LEAK (CWE-772):
>     libunistring-0.9.10/lib/vasnprintf.c:2375: alloc_fn: Storage is returned 
> from allocation function "u32_to_u16".
>     libunistring-0.9.10/lib/vasnprintf.c:2375: var_assign: Assigning: 
> "converted" = storage returned from "u32_to_u16(arg, arg_end - arg, 
> converted, &converted_len)".
>     libunistring-0.9.10/lib/vasnprintf.c:2392: leaked_storage: Variable 
> "converted" going out of scope leaks the storage it points to.
>     # 2390|                           if (converted != result + length)
>     # 2391|                             {
>     # 2392|->                             ENSURE_ALLOCATION (xsum (length, 
> converted_len));
>     # 2393|                               DCHAR_CPY (result + length, 
> converted, converted_len);
>     # 2394|                               free (converted);
>     
>     Error: RESOURCE_LEAK (CWE-772):
>     libunistring-0.9.10/lib/vasnprintf.c:5354: alloc_fn: Storage is returned 
> from allocation function "u8_conv_from_encoding".
>     libunistring-0.9.10/lib/vasnprintf.c:5354: var_assign: Assigning: 
> "tmpdst" = storage returned from "u8_conv_from_encoding(locale_charset(), 
> iconveh_question_mark, tmpsrc, count, NULL, NULL, &tmpdst_len)".
>     libunistring-0.9.10/lib/vasnprintf.c:5371: leaked_storage: Variable 
> "tmpdst" going out of scope leaks the storage it points to.
>     # 5369|                               return NULL;
>     # 5370|                             }
>     # 5371|->                         ENSURE_ALLOCATION (xsum (length, 
> tmpdst_len));
>     # 5372|                           DCHAR_CPY (result + length, tmpdst, 
> tmpdst_len);
>     # 5373|                           free (tmpdst);

Thanks for the report. Fixed through this patch (in gnulib).


2021-06-05  Bruno Haible  <bruno@clisp.org>

        vasnprintf: Don't leak memory when memory allocation fails.
        Found by Coverity. Reported by Mike Fabian <mfabian@redhat.com> in
        
<https://lists.gnu.org/archive/html/bug-libunistring/2021-06/msg00000.html>.
        * lib/vasnprintf.c (VASNPRINTF): In places where a local variable points
        to heap-allocated storage, free that storage before doing
        'goto out_of_memory;'.

diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index f859fc4..089c113 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -1924,7 +1924,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
 
     /* Ensures that allocated >= needed.  Aborts through a jump to
        out_of_memory if needed is SIZE_MAX or otherwise too big.  */
-#define ENSURE_ALLOCATION(needed) \
+#define ENSURE_ALLOCATION_ELSE(needed, oom_statement) \
     if ((needed) > allocated)                                                \
       {                                                                      \
         size_t memory_size;                                                  \
@@ -1935,17 +1935,19 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
           allocated = (needed);                                              \
         memory_size = xtimes (allocated, sizeof (DCHAR_T));                  \
         if (size_overflow_p (memory_size))                                   \
-          goto out_of_memory;                                                \
+          oom_statement                                                      \
         if (result == resultbuf || result == NULL)                           \
           memory = (DCHAR_T *) malloc (memory_size);                         \
         else                                                                 \
           memory = (DCHAR_T *) realloc (result, memory_size);                \
         if (memory == NULL)                                                  \
-          goto out_of_memory;                                                \
+          oom_statement                                                      \
         if (result == resultbuf && length > 0)                               \
           DCHAR_CPY (memory, result, length);                                \
         result = memory;                                                     \
       }
+#define ENSURE_ALLOCATION(needed) \
+  ENSURE_ALLOCATION_ELSE((needed), goto out_of_memory; )
 
     for (cp = format, i = 0, dp = &d.dir[0]; ; cp = dp->dir_end, i++, dp++)
       {
@@ -2193,7 +2195,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
                           }
                         if (converted != result + length)
                           {
-                            ENSURE_ALLOCATION (xsum (length, converted_len));
+                            ENSURE_ALLOCATION_ELSE (xsum (length, 
converted_len),
+                                                    { free (converted); goto 
out_of_memory; });
                             DCHAR_CPY (result + length, converted, 
converted_len);
                             free (converted);
                           }
@@ -2317,7 +2320,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
                           }
                         if (converted != result + length)
                           {
-                            ENSURE_ALLOCATION (xsum (length, converted_len));
+                            ENSURE_ALLOCATION_ELSE (xsum (length, 
converted_len),
+                                                    { free (converted); goto 
out_of_memory; });
                             DCHAR_CPY (result + length, converted, 
converted_len);
                             free (converted);
                           }
@@ -2441,7 +2445,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
                           }
                         if (converted != result + length)
                           {
-                            ENSURE_ALLOCATION (xsum (length, converted_len));
+                            ENSURE_ALLOCATION_ELSE (xsum (length, 
converted_len),
+                                                    { free (converted); goto 
out_of_memory; });
                             DCHAR_CPY (result + length, converted, 
converted_len);
                             free (converted);
                           }
@@ -2944,7 +2949,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
                         }
                     }
 #  else
-                  ENSURE_ALLOCATION (xsum (length, tmpdst_len));
+                  ENSURE_ALLOCATION_ELSE (xsum (length, tmpdst_len),
+                                          { free (tmpdst); goto out_of_memory; 
});
                   DCHAR_CPY (result + length, tmpdst, tmpdst_len);
                   free (tmpdst);
                   length += tmpdst_len;
@@ -3147,7 +3153,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
                         }
                     }
 # else
-                  ENSURE_ALLOCATION (xsum (length, tmpdst_len));
+                  ENSURE_ALLOCATION_ELSE (xsum (length, tmpdst_len),
+                                          { free (tmpdst); goto out_of_memory; 
});
                   DCHAR_CPY (result + length, tmpdst, tmpdst_len);
                   free (tmpdst);
                   length += tmpdst_len;
@@ -5598,7 +5605,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
                             CLEANUP ();
                             return NULL;
                           }
-                        ENSURE_ALLOCATION (xsum (length, tmpdst_len));
+                        ENSURE_ALLOCATION_ELSE (xsum (length, tmpdst_len),
+                                                { free (tmpdst); goto 
out_of_memory; });
                         DCHAR_CPY (result + length, tmpdst, tmpdst_len);
                         free (tmpdst);
                         count = tmpdst_len;