From: Phillip Wood <phillip.wood@talktalk.net>
Date: Sat, 04 Jun 2022 12:51:07 GMT
Subject: Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop
Message-ID: <2064938784.539963.1654347067250@apps.talktalk.co.uk>
In-Reply-To: <0c296c0e-bbd4-57a6-1cab-3b98c3bc8927@web.de>


> On 03 June 2022 at 22:07 René Scharfe <l.s.r@web.de> wrote:
> 
> 
> Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:
> > Fix a bug in fd3cb0501e1 (remote: move static variables into
> > per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])
> > after having NULL'd out remote->pushurl. itself.
> >
> > While we're at it let's get rid of the redundant braces per the
> > CodingGuidelines, which also serves to show in the diff context that
> > we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's
> > keep that one.
> 
> The extended context is helping, but the brace removal makes this change
> harder to read.

Indeed, a small style fix in a larger change is one thing but here at least 80% of the changed lines are unrelated to the bugfix. I'm afraid my heart has started to sink when I see the phrase "while we're at it" in a commit message.

> [...]
> >  	FREE_AND_NULL(remote->pushurl);
> 
> Why set pushurl to NULL after release?  This results in an invalid state
> unless pushurl_nr und pushurl_alloc are reset to zero.  Same goes for
> the url array above -- either a simple free(3) call suffices or url_nr
> and url_alloc need to be cleared as well.

That's a good point

Best Wishes

Phillip