[PATCH 0/4] reftable: fix realloc error handling

[PATCH 0/4] reftable: fix realloc error handling

[René Scharfe]

The current handling of reallocation errors leaks the original
allocation in most cases and corrupts the capacity variable.  Fix
that in REFTABLE_ALLOC_GROW and by providing a new macro
REFTABLE_ALLOC_GROW_OR_NULL -- solve this somewhat tricky issue
centrally, with minimal impact to calling code.

And the last two patches add error handling to the remaining
places that still lack it.

  reftable: avoid leaks on realloc error
  reftable: fix allocation count on realloc error
  reftable: handle realloc error in parse_names()
  t-reftable-merged: handle realloc errors

 reftable/basics.c                | 14 ++++-------
 reftable/basics.h                | 41 +++++++++++++++++++++++++-------
 reftable/block.c                 | 10 ++++----
 reftable/pq.c                    |  2 +-
 reftable/record.c                | 12 +++++-----
 reftable/stack.c                 |  8 ++++---
 reftable/writer.c                |  5 ++--
 t/unit-tests/t-reftable-merged.c |  4 ++--
 8 files changed, 60 insertions(+), 36 deletions(-)

--
2.47.1

[PATCH 1/4] reftable: avoid leaks on realloc error

[René Scharfe]

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() and reftable_buf_add() avoid leaking by restoring the
original pointer value on failure, but all other callers seem to be OK
with losing the old allocation.  Add a new variant of the macro,
REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the
allocation counter.  Use it for those callers.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 reftable/basics.h | 10 ++++++++++
 reftable/block.c  | 10 ++++++----
 reftable/pq.c     |  2 +-
 reftable/record.c | 12 ++++++------
 reftable/stack.c  |  8 +++++---
 reftable/writer.c |  5 +++--
 6 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/reftable/basics.h b/reftable/basics.h
index 36beda2c25..259f4c274c 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -129,6 +129,16 @@ char *reftable_strdup(const char *str);
 			REFTABLE_REALLOC_ARRAY(x, alloc); \
 		} \
 	} while (0)
+
+#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); \
+		alloc = 0; \
+	} \
+} while (0)
+
 #define REFTABLE_FREE_AND_NULL(p) do { reftable_free(p); (p) = NULL; } while (0)

 #ifndef REFTABLE_ALLOW_BANNED_ALLOCATORS
diff --git a/reftable/block.c b/reftable/block.c
index 0198078485..9858bbc7c5 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -53,7 +53,8 @@ static int block_writer_register_restart(struct block_writer *w, int n,
 	if (2 + 3 * rlen + n > w->block_size - w->next)
 		return -1;
 	if (is_restart) {
-		REFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);
+		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
+					    w->restart_cap);
 		if (!w->restarts)
 			return REFTABLE_OUT_OF_MEMORY_ERROR;
 		w->restarts[w->restart_len++] = w->next;
@@ -176,7 +177,8 @@ int block_writer_finish(struct block_writer *w)
 		 * is guaranteed to return `Z_STREAM_END`.
 		 */
 		compressed_len = deflateBound(w->zstream, src_len);
