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

[Junio C Hamano <gitster@pobox.com>]

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?