Subject: Re: [PATCH 0/3] xdiff: introduce memory allocation macros

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 30 Jun 2022 12:46:22 +0200

On Wed, Jun 29 2022, Phillip Wood via GitGitGadget wrote:

> This patch series introduces macros for allocating and growing arrays in
> xdiff. The macros are similar to ALLOC_ARRAY()/ALLOC_GROW() from the rest of
> the code base but return an error on failure to allow libgit2 to handle
> memory allocation failures gracefully rather than dying. The macros
> introduce overflow checks but these checks are currently redundant as we
> limit the maximum file size passed to xdiff and these checks alone are
> insufficient to safely remove the size limit. The aim of this series is to
> make the xdiff code more readable, there should be no change in behavior (as
> such I'm open to the argument that these are just churn and should be
> dropped).

I think it's a good direction, but why not make such new macros
non-XDL_* specific, add them to git-compat-util.h, and then define our
existing macros that call xmalloc() now in terms of these new macros?

I realize that it'll take a bit more careful hacking in wrapper.c and
git-compat-util.h, but it would allow us to eventually make some other
low-level APIs of ours use such an API.

E.g. we have some hand-rolled replacements for "struct strbuf" in at
least a couple of places (e.g. vreportf() in usage.c). If you pull on
that thread you'll see that it's for no reason other than strbuf.c calls
ALLOC_GROW(), which we'll die() in, and we don't want to die on malloc
failure in e.g. BUG().