On Fri, Apr 15 2022, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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 -----
> [...] 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...
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> 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) {
