Re: [PATCH 6/7] xdiff: remove xdl_malloc() wrapper, use malloc(), not xmalloc()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 8, 2022, 21:47 UTC
Message-ID: <220708.864jzr9sq4.gmgdl@evledraar.gmail.com>
In-Reply-To: <YsiHGliw2dYmGcDY@coredump.intra.peff.net>

On Fri, Jul 08 2022, Jeff King wrote:

Show 20 quoted lines

> On Fri, Jul 08, 2022 at 04:20:18PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> As noted in 36c83197249 (xdiff: use xmalloc/xrealloc, 2019-04-11) the
>> reason we have xdl_malloc() in the first place was to make the xdiff
>> code safer, it was not handling some allocation failures correctly at
>> the time.
>> 
>> But as noted in that commit doing this was intended as a stopgap, as
>> various code in xdiff/* did not correctly handle allocation failures,
>> and would have segfaulted if malloc() returned NULL.
>> 
>> But since then and after preceding commits we can be confident that
>> malloc() failures are handled correctly. All of these users of
>> xdl_malloc() are checking their return values, and we're returning
>> -1 (or similar ) to the top-level in case of failures.
>
> This is also losing the other parts mentioned in 36c83197249:
> respecting GIT_ALLOC_LIMIT and any memory reclamation strategies.
>
> I think you'd want an xmalloc_gently() instead of a raw malloc().

...

> For the same reason, I suspect it's better to leave this with a layer of
> preprocessor indirection. Even if we chose to use malloc() here, libgit2
> might not, and having the macro makes it easier to share the code.

I think https://lore.kernel.org/git/220708.86czef9t6y.gmgdl@evledraar.gmail.com/ in the related side-thread shows a workable way to do that.

But I didn't think about the GIT_ALLOC_LIMIT, so if we want that still we'd need to have this code routed to not just malloc() but an "xmalloc_gently()" as you suggest.

But we also have a few other uses of malloc() in the codebase. I wonder if the right thing here isn't to just use malloc(), but to have git-compat-util.h override malloc() (similar to how we we override e.g. exit() now...), which would also catch those.

Or we could just say that it's not worth the complexity, and xdiff won't respect GIT_ALLOC_LIMIT .. :(