Re: [PATCH] http.c: clear the 'finished' member once we are done with it

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2022, 23:36 UTC
Message-ID: <xmqqleurlt31.fsf@gitster.g>
In-Reply-To: <xmqqr14jluu4.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 30 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> I stumbled over the need for this while investigating the build failures
>> caused by upgrading Git for Windows' SDK's GCC to v12.x.
>>
>>> diff --git a/http.c b/http.c
>>> index 229da4d148..85437b1980 100644
>>> --- a/http.c
>>> +++ b/http.c
>>> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)
>>>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>>>  		}
>>>  	}
>>> +
>>> +	if (slot->finished == &finished)
>>> +		slot->finished = NULL;
> ...
> I have a feeling that we've mentioned that at least twice (perhaps
> three times) in the recent past that it is in essense reverting what
> the "finished" change baa7b67d (HTTP slot reuse fixes, 2006-03-10)
> did.  We used to use the in-use bit of the slot as an indicator that
> the slot dispatched by run_active_slot() has finished (i.e. the
> in-use bit must be cleared when the request held in the struct is
> fully done), but that broke when a slot we are looking at in
> run_active_slot() is serviced (which makes in_use false), and then
> another request reuses the slot (now no longer in_use), before the
> control comes back to the loop.  "while (slot->in_use)" at the
> beginning of the loop was still true, but the original request the
> slot was being used for, the one that the run_active_slot() function
> cares about, has completed.

Given that the breakage we fixed in 2006 is about run_active_slot() calling step_active_slots() repeatedly, during which this and other requests in flight completes when curl_multi_perform() receives and handles responses, and recursively ends up calling run_active_slot() for _another_ request reusing the slot we are interested in in the codepath in the above disccussion, I _think_ we do not have to consider the case where slot->finished is pointing at somebody else's finished variable on stack here. Yes, while we repeatedly call step_active_slots(), our request in the slot may complete, the slot may be marked as unused, somebody else may reuse the slot, marking it as in_use again and using slot->finished pointer to their on-stack finsihed. But that somebody else's invocation of run_active_slot() will not give control back before their on-stack finished indicates that their recursive call to step_active_slots() completes their request. So after they come back and we exit our while() loop, either slot->finished points at our finished if slot did not get reused, or it points at an unused part of the stack that has long been rewound when we returned from the recursive call. In either case, slot->finished never points at an on-stack address of an ongoing run_active_slot() call made by somebody else that the guard I added (i.e. we must only clear it if it points our on-stack "finished") was trying to protect against clobbering.

So, I guess an unconditional assignment of

	slot->finished = NULL;

there would be sufficient.