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

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Apr 25, 2022, 10:30 UTC
Message-ID: <CAFQ2z_OF5+BPxchJcydOhE5tPO2DDeuiJ0-5TzN7kTxD7TpEgA@mail.gmail.com>
In-Reply-To: <xmqq1qxyfh8k.fsf@gitster.g>

On Fri, Apr 15, 2022 at 6:23 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

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