-		REFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);
+		REFTABLE_ALLOC_GROW_OR_NULL(w->compressed, compressed_len,
+					    w->compressed_cap);
 		if (!w->compressed) {
 			ret = REFTABLE_OUT_OF_MEMORY_ERROR;
 			return ret;
@@ -235,8 +237,8 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
 		uLong src_len = block->len - block_header_skip;

 		/* Log blocks specify the *uncompressed* size in their header. */
-		REFTABLE_ALLOC_GROW(br->uncompressed_data, sz,
-				    br->uncompressed_cap);
+		REFTABLE_ALLOC_GROW_OR_NULL(br->uncompressed_data, sz,
+					    br->uncompressed_cap);
 		if (!br->uncompressed_data) {
 			err = REFTABLE_OUT_OF_MEMORY_ERROR;
 			goto done;
diff --git a/reftable/pq.c b/reftable/pq.c
index 6ee1164dd3..5591e875e1 100644
--- a/reftable/pq.c
+++ b/reftable/pq.c
@@ -49,7 +49,7 @@ int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry
 {
 	size_t i = 0;

-	REFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(pq->heap, pq->len + 1, pq->cap);
 	if (!pq->heap)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;
 	pq->heap[pq->len++] = *e;
diff --git a/reftable/record.c b/reftable/record.c
index fb5652ed57..04429d23fe 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -246,8 +246,8 @@ static int reftable_ref_record_copy_from(void *rec, const void *src_rec,
 	if (src->refname) {
 		size_t refname_len = strlen(src->refname);

-		REFTABLE_ALLOC_GROW(ref->refname, refname_len + 1,
-				    ref->refname_cap);
+		REFTABLE_ALLOC_GROW_OR_NULL(ref->refname, refname_len + 1,
+					    ref->refname_cap);
 		if (!ref->refname) {
 			err = REFTABLE_OUT_OF_MEMORY_ERROR;
 			goto out;
@@ -385,7 +385,7 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
 	SWAP(r->refname, refname);
 	SWAP(r->refname_cap, refname_cap);

-	REFTABLE_ALLOC_GROW(r->refname, key.len + 1, r->refname_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(r->refname, key.len + 1, r->refname_cap);
 	if (!r->refname) {
 		err = REFTABLE_OUT_OF_MEMORY_ERROR;
 		goto done;
@@ -839,7 +839,7 @@ static int reftable_log_record_decode(void *rec, struct reftable_buf key,
 	if (key.len <= 9 || key.buf[key.len - 9] != 0)
 		return REFTABLE_FORMAT_ERROR;

-	REFTABLE_ALLOC_GROW(r->refname, key.len - 8, r->refname_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(r->refname, key.len - 8, r->refname_cap);
 	if (!r->refname) {
 		err = REFTABLE_OUT_OF_MEMORY_ERROR;
 		goto done;
@@ -947,8 +947,8 @@ static int reftable_log_record_decode(void *rec, struct reftable_buf key,
 	}
 	string_view_consume(&in, n);

-	REFTABLE_ALLOC_GROW(r->value.update.message, scratch->len + 1,
-			    r->value.update.message_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(r->value.update.message, scratch->len + 1,
+				    r->value.update.message_cap);
 	if (!r->value.update.message) {
 		err = REFTABLE_OUT_OF_MEMORY_ERROR;
 		goto done;
diff --git a/reftable/stack.c b/reftable/stack.c
index 634f0c5425..531660a49f 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -317,7 +317,9 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
 				 * thus need to keep them alive here, which we
 				 * do by bumping their refcount.
 				 */
-				REFTABLE_ALLOC_GROW(reused, reused_len + 1, reused_alloc);
+				REFTABLE_ALLOC_GROW_OR_NULL(reused,
+							    reused_len + 1,
+							    reused_alloc);
 				if (!reused) {
 					err = REFTABLE_OUT_OF_MEMORY_ERROR;
 					goto done;
@@ -949,8 +951,8 @@ int reftable_addition_add(struct reftable_addition *add,
 	if (err < 0)
 		goto done;

-	REFTABLE_ALLOC_GROW(add->new_tables, add->new_tables_len + 1,
-			    add->new_tables_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(add->new_tables, add->new_tables_len + 1,
+				    add->new_tables_cap);
 	if (!add->new_tables) {
 		err = REFTABLE_OUT_OF_MEMORY_ERROR;
 		goto done;
diff --git a/reftable/writer.c b/reftable/writer.c
index 624e90fb53..740c98038e 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -254,7 +254,8 @@ static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has
 	if (key->offset_len > 0 && key->offsets[key->offset_len - 1] == off)
 		return 0;

-	REFTABLE_ALLOC_GROW(key->offsets, key->offset_len + 1, key->offset_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(key->offsets, key->offset_len + 1,
+				    key->offset_cap);
 	if (!key->offsets)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;
 	key->offsets[key->offset_len++] = off;
@@ -820,7 +821,7 @@ static int writer_flush_nonempty_block(struct reftable_writer *w)
 	 * Note that this also applies when flushing index blocks, in which
 	 * case we will end up with a multi-level index.
 	 */
-	REFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(w->index, w->index_len + 1, w->index_cap);
 	if (!w->index)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;

--
2.47.1

[PATCH 2/4] reftable: fix allocation count on realloc error

[René Scharfe]

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>
---
 reftable/basics.c | 11 +++--------
 reftable/basics.h | 39 ++++++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/reftable/basics.c b/reftable/basics.c
index 70b1091d14..cd6b39dbe9 100644
--- 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 259f4c274c..fa5d75868b 100644
--- 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(x); \
 		alloc = 0; \
+	} else { \
+		alloc = reftable_alloc_grow_or_null_alloc; \
 	} \
 } while (0)

--
2.47.1

[PATCH 3/4] reftable: handle realloc error in parse_names()

[René Scharfe]

Check the final reallocation for adding the terminating NULL and handle
it just like those in the loop.  Simply use REFTABLE_ALLOC_GROW instead
of keeping the REFTABLE_REALLOC_ARRAY call and adding code to preserve
the original pointer value around it.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 reftable/basics.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/reftable/basics.c b/reftable/basics.c
index cd6b39dbe9..fe2b83ff83 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -241,7 +241,8 @@ char **parse_names(char *buf, int size)
 		p = next + 1;
 	}

-	REFTABLE_REALLOC_ARRAY(names, names_len + 1);
+	if (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap))
+		goto err;
 	names[names_len] = NULL;

 	return names;
--
2.47.1

[PATCH 4/4] t-reftable-merged: check realloc errors

[René Scharfe]

Report reallocation errors in unit tests, like everywhere else.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/unit-tests/t-reftable-merged.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index a12bd0e1a3..60836f80d6 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -178,7 +178,7 @@ static void t_merged_refs(void)
 		if (err > 0)
 			break;

-		REFTABLE_ALLOC_GROW(out, len + 1, cap);
+		check(!REFTABLE_ALLOC_GROW(out, len + 1, cap));
 		out[len++] = ref;
 	}
 	reftable_iterator_destroy(&it);
@@ -459,7 +459,7 @@ static void t_merged_logs(void)
 		if (err > 0)
 			break;

-		REFTABLE_ALLOC_GROW(out, len + 1, cap);
+		check(!REFTABLE_ALLOC_GROW(out, len + 1, cap));
 		out[len++] = log;
 	}
 	reftable_iterator_destroy(&it);
--
2.47.1

Re: [PATCH 1/4] reftable: avoid leaks on realloc error

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> +#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); \
> +		alloc = 0; \
> +	} \
> +} while (0)

It is tricky what (x) means at each reference site ;-) but the above
looks good.

>  #define REFTABLE_FREE_AND_NULL(p) do { reftable_free(p); (p) = NULL; } while (0)
>
>  #ifndef REFTABLE_ALLOW_BANNED_ALLOCATORS
> diff --git a/reftable/block.c b/reftable/block.c
> index 0198078485..9858bbc7c5 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -53,7 +53,8 @@ static int block_writer_register_restart(struct block_writer *w, int n,
>  	if (2 + 3 * rlen + n > w->block_size - w->next)
>  		return -1;
>  	if (is_restart) {
> -		REFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);
> +		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
> +					    w->restart_cap);
>  		if (!w->restarts)
>  			return REFTABLE_OUT_OF_MEMORY_ERROR;
>  		w->restarts[w->restart_len++] = w->next;
> @@ -176,7 +177,8 @@ int block_writer_finish(struct block_writer *w)
>  		 * is guaranteed to return `Z_STREAM_END`.
>  		 */
>  		compressed_len = deflateBound(w->zstream, src_len);
> -		REFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);
> +		REFTABLE_ALLOC_GROW_OR_NULL(w->compressed, compressed_len,
> +					    w->compressed_cap);
>  		if (!w->compressed) {
>  			ret = REFTABLE_OUT_OF_MEMORY_ERROR;
>  			return ret;
> @@ -235,8 +237,8 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
>  		uLong src_len = block->len - block_header_skip;
>
>  		/* Log blocks specify the *uncompressed* size in their header. */
> -		REFTABLE_ALLOC_GROW(br->uncompressed_data, sz,
> -				    br->uncompressed_cap);
> +		REFTABLE_ALLOC_GROW_OR_NULL(br->uncompressed_data, sz,
> +					    br->uncompressed_cap);
>  		if (!br->uncompressed_data) {
>  			err = REFTABLE_OUT_OF_MEMORY_ERROR;
>  			goto done;

These all have "has the preceding realloc() return NULL?" check and
error handling, so they are strict improvement whose only effect is
to plug the leak (and clearing of the allocation).

> diff --git a/reftable/pq.c b/reftable/pq.c
> index 6ee1164dd3..5591e875e1 100644
> --- a/reftable/pq.c
> +++ b/reftable/pq.c
> @@ -49,7 +49,7 @@ int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry
>  {
>  	size_t i = 0;
>
> -	REFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);
> +	REFTABLE_ALLOC_GROW_OR_NULL(pq->heap, pq->len + 1, pq->cap);
>  	if (!pq->heap)
>  		return REFTABLE_OUT_OF_MEMORY_ERROR;
>  	pq->heap[pq->len++] = *e;

Ditto.  And the same can be said to all hunks that follow (omitted).

Makes one wonder what remaining callers of REFTABLE_ALLOC_GROW()
(other than the one in REFTABLE_ALLOC_GROW_OR_NULL()) are getting;
hopefully the next steps will deal with them?

Re: [PATCH 4/4] t-reftable-merged: check realloc errors

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Report reallocation errors in unit tests, like everywhere else.

OK.  That's good for consistency if anything else.

We have a test framework for doing unit test at such low level, yet
we cannot really write tests that validates that the right thing
happens when a particular realloc() call returns NULL, which feels
somewhat disappointing, but that is not a fault of this series.

Thanks.


>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  t/unit-tests/t-reftable-merged.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
> index a12bd0e1a3..60836f80d6 100644
> --- a/t/unit-tests/t-reftable-merged.c
> +++ b/t/unit-tests/t-reftable-merged.c
> @@ -178,7 +178,7 @@ static void t_merged_refs(void)
>  		if (err > 0)
>  			break;
>
> -		REFTABLE_ALLOC_GROW(out, len + 1, cap);
> +		check(!REFTABLE_ALLOC_GROW(out, len + 1, cap));
>  		out[len++] = ref;
>  	}
>  	reftable_iterator_destroy(&it);
> @@ -459,7 +459,7 @@ static void t_merged_logs(void)
>  		if (err > 0)
>  			break;
>
> -		REFTABLE_ALLOC_GROW(out, len + 1, cap);
> +		check(!REFTABLE_ALLOC_GROW(out, len + 1, cap));
>  		out[len++] = log;
>  	}
>  	reftable_iterator_destroy(&it);
> --
> 2.47.1

Re: [PATCH 1/4] reftable: avoid leaks on realloc error

[Patrick Steinhardt]

On Wed, Dec 25, 2024 at 07:38:29PM +0100, René Scharfe wrote:
> 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() and reftable_buf_add() avoid leaking by restoring the
> original pointer value on failure, but all other callers seem to be OK
> with losing the old allocation.  Add a new variant of the macro,
> REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the
> allocation counter.  Use it for those callers.

Hm, okay. I find it a bit curious to discern those two macros from each
other as all callers need to handle OOM errors anyway, so doing the safe
thing should likely be our default here and all callsites that don't
should be adapted, shouldn't they?

In the case of `reftable_buf_add()` I kind of doubt the usefulness of
handling the error just to keep the old pointer intact, as all callsites
will ultimately error out anyway. But in the case of `parse_names()` we
do in fact want to handle the case specially so that we can free any
names we have already parsed, so that case makes sense indeed.

So there is merit in having two separate wrappers, but it would be nice
if `REFTABLE_ALLOC_GROW()` would be doing the "right thing" for most
cases while the above two callsites would be adapted to use a wrapper
that requires a bit more thought to use correctly. For example something
like `REFTABLE_TRY_ALLOC_GROW()` or similar.

Patrick

Re: [PATCH 2/4] reftable: fix allocation count on realloc error

[Patrick Steinhardt]

On Wed, Dec 25, 2024 at 07:38:35PM +0100, René Scharfe wrote:
> 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.

Okay, this change goes into the direction I was wondering about with the
preceding commit, good.

> diff --git a/reftable/basics.h b/reftable/basics.h
> index 259f4c274c..fa5d75868b 100644
> --- 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) \
> +	) \
> +)

Do we even need this macro? I don't really think it serves much of a
purpose anymore.

Patrick

Re: [PATCH 4/4] t-reftable-merged: check realloc errors

[Patrick Steinhardt]

On Thu, Dec 26, 2024 at 09:46:51PM -0800, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
> > Report reallocation errors in unit tests, like everywhere else.
> 
> OK.  That's good for consistency if anything else.
> 
> We have a test framework for doing unit test at such low level, yet
> we cannot really write tests that validates that the right thing
> happens when a particular realloc() call returns NULL, which feels
> somewhat disappointing, but that is not a fault of this series.

In the context of the reftable library we can because we've got
pluggable allocators. We could in theory swap them out against variants
that fail, but it's not easy to make them fail in one specific code
path. For the case at hand though it would work alright.

Patrick

Re: [PATCH 0/4] reftable: fix realloc error handling

[Patrick Steinhardt]

On Wed, Dec 25, 2024 at 07:33:07PM +0100, René Scharfe wrote:
> The current handling of reallocation errors leaks the original
> allocation in most cases and corrupts the capacity variable.  Fix
> that in REFTABLE_ALLOC_GROW and by providing a new macro
> REFTABLE_ALLOC_GROW_OR_NULL -- solve this somewhat tricky issue
> centrally, with minimal impact to calling code.
> 
> And the last two patches add error handling to the remaining
> places that still lack it.

Thanks a lot for working on this!

Patrick

Re: [PATCH 0/4] reftable: fix realloc error handling

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Dec 25, 2024 at 07:33:07PM +0100, René Scharfe wrote:
>> The current handling of reallocation errors leaks the original
>> allocation in most cases and corrupts the capacity variable.  Fix
>> that in REFTABLE_ALLOC_GROW and by providing a new macro
>> REFTABLE_ALLOC_GROW_OR_NULL -- solve this somewhat tricky issue
>> centrally, with minimal impact to calling code.
>> 
>> And the last two patches add error handling to the remaining
>> places that still lack it.
>
> Thanks a lot for working on this!

And thank you for reviewing.

Re: [PATCH 2/4] reftable: fix allocation count on realloc error

[René Scharfe]

Am 25.12.24 um 19:38 schrieb René Scharfe:
> 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>
> ---
>  reftable/basics.c | 11 +++--------
>  reftable/basics.h | 39 ++++++++++++++++++++++++++-------------
>  2 files changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/reftable/basics.c b/reftable/basics.c
> index 70b1091d14..cd6b39dbe9 100644
> --- 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 259f4c274c..fa5d75868b 100644
> --- 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(x); \
>  		alloc = 0; \

This forgets to set x = NULL, which is bad since that's what callers check. :-O
It was not necessary without this patch, because realloc(3) did it.

> +	} else { \
> +		alloc = reftable_alloc_grow_or_null_alloc; \
>  	} \
>  } while (0)
>
> --
> 2.47.1
>

Re: [PATCH 4/4] t-reftable-merged: check realloc errors

[René Scharfe]

Am 27.12.24 um 11:34 schrieb Patrick Steinhardt:
> On Thu, Dec 26, 2024 at 09:46:51PM -0800, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> Report reallocation errors in unit tests, like everywhere else.
>>
>> OK.  That's good for consistency if anything else.
>>
>> We have a test framework for doing unit test at such low level, yet
>> we cannot really write tests that validates that the right thing
>> happens when a particular realloc() call returns NULL, which feels
>> somewhat disappointing, but that is not a fault of this series.
>
> In the context of the reftable library we can because we've got
> pluggable allocators. We could in theory swap them out against variants
> that fail, but it's not easy to make them fail in one specific code
> path. For the case at hand though it would work alright.

It should be easy and safe to provide allocator stubs that just fail
and swap them in and out with reftable_set_alloc() before and after
an operation that performs a single allocation.  Will add basic
tests in the next round.

Injecting an allocation error in the middle of parse_names() would
require a version that starts failing after a certain number of
allocations (or allocated bytes).  Possible, but not in scope for
this series.

René

Re: [PATCH 2/4] reftable: fix allocation count on realloc error

[René Scharfe]

Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:
> On Wed, Dec 25, 2024 at 07:38:35PM +0100, René Scharfe wrote:
>
>> diff --git a/reftable/basics.h b/reftable/basics.h
>> index 259f4c274c..fa5d75868b 100644
>> --- 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) ( \
>> +	(nr) > (alloc) && ( \
>> +		(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \
>> +		(nr) > (alloc) \
>> +	) \
>> +)
>
> Do we even need this macro? I don't really think it serves much of a
> purpose anymore.

It provides the same value as the *ALLOC_ARRAY macros above: automatic
sizeof handling.  Plus it returns whether the allocation succeeded,
avoiding repetition of its arguments in callers.

René

Re: [PATCH 1/4] reftable: avoid leaks on realloc error

[René Scharfe]

Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:
> On Wed, Dec 25, 2024 at 07:38:29PM +0100, René Scharfe wrote:
>> 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() and reftable_buf_add() avoid leaking by restoring the
>> original pointer value on failure, but all other callers seem to be OK
>> with losing the old allocation.  Add a new variant of the macro,
>> REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the
>> allocation counter.  Use it for those callers.
>
> Hm, okay. I find it a bit curious to discern those two macros from each
> other as all callers need to handle OOM errors anyway, so doing the safe
> thing should likely be our default here and all callsites that don't
> should be adapted, shouldn't they?

I agree, and I my first version only had REFTABLE_ALLOC_GROW.  Keeping
stuff unchanged if we cannot grow should be safer, right?  But it would
introduce a leak if the caller exits without cleaning up, so each of
them needs to be audited.  I was too lazy for that.  And it's work that
can be parallelized..

> In the case of `reftable_buf_add()` I kind of doubt the usefulness of
> handling the error just to keep the old pointer intact, as all callsites
> will ultimately error out anyway.

I can imagine use cases where an object is built piece by piece, one
part is too large and then you still want to keep all the rest and just
replace the huge thing with a placeholder or entirely ignore it.  Could
be a case of YAGNI, though.

> But in the case of `parse_names()` we
> do in fact want to handle the case specially so that we can free any
> names we have already parsed, so that case makes sense indeed.

Yes.  But that leads me on a tangent: Is it really a good idea to load
a file into lots of individual string objects instead of loading into
a single big buffer and pointing directly into it?  Do those strings
need to have individual lifetimes?

> So there is merit in having two separate wrappers, but it would be nice
> if `REFTABLE_ALLOC_GROW()` would be doing the "right thing" for most
> cases while the above two callsites would be adapted to use a wrapper
> that requires a bit more thought to use correctly. For example something
> like `REFTABLE_TRY_ALLOC_GROW()` or similar.

So this is about naming?  And with "right thing" you mean failing to
grow should lead to destruction?

René

[PATCH v2 0/4] reftable: fix realloc error handling

[René Scharfe]

Changes since v1:
- added unit tests
- explicitly set pointer to NULL on REFTABLE_ALLOC_GROW_OR_NULL failure
  in patch 2; omission found by unit test

  reftable: avoid leaks on realloc error
  reftable: fix allocation count on realloc error
  reftable: handle realloc error in parse_names()
  t-reftable-merged: handle realloc errors

 reftable/basics.c                | 14 +++-----
 reftable/basics.h                | 41 ++++++++++++++++++-----
 reftable/block.c                 | 10 +++---
 reftable/pq.c                    |  2 +-
 reftable/record.c                | 12 +++----
 reftable/stack.c                 |  8 +++--
 reftable/writer.c                |  5 +--
 t/unit-tests/t-reftable-basics.c | 56 ++++++++++++++++++++++++++++++++
 t/unit-tests/t-reftable-merged.c |  4 +--
 9 files changed, 116 insertions(+), 36 deletions(-)

Range-diff against v1:
1:  b41547720d ! 1:  b3cad92038 reftable: avoid leaks on realloc error
    @@ reftable/writer.c: static int writer_flush_nonempty_block(struct reftable_writer
      	if (!w->index)
      		return REFTABLE_OUT_OF_MEMORY_ERROR;

    +
    + ## t/unit-tests/t-reftable-basics.c ##
    +@@ t/unit-tests/t-reftable-basics.c: static int integer_needle_lesseq(size_t i, void *_args)
    + 	return args->needle <= args->haystack[i];
    + }
    +
    ++static void *realloc_stub(void *p UNUSED, size_t size UNUSED)
    ++{
    ++	return NULL;
    ++}
    ++
    + int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
    + {
    + 	if_test ("binary search with binsearch works") {
    +@@ t/unit-tests/t-reftable-basics.c: int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
    + 		check_int(in, ==, out);
    + 	}
    +
    ++	if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
    ++		int *arr = NULL;
    ++		size_t alloc = 0, old_alloc;
    ++
    ++		REFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);
    ++		check(arr != NULL);
    ++		check_uint(alloc, >=, 1);
    ++		arr[0] = 42;
    ++
    ++		old_alloc = alloc;
    ++		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
    ++		check(arr != NULL);
    ++		check_uint(alloc, >, old_alloc);
    ++		arr[alloc - 1] = 42;
    ++
    ++		old_alloc = alloc;
    ++		reftable_set_alloc(malloc, realloc_stub, free);
    ++		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
    ++		check(arr == NULL);
    ++		check_uint(alloc, ==, 0);
    ++		reftable_set_alloc(malloc, realloc, free);
    ++
    ++		reftable_free(arr);
    ++	}
    ++
    + 	return test_done();
    + }
2:  bde2f0e4a5 ! 2:  62a1042825 reftable: fix allocation count on realloc error
    @@ reftable/basics.h: char *reftable_strdup(const char *str);
     -		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(x); \
    ++		REFTABLE_FREE_AND_NULL(x); \
      		alloc = 0; \
     +	} else { \
     +		alloc = reftable_alloc_grow_or_null_alloc; \
      	} \
      } while (0)

    +
    + ## t/unit-tests/t-reftable-basics.c ##
    +@@ t/unit-tests/t-reftable-basics.c: 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;
