Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()

[Han-Wen Nienhuys <hanwen@google.com>]

On Fri, Apr 15, 2022 at 6:23 PM Junio C Hamano <gitster@pobox.com> wrote:
> >> git grep -e 'reftable_[mc]alloc' -e 'reftable_realloc' -A2 origin/master
> >>
> >> and I didn't see a single NULL check
> >
> > I think you're right, I retrieved that information from wetware. Looking
> > again I think I was confusing it with the if (x) free(x) fixes in
> > 34230514b83 (Merge branch 'hn/reftable-coverity-fixes', 2022-02-16).
>
> True.  Initially I hoped that these RFC changes gives us a better
> solution that comes from stepping back and rethinking the issues
> around the original "why are we calling memset() with NULL?", and

memset with NULL is an oversight.

The malloc routines are pluggable so the code could be reused for
libgit2. However, as use within Git itself is still not imminent, they
could just as well be removed as they are just a premature
generalization right now.

> if it were only "well, in _return_block() functions, we clear the
> block before calling _free()---that shouldn't be necessary, if the
> calling custom malloc-free pair cares, they can do the clearing, and
> plain vanilla free() certainly doesn't, so let's stop calling

The memset() calls on free() (eg. in  are there to tease out
use-after-free bugs in the unittests, but they should probably be
removed from file_return_block() as that is production code.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.