| Message ID | 20210916023706.55760-2-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t0000: truly leak free | expand |
On Wed, Sep 15, 2021 at 07:37:05PM -0700, Carlo Marcelo Arenas Belón wrote: > 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. > 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
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 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). Nicely spotted and analyzed. Thanks.
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)); \ + else \ free((x)); \ } while(0)
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. This avoids a leak reported by LSAN as while running t0000 but that wouldn't fail the test (which will be fixed next) : SUMMARY: LeakSanitizer: 1034 byte(s) leaked in 15 allocation(s). Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- tree-diff.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)