mbox series

[0/3] xdiff: introduce memory allocation macros

Message ID pull.1272.git.1656516334.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series xdiff: introduce memory allocation macros | expand

Message

Eric DeCosta via GitGitGadget June 29, 2022, 3:25 p.m. UTC
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).

Phillip Wood (3):
  xdiff: introduce XDL_ALLOC_ARRAY()
  xdiff: introduce XDL_CALLOC_ARRAY()
  xdiff: introduce XDL_ALLOC_GROW()

 xdiff/xdiffi.c     |  2 +-
 xdiff/xhistogram.c | 19 ++++++-------------
 xdiff/xmacros.h    | 21 +++++++++++++++++++++
 xdiff/xpatience.c  |  9 +++------
 xdiff/xprepare.c   | 41 ++++++++++++-----------------------------
 xdiff/xutils.c     | 17 +++++++++++++++++
 xdiff/xutils.h     |  3 ++-
 7 files changed, 62 insertions(+), 50 deletions(-)


base-commit: e4a4b31577c7419497ac30cebe30d755b97752c5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1272%2Fphillipwood%2Fwip%2Fxdiff-memory-allocation-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1272/phillipwood/wip/xdiff-memory-allocation-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1272

Comments

Ævar Arnfjörð Bjarmason June 30, 2022, 10:46 a.m. UTC | #1
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().