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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2022, 14:23 UTC
Message-ID: <xmqq1qxx6rab.fsf@gitster.g>
In-Reply-To: <220416.86lew5akh1.gmgdl@evledraar.gmail.com>

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

> I.e. the assignment earlier in the function is unconditional, why
> wouldn't the clearing of the data correspond to that assignment and
> clear it unconditionally?

The original problem description that introduced .finished member indicates that inside the while() loop, the same slot object can be completed (by feeding it to finish_active_slot(), which would also clears its in_use thus making it reusable) and then later be reused (by using it for a different request).

The dispatching is done by calling step_active_slots() repeatedly inside the loop and I do not think there is any multi-threaded concurrency to worry about here. The protection is against a case where such a slot, which was originally ours and pointed at our on-stack finished variable with its finished member, is reused for a different request, and its finished member is used in a similar way to point at the on-stack finish variable in somebody else's stackframe in the future code. If the slot instance we were using as ours upon the entry of this function is being used for another request already (the fix that required the .finished member is an enough explanation that it is a real concern), after we leave the loop, the slot instance is no longer ours, so we need to be careful when we clear it.

At the entry of this function, the story is vastly different. The slot instance belongs to us---the caller chose the slot and decided to use it to service a particular request and threw the slot instance at us, so there is nothing wrong to unconditionally use the .finished member of the slot and point it at a variable in our stackframe. But after the loop leaves, and the slot may or may not be already reused to hold another request. If .finished is set and it is the value that points at the variable in our stackframe, then we are the only one who could have set that and it is safe to clear. Any other value other than NULL, we do not know at that point who set it, and it is being used for a request that we have nothing to do with. That is why we want to refrain from touching it when it is not clearly ours.