Re: [PATCH 2/2] config.mak.dev: alternative workaround to gcc 12 warning in http.c

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 16, 2022, 12:50 UTC
Message-ID: <220416.868rs59ny6.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq35id970u.fsf@gitster.g>

On Fri, Apr 15 2022, Junio C Hamano wrote:

Show 14 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> ...  We can take
>> the patch you posted and then post release we can apply the "clear
>> the .finished member as we are done with the slot" fix, which is a
>> good hygiene regardless of any compiler warning issue.
>> ...
>> At this point, my inclination is to merge these two DEVELOPER_CFLAGS
>> changes before the 2.36 final gets tagged.
>
> So, the post release longer term clean-up with log message may look
> like this.
>
> ----- >8 --------- >8 --------- >8 --------- >8 -----

Show 5 quoted lines

> [...] clear the
> finished member but make sure to limit it to the case where the
> pointer still points at the on-stack variable of ours (the pointer
> may be set to point at the on-stack variable of somebody else after
> the slot gets reused, in which case we do not want to touch it).

I'm still not sure I get this. So while we're in the run_active_slot() will we have a nested or concurrent invocation of another run_active_slot() (driven by the curl API?).

My reading of this code in get_active_slot() is that the "in_use" member is guard in "struct active_request_slot" against any such potential shenanigans, even if this was racily running multi-threaded (although then the slot selection loop itself would need some mutexing).

Then in finish_active_slot() we have since baa7b67d091 (HTTP slot reuse fixes, 2006-03-10) unconditionally clobbered "slot->finished" if it's non-NULL, without any "is it ours?" check.

Which has been my analysis of this, i.e. that at this point we "own" that member, and we won't race with anyone. We were even doing the unconditional clearing already, it was just across a function boundary. But GCC started warning about /how/ we did the clearing, so.

If it's just general paranoia OK, quite confusing though, because we're doing this in 2 places, and this puts a guard on 1/2.

But I'm asking in case you see some path through this where "the slot gets reused [by someone else, it's not our value, and] we do not want to touch it".

I don't see how that could happen *within* run_active_slot() but not between the existing code between run_active_slot() and finish_active_slot(), which is doing that clearing unconditionally...

Show 22 quoted lines

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  http.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/http.c b/http.c
> index 229da4d148..626b4051e1 100644
> --- a/http.c
> +++ b/http.c
> @@ -197,8 +197,10 @@ static void finish_active_slot(struct active_request_slot *slot)
>  	closedown_active_slot(slot);
>  	curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);
>  
> -	if (slot->finished != NULL)
> -		(*slot->finished) = 1;
> +	if (slot->finished != NULL) {
> +		*slot->finished = 1;
> +		slot->finished = NULL;
> +	}
>  
>  	/* Store slot results so they can be read after the slot is reused */
>  	if (slot->results != NULL) {