public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Jeff King <peff@peff•net>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail•com>
Cc: git@vger•kernel.org, me@ttaylorr•com, gitster@pobox•com
Subject: Re: [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H
Date: Thu, 16 Sep 2021 11:00:30 -0400	[thread overview]
Message-ID: <YUNcDsMkSPhIsFoI@coredump.intra.peff.net> (raw)
In-Reply-To: <20210916085523.68581-2-carenas@gmail.com>

On Thu, Sep 16, 2021 at 01:55:22AM -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_H (ex: macOS and other BSD).
> 
> Add the missing free call, xalloca_free(), which is a noop if we
> allocated memory in the stack frame, but a real free() if we
> allocated in the heap instead, and while at it, change the expression
> to match in both macros for ease of readability.

Thanks, this is definitely my bug introduced by b8ba412bf7 and this is
the right fix.

I continue to find the whole xalloca() thing pretty gross, and doubly so
now that there are _two_ layers of "maybe alloca(), and maybe malloc()"
logic (one in xalloca(), and one in this FAST_ARRAY stuff).

We should definitely take this fix to address the immediate problem, but
I wonder if this size logic should be pushed into xalloca to make this
kind of problem harder. Of course this is the only caller, so it might
not matter much either way.

(I'd also be really happy to see it go away entirely, as alloca() is a
foot-gun in the first place. But I think it did make things slightly
faster. It might be worth re-measuring).

-Peff

next prev parent reply	other threads:[~2021-09-16 15:00 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-16  2:37 [PATCH 0/2] t0000: truly leak free Carlo Marcelo Arenas Belón
2021-09-16  2:37 ` [PATCH 1/2] tree-diff: fix leak when not HAVE_ALLOCA Carlo Marcelo Arenas Belón
2021-09-16  5:17   ` Taylor Blau
2021-09-16  5:27   ` Junio C Hamano
2021-09-16  2:37 ` [PATCH 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón
2021-09-16  5:21   ` Taylor Blau
2021-09-16  6:23     ` Junio C Hamano
2021-09-16 16:49       ` Taylor Blau
2021-09-16  5:29   ` Junio C Hamano
2021-09-16 10:45   ` Ævar Arnfjörð Bjarmason
2021-09-16  8:55 ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Carlo Marcelo Arenas Belón
2021-09-16  8:55   ` [PATCH v2 1/2] tree-diff: fix leak when not HAVE_ALLOCA_H Carlo Marcelo Arenas Belón
2021-09-16 15:00     ` Jeff King [this message]
2021-09-16  8:55   ` [PATCH v2 2/2] t0000: avoid masking git exit value through pipes Carlo Marcelo Arenas Belón
2021-09-16 16:53   ` [PATCH v2 0/2] reroll for cb/plug-leaks-in-alloca-emu-users Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YUNcDsMkSPhIsFoI@coredump.intra.peff.net \
    --to=peff@peff$(echo .)net \
    --cc=carenas@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitster@pobox$(echo .)com \
    --cc=me@ttaylorr$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox