Re: [PATCH v3] http API: fix dangling pointer issue noted by GCC 12.0

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 14, 2022, 15:27 UTC
Message-ID: <220414.86h76vd69t.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqy20x7eqv.fsf@gitster.g>

On Fri, Mar 25 2022, Junio C Hamano wrote:

Show 23 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
>
>> On Fri, Mar 25, 2022 at 03:34:49PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>> This isn't the only caller that assigns to "slot->finished", see see
>>
>> s/see see/see ?
>>
>>> the assignments in http-walker.c:process_alternates_response() and
>>> http.c:finish_active_slot().
>>>
>>> But those assignments are both to the pointer to our local variable
>>> here, so this fix is correct. The only way that code in http-walker.c
>>> could have done its assignments is to the pointer to this specific
>>> variable.
>>
>> Got it; this is the key piece that I was missing in my earlier review.
>> Sorry about that, this looks completely safe to me now.
>
> It does not exactly look safe to me, though.
>
> With a bit of rewrite, here is what I'd queue for now.  I really
> hope that the pre-release compiler will be fixed soon so that we do
> not have to worry about this patch.

Late reply, sorry.

I don't think the compiler is broken in this case.

Having spelunked in the GCC docs, source, commits involved & in their bugtracker I don't think they'd consider this broken. It's working as designed.

Now, of course as with any new compiler warning you might argue that it's too overzealous, but in this case it's included it a -Wall in GCC 12.0.

The warning is specifically about the local pointer being stored in a structure that survives the exit of function, and therefore if you were to dereference that pointer after the function exists the results are undefined.

Now, we happen to be able to carefully read the code in this case and reason that we won't be looking at it except in the code that's called within this function, so in practice we're OK.

But we'd have a bug sneak up on us if that wasn't the case, so it's safer to NULL this out, which is exactly what the new GCC warning is concerning itself with.

I.e. it's not promising to narrow itself to only those cases where GCC can know for absolute certain that it's a *practical* issue.

Which, basically would be asking for it to do as much work to emit it as it has to do on -fanalyzer, which makes compilation of git.git take somewhere in the mid-range of double digit minutes for me, instead of ~20s.

So I'd prefer:

 1. Adjust for release etc., but that what I submitted in
    https://lore.kernel.org/git/patch-v3-1.1-69190804c67-20220325T143322Z-avarab@gmail.com/
    go in as-is.

 2. If you're convinced this is a compiler bug could you please either
    drop the current version, or rewrite the commit envelope so that
    you're the author?

    I very much appreciate when you do local commit fix-ups for
    rewording, typos, or even rewriting something I did for the better.

    But in this case it's got my name on the envelope & has been
    reworded to say pretty much the exact opposite of what I
    think/believe about this warning.

    Which isn't something wrong or not permitted by the
    license/SOB/whatever.

    I'd just find it confusing when I'll dig this up at some point in
    the future to find my name on it, read the explanation, and then be
    perplexed why I ever thought that about this particular warning
    ... :)

Thanks.