threads / patch / 62694

patch, 4 partsreftable: fix realloc error handling

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

## tl;dr

25 messages between Dec 25, 2024 and Dec 30, 2024. Diffs are folded; open one to read it.

replies: 24people: 3as markdown or json

René Scharfe· Dec 25, 2024, 18:33 UTC · lore

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

René Scharfe· Dec 25, 2024, 18:38 UTC · re: René Scharfe · lore

[PATCH 1/4] reftable: avoid leaks 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() 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(-)
Show changes to 6 files +31 −17

reftable/basics.h, reftable/block.c, reftable/pq.c, reftable/record.c, reftable/stack.c, reftable/writer.c

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
Junio C Hamano· Dec 27, 2024, 05:39 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 8 quoted lines
> +#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.

Show 38 quoted lines
>  #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).

Show 13 quoted lines
> 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?

Patrick Steinhardt· Dec 27, 2024, 10:33 UTC · re: René Scharfe · lore

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

On Wed, Dec 25, 2024 at 07:38:29PM +0100, René Scharfe wrote:
Show 10 quoted lines
> 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
René Scharfe· Dec 27, 2024, 20:16 UTC · re: Patrick Steinhardt · lore

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

Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:
Show 16 quoted lines
> 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?

Show 5 quoted lines
> 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é
Patrick Steinhardt· Dec 30, 2024, 06:29 UTC · re: René Scharfe · lore

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

On Fri, Dec 27, 2024 at 09:16:27PM +0100, René Scharfe wrote:
Show 23 quoted lines
> 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.
Show 8 quoted lines
> > 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.
Show 8 quoted lines
> > 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.

Show 8 quoted lines
> > 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
René Scharfe· Dec 25, 2024, 18:38 UTC · re: René Scharfe · lore

[PATCH 2/4] 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>
---
 reftable/basics.c | 11 +++--------
 reftable/basics.h | 39 ++++++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 21 deletions(-)
Show changes to 2 files +29 −22

reftable/basics.c, reftable/basics.h

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
Patrick Steinhardt· Dec 27, 2024, 10:33 UTC · re: René Scharfe · lore

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

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

Show 38 quoted lines
> 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
René Scharfe· Dec 27, 2024, 20:16 UTC · re: Patrick Steinhardt · lore

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

Am 27.12.24 um 11:33 schrieb Patrick Steinhardt:
Show 10 quoted lines
> 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)))
Show 9 quoted lines
>> +#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é
René Scharfe· Dec 27, 2024, 20:16 UTC · re: René Scharfe · lore

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

Am 25.12.24 um 19:38 schrieb René Scharfe:
Show 110 quoted lines
> 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.

Show 8 quoted lines
> +	} else { \
> +		alloc = reftable_alloc_grow_or_null_alloc; \
>  	} \
>  } while (0)
>
> --
> 2.47.1
>
René Scharfe· Dec 25, 2024, 18:38 UTC · re: René Scharfe · lore

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

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(-)
Show changes to reftable/basics.c +2 −2
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
René Scharfe· Dec 25, 2024, 18:38 UTC · re: René Scharfe · lore

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

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(-)
Show changes to t/unit-tests/t-reftable-merged.c +2 −3
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
Junio C Hamano· Dec 27, 2024, 05:46 UTC · re: René Scharfe · lore

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

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.
Show 30 quoted lines
>
> 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
Patrick Steinhardt· Dec 27, 2024, 10:34 UTC · re: Junio C Hamano · lore

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

On Thu, Dec 26, 2024 at 09:46:51PM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> 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
René Scharfe· Dec 27, 2024, 20:16 UTC · re: Patrick Steinhardt · lore

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

Am 27.12.24 um 11:34 schrieb Patrick Steinhardt:
Show 16 quoted lines
> 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é
Patrick Steinhardt· Dec 27, 2024, 10:34 UTC · re: René Scharfe · lore

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

On Wed, Dec 25, 2024 at 07:33:07PM +0100, René Scharfe wrote:
Show 8 quoted lines
> 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
Junio C Hamano· Dec 27, 2024, 16:02 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 11 quoted lines
> 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.
René Scharfe· Dec 28, 2024, 09:43 UTC · re: René Scharfe · lore

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

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
René Scharfe· Dec 28, 2024, 09:47 UTC · re: René Scharfe · lore

[PATCH v2 1/4] reftable: avoid leaks 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() 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(-)
Show changes to 7 files +61 −17

reftable/basics.h, reftable/block.c, reftable/pq.c, reftable/record.c, reftable/stack.c, reftable/writer.c, t/unit-tests/t-reftable-basics.c

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
Patrick Steinhardt· Dec 30, 2024, 07:25 UTC · re: René Scharfe · lore

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

On Sat, Dec 28, 2024 at 10:47:05AM +0100, René Scharfe wrote:
Show 29 quoted lines
> @@ -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
René Scharfe· Dec 28, 2024, 09:48 UTC · re: René Scharfe · lore

[PATCH v2 2/4] 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>
---
 reftable/basics.c                | 11 +++------
 reftable/basics.h                | 39 +++++++++++++++++++++-----------
 t/unit-tests/t-reftable-basics.c | 26 +++++++++++++++++++++
 3 files changed, 55 insertions(+), 21 deletions(-)
Show changes to 3 files +55 −22

reftable/basics.c, reftable/basics.h, t/unit-tests/t-reftable-basics.c

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
René Scharfe· Dec 28, 2024, 09:48 UTC · re: René Scharfe · lore

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

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(-)
Show changes to reftable/basics.c +2 −2
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
Patrick Steinhardt· Dec 30, 2024, 07:25 UTC · re: René Scharfe · lore

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

On Sat, Dec 28, 2024 at 10:48:50AM +0100, René Scharfe wrote:
Show 22 quoted lines
> 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
René Scharfe· Dec 28, 2024, 09:49 UTC · re: René Scharfe · lore

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

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(-)
Show changes to t/unit-tests/t-reftable-merged.c +2 −3
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
Patrick Steinhardt· Dec 30, 2024, 07:25 UTC · re: René Scharfe · lore

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

On Sat, Dec 28, 2024 at 10:43:41AM +0100, René Scharfe wrote:
Show 9 quoted lines
> 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

← back to recent threads