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

[Junio C Hamano <gitster@pobox.com>]

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

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