3:  7c9f044813 = 3:  ed1a292622 reftable: handle realloc error in parse_names()
4:  3d9493f48a = 4:  916032657e t-reftable-merged: handle realloc errors
--
2.47.1

[PATCH v2 1/4] reftable: avoid leaks on realloc error

[René Scharfe]

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() and reftable_buf_add() avoid leaking by restoring the
original pointer value on failure, but all other callers seem to be OK
with losing the old allocation.  Add a new variant of the macro,
REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the
allocation counter.  Use it for those callers.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 reftable/basics.h                | 10 ++++++++++
 reftable/block.c                 | 10 ++++++----
 reftable/pq.c                    |  2 +-
 reftable/record.c                | 12 ++++++------
 reftable/stack.c                 |  8 +++++---
 reftable/writer.c                |  5 +++--
 t/unit-tests/t-reftable-basics.c | 30 ++++++++++++++++++++++++++++++
 7 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/reftable/basics.h b/reftable/basics.h
index 36beda2c25..259f4c274c 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -129,6 +129,16 @@ char *reftable_strdup(const char *str);
 			REFTABLE_REALLOC_ARRAY(x, alloc); \
 		} \
 	} while (0)
+
+#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); \
+		alloc = 0; \
+	} \
+} while (0)
+
 #define REFTABLE_FREE_AND_NULL(p) do { reftable_free(p); (p) = NULL; } while (0)

 #ifndef REFTABLE_ALLOW_BANNED_ALLOCATORS
