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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2024, 05:39 UTC
Message-ID: <xmqqttapvg42.fsf@gitster.g>
In-Reply-To: <9b2f4baa-b602-4cc5-8dfc-dd941b1d7af6@web.de>

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?