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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 30, 2022, 10:46 UTC
Message-ID: <220630.86h742e81q.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1272.git.1656516334.gitgitgadget@gmail.com>

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

Show 10 quoted lines

> 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().