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

From: Jeff King <peff@peff.net>
Date: Jul 11, 2022, 09:33 UTC
Message-ID: <YsvuYX7xekLGIdVt@coredump.intra.peff.net>
In-Reply-To: <220708.864jzr9sq4.gmgdl@evledraar.gmail.com>

On Fri, Jul 08, 2022 at 11:47:58PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

I suspect that introduces new complexities, as some calls really may want an actual no-frills malloc. I'm thinking stuff in compat/, for example, where extra actions taken by xmalloc() could cause weird looping or races. This was probably a lot more likely when we closed packs via xmalloc (e.g., via git_mmap()'s malloc() call) but I think the general principle still holds. E.g., gitsetenv() calling getenv() is a potential questionable area.

It does look like the call in submodule--helper.c is just wrong, though, and should be changed. You could probably detect these with the preprocessor, but again, you run into complexities with the cases that _should_ be vanilla malloc. Given how little of a problem this has been historically, I'm mostly content to notice these in review and occasionally grep for fixes.

I suppose a coccinelle rule could help, because it makes it easy to suppress false positives (like all of compat/), which the preprocessor doesn't.

-Peff