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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2022, 17:04 UTC
Message-ID: <xmqqo8131tr8.fsf@gitster.g>
In-Reply-To: <220414.86h76vd69t.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 7 quoted lines

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

Whatever. I do not care if this is a broken or wai from GCC's point of view.

I care more about the correct operation of the production code of ours, than a workaround to squelch a warning, overzealous or not, by a compiler, and if it is not clear that the workaround is obviously free of negative side effect, we do not want to deliberately introduce potential breakage in our code base just for that.

And I still do not see how it is safe to unconditionally clearing the pointer in the slot instance to NULL. It was asked late January in https://lore.kernel.org/git/xmqqv8y52g3a.fsf@gitster.g/

In other words, what we should have been spelunking is *not* in the GCC docs and code, but the http codepath in our code that depends on the slot not being cleared when we didn't set up the pointer in the current recursion of that function. With a clear description on how this change is safe, with a clear understanding on why the pointer member "finished" was added in the first place to avoid the same mistake as an earlier round of this patch [*1*], it would become an acceptable patch, with or without GCC warning.

Namely, the finding in this part of a review comment [*2*]

    The only way the separation could make a difference is while
    step_active_slots(), the current slot is completed, our local
    "finished" gets marked as such thanks to the pointer-ness of the
    finished member, and then another pending request is started
    reusing the same slot object (properly initialized for that new
    request).  In such a case, the while loop we want to see exit
    will see that slot->in_use member is _still_ true, even though
    it is true only because it is now about a separate and unrelated
    request that is still waiting for completion, and the original
    request the caller is waiting for has already finished.

that was made to explain why the pointer member is there, and a possible case that the code before the introduction of the pointer would misbehave and today's code would behave better, worries me the most, as unconditionally assigning NULL there like this patch does without any guard would mean that we are breaking the code exactly in such a case, I would think.

In short, I do not care who takes the credit, I care more about the correctness of the code than a warning by a version of a compiler, I do not care at all if the compiler writers considers the warning a bug, and I worry that the change proposed, while it may certainly squelch the bug, may break the code that has been working happily, and I haven't seen a clear explanation why it is not the case.

As long as the same slot is never passed to run_active_slot() recursively, clearing the member unconditionally when the control leaves the function should not break the code. Nobody seems to have explained how it is the case.

[References]

*1* https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/ *2* https://lore.kernel.org/git/xmqqczkengsg.fsf@gitster.g/