Re: [RFC PATCH 01/15] remote.c: don't dereference NULL in freeing loop
Show 15 quoted lines
> 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.
Show 7 quoted lines
> [...]
> > 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