reftable: fix allocation count on realloc error

When realloc(3) fails, it returns NULL and keeps the original allocation
intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and
the allocation count variable in that case, simultaneously leaking the
original allocation and misrepresenting the number of storable items.

parse_names() avoids the leak by keeping the original pointer if
reallocation fails, but still increase the allocation count in such a
case as if it succeeded.  That's OK, because the error handling code
just frees everything and doesn't look at names_cap anymore.

reftable_buf_add() does the same, but here it is a problem as it leaves
the reftable_buf in a broken state, with ->alloc being roughly twice as
big as the actually allocated memory, allowing out-of-bounds writes in
subsequent calls.

Reimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts
in sync and still signal failures to callers while avoiding code
duplication in callers.  Make it an expression that evaluates to 0 if no
reallocation is needed or it succeeded and 1 on failure while keeping
the original pointer and allocation counter values.

Adjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for
REFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables
for now.

Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/basics.c b/reftable/basics.c
index 70b1091d1495bb5b4c8aae63bd9213dc704aecde..cd6b39dbe9c8dba619154ff1d11448845399567e 100644 (file)
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -124,11 +124,8 @@ int reftable_buf_add(struct reftable_buf *buf, const void *data, size_t len)
        size_t newlen = buf->len + len;
 
        if (newlen + 1 > buf->alloc) {
-               char *reallocated = buf->buf;
-               REFTABLE_ALLOC_GROW(reallocated, newlen + 1, buf->alloc);
-               if (!reallocated)
+               if (REFTABLE_ALLOC_GROW(buf->buf, newlen + 1, buf->alloc))
                        return REFTABLE_OUT_OF_MEMORY_ERROR;
-               buf->buf = reallocated;
        }
 
        memcpy(buf->buf + buf->len, data, len);
@@ -233,11 +230,9 @@ char **parse_names(char *buf, int size)
                        next = end;
                }
                if (p < next) {
-                       char **names_grown = names;
-                       REFTABLE_ALLOC_GROW(names_grown, names_len + 1, names_cap);
-                       if (!names_grown)
+                       if (REFTABLE_ALLOC_GROW(names, names_len + 1,
+                                               names_cap))
                                goto err;
-                       names = names_grown;
 
                        names[names_len] = reftable_strdup(p);
                        if (!names[names_len++])

diff --git a/reftable/basics.h b/reftable/basics.h
index 259f4c274c2417ce0278edd8e7a3b1392d8b20d9..4bf71b0954920afd4c049ea861abccf6e7c11662 100644 (file)
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);
 #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))
 #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))
 #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
-#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
-       do { \
-               if ((nr) > alloc) { \
-                       alloc = 2 * (alloc) + 1; \
-                       if (alloc < (nr)) \
-                               alloc = (nr); \
-                       REFTABLE_REALLOC_ARRAY(x, alloc); \
-               } \
-       } while (0)
+
+static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,
+                                       size_t *allocp)
+{
+       void *new_p;
+       size_t alloc = *allocp * 2 + 1;
+       if (alloc < nelem)
+               alloc = nelem;
+       new_p = reftable_realloc(p, st_mult(elsize, alloc));
+       if (!new_p)
+               return p;
+       *allocp = alloc;
+       return new_p;
+}
+
+#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \
+       (nr) > (alloc) && ( \
+               (x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \
+               (nr) > (alloc) \
+       ) \
+)
 
 #define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \
-       void *reftable_alloc_grow_or_null_orig_ptr = (x); \
-       REFTABLE_ALLOC_GROW((x), (nr), (alloc)); \
-       if (!(x)) { \
-               reftable_free(reftable_alloc_grow_or_null_orig_ptr); \
+       size_t reftable_alloc_grow_or_null_alloc = alloc; \
+       if (REFTABLE_ALLOC_GROW((x), (nr), reftable_alloc_grow_or_null_alloc)) { \
+               REFTABLE_FREE_AND_NULL(x); \
                alloc = 0; \
+       } else { \
+               alloc = reftable_alloc_grow_or_null_alloc; \
        } \
 } while (0)
 

diff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c
index 5bf79c9976517c4c045a74144b60fa029d165be7..990dc1a2445ddd004a1db634ceb928ce8320e230 100644 (file)
--- a/t/unit-tests/t-reftable-basics.c
+++ b/t/unit-tests/t-reftable-basics.c
@@ -146,6 +146,32 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
                check_int(in, ==, out);
        }
 
+       if_test ("REFTABLE_ALLOC_GROW works") {
+               int *arr = NULL, *old_arr;
+               size_t alloc = 0, old_alloc;
+
+               check(!REFTABLE_ALLOC_GROW(arr, 1, alloc));
+               check(arr != NULL);
+               check_uint(alloc, >=, 1);
+               arr[0] = 42;
+
+               old_alloc = alloc;
+               old_arr = arr;
+               reftable_set_alloc(malloc, realloc_stub, free);
+               check(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
+               check(arr == old_arr);
+               check_uint(alloc, ==, old_alloc);
+
+               old_alloc = alloc;
+               reftable_set_alloc(malloc, realloc, free);
+               check(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
+               check(arr != NULL);
+               check_uint(alloc, >, old_alloc);
+               arr[alloc - 1] = 42;
+
+               reftable_free(arr);
+       }
+
        if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
                int *arr = NULL;
                size_t alloc = 0, old_alloc;