diff --git a/reftable/block.c b/reftable/block.c
index 0198078485..9858bbc7c5 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -53,7 +53,8 @@ static int block_writer_register_restart(struct block_writer *w, int n,
 	if (2 + 3 * rlen + n > w->block_size - w->next)
 		return -1;
 	if (is_restart) {
-		REFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);
+		REFTABLE_ALLOC_GROW_OR_NULL(w->restarts, w->restart_len + 1,
+					    w->restart_cap);
 		if (!w->restarts)
 			return REFTABLE_OUT_OF_MEMORY_ERROR;
 		w->restarts[w->restart_len++] = w->next;
@@ -176,7 +177,8 @@ int block_writer_finish(struct block_writer *w)
 		 * is guaranteed to return `Z_STREAM_END`.
 		 */
 		compressed_len = deflateBound(w->zstream, src_len);
-		REFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);
+		REFTABLE_ALLOC_GROW_OR_NULL(w->compressed, compressed_len,
+					    w->compressed_cap);
 		if (!w->compressed) {
 			ret = REFTABLE_OUT_OF_MEMORY_ERROR;
 			return ret;
@@ -235,8 +237,8 @@ int block_reader_init(struct block_reader *br, struct reftable_block *block,
 		uLong src_len = block->len - block_header_skip;

 		/* Log blocks specify the *uncompressed* size in their header. */
-		REFTABLE_ALLOC_GROW(br->uncompressed_data, sz,
-				    br->uncompressed_cap);
+		REFTABLE_ALLOC_GROW_OR_NULL(br->uncompressed_data, sz,
+					    br->uncompressed_cap);
 		if (!br->uncompressed_data) {
 			err = REFTABLE_OUT_OF_MEMORY_ERROR;
 			goto done;
diff --git a/reftable/pq.c b/reftable/pq.c
index 6ee1164dd3..5591e875e1 100644
--- a/reftable/pq.c
+++ b/reftable/pq.c
@@ -49,7 +49,7 @@ int merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry
 {
 	size_t i = 0;

-	REFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(pq->heap, pq->len + 1, pq->cap);
 	if (!pq->heap)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;
 	pq->heap[pq->len++] = *e;
diff --git a/reftable/record.c b/reftable/record.c
index fb5652ed57..04429d23fe 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -246,8 +246,8 @@ static int reftable_ref_record_copy_from(void *rec, const void *src_rec,
 	if (src->refname) {
 		size_t refname_len = strlen(src->refname);

-		REFTABLE_ALLOC_GROW(ref->refname, refname_len + 1,
-				    ref->refname_cap);
+		REFTABLE_ALLOC_GROW_OR_NULL(ref->refname, refname_len + 1,
+					    ref->refname_cap);
 		if (!ref->refname) {
 			err = REFTABLE_OUT_OF_MEMORY_ERROR;
 			goto out;
@@ -385,7 +385,7 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
 	SWAP(r->refname, refname);
 	SWAP(r->refname_cap, refname_cap);

-	REFTABLE_ALLOC_GROW(r->refname, key.len + 1, r->refname_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(r->refname, key.len + 1, r->refname_cap);
 	if (!r->refname) {
 		err = REFTABLE_OUT_OF_MEMORY_ERROR;
 		goto done;
@@ -839,7 +839,7 @@ static int reftable_log_record_decode(void *rec, struct reftable_buf key,
 	if (key.len <= 9 || key.buf[key.len - 9] != 0)
 		return REFTABLE_FORMAT_ERROR;

-	REFTABLE_ALLOC_GROW(r->refname, key.len - 8, r->refname_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(r->refname, key.len - 8, r->refname_cap);
 	if (!r->refname) {
 		err = REFTABLE_OUT_OF_MEMORY_ERROR;
 		goto done;
@@ -947,8 +947,8 @@ static int reftable_log_record_decode(void *rec, struct reftable_buf key,
 	}
 	string_view_consume(&in, n);

-	REFTABLE_ALLOC_GROW(r->value.update.message, scratch->len + 1,
-			    r->value.update.message_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(r->value.update.message, scratch->len + 1,
+				    r->value.update.message_cap);
 	if (!r->value.update.message) {
 		err = REFTABLE_OUT_OF_MEMORY_ERROR;
 		goto done;
diff --git a/reftable/stack.c b/reftable/stack.c
index 634f0c5425..531660a49f 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -317,7 +317,9 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
 				 * thus need to keep them alive here, which we
 				 * do by bumping their refcount.
 				 */
-				REFTABLE_ALLOC_GROW(reused, reused_len + 1, reused_alloc);
+				REFTABLE_ALLOC_GROW_OR_NULL(reused,
+							    reused_len + 1,
+							    reused_alloc);
 				if (!reused) {
 					err = REFTABLE_OUT_OF_MEMORY_ERROR;
 					goto done;
@@ -949,8 +951,8 @@ int reftable_addition_add(struct reftable_addition *add,
 	if (err < 0)
 		goto done;

-	REFTABLE_ALLOC_GROW(add->new_tables, add->new_tables_len + 1,
-			    add->new_tables_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(add->new_tables, add->new_tables_len + 1,
+				    add->new_tables_cap);
 	if (!add->new_tables) {
 		err = REFTABLE_OUT_OF_MEMORY_ERROR;
 		goto done;
diff --git a/reftable/writer.c b/reftable/writer.c
index 624e90fb53..740c98038e 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -254,7 +254,8 @@ static int writer_index_hash(struct reftable_writer *w, struct reftable_buf *has
 	if (key->offset_len > 0 && key->offsets[key->offset_len - 1] == off)
 		return 0;

-	REFTABLE_ALLOC_GROW(key->offsets, key->offset_len + 1, key->offset_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(key->offsets, key->offset_len + 1,
+				    key->offset_cap);
 	if (!key->offsets)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;
 	key->offsets[key->offset_len++] = off;
@@ -820,7 +821,7 @@ static int writer_flush_nonempty_block(struct reftable_writer *w)
 	 * Note that this also applies when flushing index blocks, in which
 	 * case we will end up with a multi-level index.
 	 */
-	REFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);
+	REFTABLE_ALLOC_GROW_OR_NULL(w->index, w->index_len + 1, w->index_cap);
 	if (!w->index)
 		return REFTABLE_OUT_OF_MEMORY_ERROR;

diff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c
index 65d50df091..5bf79c9976 100644
--- a/t/unit-tests/t-reftable-basics.c
+++ b/t/unit-tests/t-reftable-basics.c
@@ -20,6 +20,11 @@ static int integer_needle_lesseq(size_t i, void *_args)
 	return args->needle <= args->haystack[i];
 }

+static void *realloc_stub(void *p UNUSED, size_t size UNUSED)
+{
+	return NULL;
+}
+
 int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
 {
 	if_test ("binary search with binsearch works") {
@@ -141,5 +146,30 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
 		check_int(in, ==, out);
 	}

+	if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
+		int *arr = NULL;
+		size_t alloc = 0, old_alloc;
+
+		REFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);
+		check(arr != NULL);
+		check_uint(alloc, >=, 1);
+		arr[0] = 42;
+
+		old_alloc = alloc;
+		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
+		check(arr != NULL);
+		check_uint(alloc, >, old_alloc);
+		arr[alloc - 1] = 42;
+
+		old_alloc = alloc;
+		reftable_set_alloc(malloc, realloc_stub, free);
+		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
+		check(arr == NULL);
+		check_uint(alloc, ==, 0);
+		reftable_set_alloc(malloc, realloc, free);
+
+		reftable_free(arr);
+	}
+
 	return test_done();
 }
--
2.47.1

[PATCH v2 2/4] reftable: fix allocation count on realloc error

[René Scharfe]

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>
---
 reftable/basics.c                | 11 +++------
 reftable/basics.h                | 39 +++++++++++++++++++++-----------
 t/unit-tests/t-reftable-basics.c | 26 +++++++++++++++++++++
 3 files changed, 55 insertions(+), 21 deletions(-)

diff --git a/reftable/basics.c b/reftable/basics.c
index 70b1091d14..cd6b39dbe9 100644
--- 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 259f4c274c..4bf71b0954 100644
--- 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 5bf79c9976..990dc1a244 100644
--- 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;
--
2.47.1

[PATCH v2 3/4] reftable: handle realloc error in parse_names()

[René Scharfe]

Check the final reallocation for adding the terminating NULL and handle
it just like those in the loop.  Simply use REFTABLE_ALLOC_GROW instead
of keeping the REFTABLE_REALLOC_ARRAY call and adding code to preserve
the original pointer value around it.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 reftable/basics.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/reftable/basics.c b/reftable/basics.c
index cd6b39dbe9..fe2b83ff83 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -241,7 +241,8 @@ char **parse_names(char *buf, int size)
 		p = next + 1;
 	}

-	REFTABLE_REALLOC_ARRAY(names, names_len + 1);
+	if (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap))
+		goto err;
 	names[names_len] = NULL;

 	return names;
--
2.47.1

[PATCH v2 4/4] t-reftable-merged: handle realloc errors

[René Scharfe]

Check reallocation errors in unit tests, like everywhere else.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/unit-tests/t-reftable-merged.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/unit-tests/t-reftable-merged.c b/t/unit-tests/t-reftable-merged.c
index a12bd0e1a3..60836f80d6 100644
--- a/t/unit-tests/t-reftable-merged.c
+++ b/t/unit-tests/t-reftable-merged.c
@@ -178,7 +178,7 @@ static void t_merged_refs(void)
 		if (err > 0)
 			break;

-		REFTABLE_ALLOC_GROW(out, len + 1, cap);
+		check(!REFTABLE_ALLOC_GROW(out, len + 1, cap));
 		out[len++] = ref;
 	}
 	reftable_iterator_destroy(&it);
@@ -459,7 +459,7 @@ static void t_merged_logs(void)
 		if (err > 0)
 			break;

-		REFTABLE_ALLOC_GROW(out, len + 1, cap);
+		check(!REFTABLE_ALLOC_GROW(out, len + 1, cap));
 		out[len++] = log;
 	}
 	reftable_iterator_destroy(&it);
--
2.47.1

Re: [PATCH 1/4] reftable: avoid leaks on realloc error

[Patrick Steinhardt]

On Fri, Dec 27, 2024 at 09:16:27PM +0100, René Scharfe wrote:
> Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:
> > On Wed, Dec 25, 2024 at 07:38:29PM +0100, René Scharfe wrote:
> >> 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() and reftable_buf_add() avoid leaking by restoring the
> >> original pointer value on failure, but all other callers seem to be OK
> >> with losing the old allocation.  Add a new variant of the macro,
> >> REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the
> >> allocation counter.  Use it for those callers.
> >
> > Hm, okay. I find it a bit curious to discern those two macros from each
> > other as all callers need to handle OOM errors anyway, so doing the safe
> > thing should likely be our default here and all callsites that don't
> > should be adapted, shouldn't they?
> 
> I agree, and I my first version only had REFTABLE_ALLOC_GROW.  Keeping
> stuff unchanged if we cannot grow should be safer, right?  But it would
> introduce a leak if the caller exits without cleaning up, so each of
> them needs to be audited.  I was too lazy for that.  And it's work that
> can be parallelized..

Fair enough.

> > In the case of `reftable_buf_add()` I kind of doubt the usefulness of
> > handling the error just to keep the old pointer intact, as all callsites
> > will ultimately error out anyway.
> 
> I can imagine use cases where an object is built piece by piece, one
> part is too large and then you still want to keep all the rest and just
> replace the huge thing with a placeholder or entirely ignore it.  Could
> be a case of YAGNI, though.

Probably.

> > But in the case of `parse_names()` we
> > do in fact want to handle the case specially so that we can free any
> > names we have already parsed, so that case makes sense indeed.
> 
> Yes.  But that leads me on a tangent: Is it really a good idea to load
> a file into lots of individual string objects instead of loading into
> a single big buffer and pointing directly into it?  Do those strings
> need to have individual lifetimes?

Good question indeed. I don't think we ever need individual lifetimes
here. On the other hand it's probably okayish, too, given that the
number of table names should be limited due to automatic compaction.

> > So there is merit in having two separate wrappers, but it would be nice
> > if `REFTABLE_ALLOC_GROW()` would be doing the "right thing" for most
> > cases while the above two callsites would be adapted to use a wrapper
> > that requires a bit more thought to use correctly. For example something
> > like `REFTABLE_TRY_ALLOC_GROW()` or similar.
> 
> So this is about naming?  And with "right thing" you mean failing to
> grow should lead to destruction?

Yup, exactly. I just want to give callers a better indicator which of
these functions does what, and having sane defaults helps in my opinion.

I guess this also depends on whether or not we want to eventually adapt
all callsites to handle allocation errors themselves.

Patrick

Re: [PATCH v2 1/4] reftable: avoid leaks on realloc error

[Patrick Steinhardt]

On Sat, Dec 28, 2024 at 10:47:05AM +0100, René Scharfe wrote:
> @@ -141,5 +146,30 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)
>  		check_int(in, ==, out);
>  	}
> 
> +	if_test ("REFTABLE_ALLOC_GROW_OR_NULL works") {
> +		int *arr = NULL;
> +		size_t alloc = 0, old_alloc;
> +
> +		REFTABLE_ALLOC_GROW_OR_NULL(arr, 1, alloc);
> +		check(arr != NULL);
> +		check_uint(alloc, >=, 1);
> +		arr[0] = 42;
> +
> +		old_alloc = alloc;
> +		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> +		check(arr != NULL);
> +		check_uint(alloc, >, old_alloc);
> +		arr[alloc - 1] = 42;
> +
> +		old_alloc = alloc;
> +		reftable_set_alloc(malloc, realloc_stub, free);
> +		REFTABLE_ALLOC_GROW_OR_NULL(arr, old_alloc + 1, alloc);
> +		check(arr == NULL);
> +		check_uint(alloc, ==, 0);
> +		reftable_set_alloc(malloc, realloc, free);
> +
> +		reftable_free(arr);
> +	}
> +

Thanks for adding this test!

Patrick

Re: [PATCH v2 3/4] reftable: handle realloc error in parse_names()

[Patrick Steinhardt]

On Sat, Dec 28, 2024 at 10:48:50AM +0100, René Scharfe wrote:
> Check the final reallocation for adding the terminating NULL and handle
> it just like those in the loop.  Simply use REFTABLE_ALLOC_GROW instead
> of keeping the REFTABLE_REALLOC_ARRAY call and adding code to preserve
> the original pointer value around it.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  reftable/basics.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/reftable/basics.c b/reftable/basics.c
> index cd6b39dbe9..fe2b83ff83 100644
> --- a/reftable/basics.c
> +++ b/reftable/basics.c
> @@ -241,7 +241,8 @@ char **parse_names(char *buf, int size)
>  		p = next + 1;
>  	}
> 
> -	REFTABLE_REALLOC_ARRAY(names, names_len + 1);
> +	if (REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap))
> +		goto err;
>  	names[names_len] = NULL;

Okay. We may be overallocating in some cases now, but it's most likely
that we can completely skip this allocation here because `names_cap` is
big enough already.

Patrick

Re: [PATCH v2 0/4] reftable: fix realloc error handling

[Patrick Steinhardt]

On Sat, Dec 28, 2024 at 10:43:41AM +0100, René Scharfe wrote:
> Changes since v1:
> - added unit tests
> - explicitly set pointer to NULL on REFTABLE_ALLOC_GROW_OR_NULL failure
>   in patch 2; omission found by unit test
> 
>   reftable: avoid leaks on realloc error
>   reftable: fix allocation count on realloc error
>   reftable: handle realloc error in parse_names()
>   t-reftable-merged: handle realloc errors

I think this version is good enough for now. I'm not particularly happy
about the split we have with the reftable reallocators now, but don't
think that the series really is to blame for that as it simply fixes a
preexisting issue. We can iterate on this in the future.

Thanks!

Patrick