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

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2022, 23:19 UTC
Message-ID: <xmqqo7zmcydv.fsf@gitster.g>
In-Reply-To: <20220524201639.2gucdkzponddk5qt@carlos-mbp.lan>

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

Show 11 quoted lines

> On Tue, May 24, 2022 at 10:15:57AM -0700, Junio C Hamano wrote:
>> 
>> I _think_ we can even get away by not doing anything to
>> slot->finished at the end of run_active_slot(), as we are not
>> multi-threaded and the callee only returns to the caller, but if it
>> helps pleasing the warning compiler, I'd prefer the simplest
>> workaround, perhaps with an unconditional clearing there?
>
> Assuming that some overly clever compiler might optimize that out (either
> because it might think it is Undefined Behaviour or for other unknown
> reasons) then Ævar's version would be better for clearing the "warning".

You keep saying undefined behaviour but in this case I do not quite see there is anything undefined.

The warning, as Ævar said in a message, is about storing an address of an object on the stack in another object whose lifetime lasts beyond the life of the stackframe. If you dereference such a pointer _after_ we return from run_active_slot() function, the behaviour may indeed be undefined.

But if you recall one such call trace I walked through for Dscho in another message this morning, we do not make such a dereferencing. The run_active_slot() function sets up the slot with the pointer to its on-stack variable in it, we make a call chain that is several levels deep, and at some point in the call chain, the request that is represented by the slot may complete and slot may be passed to finish_active_slot(), which would update (*slot->finished) thus modifying the on-stack variable of the run_active_slot() that we will eventually return to.

Is such a use of the pointer in the structure a cause for an undefined behaviour?