Re: [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 16, 2021, 05:17 UTC
Message-ID: <YULTcXX6i2pXW0BE@nand.local>
In-Reply-To: <20210916023706.55760-2-carenas@gmail.com>

On Wed, Sep 15, 2021 at 07:37:05PM -0700, Carlo Marcelo Arenas Belón wrote:

Show 8 quoted lines

> b8ba412bf7 (tree-diff: avoid alloca for large allocations, 2016-06-07)
> adds a way to route some bigger allocations out of the stack and free
> them through the addition of two conveniently named macros, but leaves
> the calls to free the xalloca part, which could be also in the heap,
> if the system doesn't HAVE_ALLOCA (ex: macOS).
>
> Add the missing free call, and while at it, change the expression to
> match in both macros for easy of readability.

s/easy/ease/ or s/easy of/easier/.

> This avoids a leak reported by LSAN as while running t0000 but that
> wouldn't fail the test (which will be fixed next) :

Nit; extra space between the closing parenthesis and colon.

Show 11 quoted lines

> diff --git a/tree-diff.c b/tree-diff.c
> index 1572615bd9..437c98a70e 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -21,7 +21,9 @@
>  		ALLOC_ARRAY((x), nr); \
>  } while(0)
>  #define FAST_ARRAY_FREE(x, nr) do { \
> -	if ((nr) > 2) \
> +	if ((nr) <= 2) \
> +		xalloca_free((x)); \

OK. So the point is that FAST_ARRAY_ALLOC uses xalloca() for small arrays. But that might turn into a full-blown malloc() if we don't have alloca.h. So we need to call xalloca_free() which is a noop if we used alloca(), but calls free() if we actually used malloc() instead.

Now that I wrote it out myself, I think you basically said as much in the patch message. But it may have been clearer to say:

    Add the missing free call, [xmalloca_free(), which is a noop if we
    allocated memory in the stack frame, but a real free() if we
    allocaegd in the heap instead].

Thanks, Taylor