@matheusbsbs / matheus-bagliano / commits / 2cca185e85

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>

René Scharfe committed Dec 28, 2024 at 10:48 UTC 2cca185e85171c462166839cfd6ee57c09573160
3 files changed +55 -21
reftable/basics.c
+3 -8
@@ -124,11 +124,8 @@ int reftable_buf_add(struct reftable_buf *buf, const void *data, size_t len)
124 size_t newlen = buf->len + len;
125
126 if (newlen + 1 > buf->alloc) {
127 - char *reallocated = buf->buf;
128 - REFTABLE_ALLOC_GROW(reallocated, newlen + 1, buf->alloc);
129 - if (!reallocated)
127 + if (REFTABLE_ALLOC_GROW(buf->buf, newlen + 1, buf->alloc))
128 return REFTABLE_OUT_OF_MEMORY_ERROR;
131 - buf->buf = reallocated;
129 }
130
131 memcpy(buf->buf + buf->len, data, len);
@@ -233,11 +230,9 @@ char **parse_names(char *buf, int size)
230 next = end;
231 }
232 if (p < next) {
236 - char **names_grown = names;
237 - REFTABLE_ALLOC_GROW(names_grown, names_len + 1, names_cap);
238 - if (!names_grown)
233 + if (REFTABLE_ALLOC_GROW(names, names_len + 1,
234 + names_cap))
235 goto err;
240 - names = names_grown;
236
237 names[names_len] = reftable_strdup(p);
238 if (!names[names_len++])
reftable/basics.h
+26 -13
@@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);
120 #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))
121 #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))
122 #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
123 -#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
124 - do { \
125 - if ((nr) > alloc) { \
126 - alloc = 2 * (alloc) + 1; \
127 - if (alloc < (nr)) \
128 - alloc = (nr); \
129 - REFTABLE_REALLOC_ARRAY(x, alloc); \
130 - } \
131 - } while (0)
123 +
124 +static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,
125 + size_t *allocp)
126 +{
127 + void *new_p;
128 + size_t alloc = *allocp * 2 + 1;
129 + if (alloc < nelem)
130 + alloc = nelem;
131 + new_p = reftable_realloc(p, st_mult(elsize, alloc));
132 + if (!new_p)
133 + return p;
134 + *allocp = alloc;
135 + return new_p;
136 +}
137 +
138 +#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \
139 + (nr) > (alloc) && ( \
140 + (x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \
141 + (nr) > (alloc) \
142 + ) \
143 +)
144
145 #define REFTABLE_ALLOC_GROW_OR_NULL(x, nr, alloc) do { \
134 - void *reftable_alloc_grow_or_null_orig_ptr = (x); \
135 - REFTABLE_ALLOC_GROW((x), (nr), (alloc)); \
136 - if (!(x)) { \
137 - reftable_free(reftable_alloc_grow_or_null_orig_ptr); \
146 + size_t reftable_alloc_grow_or_null_alloc = alloc; \
147 + if (REFTABLE_ALLOC_GROW((x), (nr), reftable_alloc_grow_or_null_alloc)) { \
148 + REFTABLE_FREE_AND_NULL(x); \
149 alloc = 0; \
150 + } else { \
151 + alloc = reftable_alloc_grow_or_null_alloc; \
152 } \
153 } while (0)
154
t/unit-tests/t-reftable-basics.c
+26
@@ -146,6 +146,32 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
146 check_int(in, ==, out);
147 }
148
149 + if_test ("REFTABLE_ALLOC_GROW works") {
150 + int *arr = NULL, *old_arr;
151 + size_t alloc = 0, old_alloc;
152 +
153 + check(!REFTABLE_ALLOC_GROW(arr, 1, alloc));
154 + check(arr != NULL);
155 + check_uint(alloc, >=, 1);
156 + arr[0] = 42;
157 +
158 + old_alloc = alloc;
159 + old_arr = arr;
160 + reftable_set_alloc(malloc, realloc_stub, free);
161 + check(REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
162 + check(arr == old_arr);
163 + check_uint(alloc, ==, old_alloc);
164 +
165 + old_alloc = alloc;
166 + reftable_set_alloc(malloc, realloc, free);
167 + check(!REFTABLE_ALLOC_GROW(arr, old_alloc + 1, alloc));
168 + check(arr != NULL);
169 + check_uint(alloc, >, old_alloc);
170 + arr[alloc - 1] = 42;
171 +
172 + reftable_free(arr);
173 + }
174 +
175 if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
176 int *arr = NULL;
177 size_t alloc = 0, old_alloc;