Re: Re* jc/http-clear-finished-pointer

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Thu, 26 May 2022, Junio C Hamano wrote:

> diff --git a/http.c b/http.c
> index 229da4d148..9a98372f74 100644
> --- a/http.c
> +++ b/http.c
> @@ -1367,6 +1367,32 @@ void run_active_slot(struct active_request_slot *slot)
>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>  		}
>  	}
> +
> +	/*
> +	 * The value of slot->finished we set before the loop was used
> +	 * to set our "finished" variable when our request completed.
> +	 *
> +	 * 1. The slot may not have been reused for another requst
> +	 *    yet, in which case it still has &finished.
> +	 *
> +	 * 2. The slot may already be in-use to serve another request,
> +	 *    which can further be divided into two cases:
> +	 *
> +	 * (a) If call run_active_slot() hasn't been called for that
> +	 *     other request, slot->finished may still have the
> +	 *     address of our &finished.
> +	 *
> +	 * (b) If the request did call run_active_slot(), then the
> +	 *     call would have updated slot->finished at the beginning
> +	 *     of this function, and with the clearing of the member
> +	 *     below, we would find that slot->finished is now NULL.
> +	 *
> +	 * In all cases, slot->finished has no useful information to
> +	 * anybody at this point.  Some compilers warn us for
> +	 * attempting to smuggle a pointer that is about to become
> +	 * invalid, i.e. &finished.  We clear it here to assure them.
> +	 */
> +	slot->finished = NULL;
>  }
>
>  static void release_active_slot(struct active_request_slot *slot)
> --
> 2.36.1-306-g0dbcc0e187

I just verified that there is currently no other location in Git's code
that assigns a non-NULL value to `slot->finished` than
`run_active_slot()`. Otherwise we would potentially overwrite the value
here (which is why I preferred the conditional assignment, which does not
shut up GCC though). So for now, this solution is safe.

Having said that, it is quite puzzling that GCC thinks it is safe to
assign a local variable's pointer to a struct that is then accessed
outside the current file. This would make it easy to copy and use the
pointer well after the function scope was left. This is _not_ the case in
Git's source code, but GCC seems that this isn't possible by
(mis-)interpreting the final `slot->finished = NULL` to mean that the
`slot->finished = &finished` was safe (because it clearly isn't). In GCC's
defense, there is probably a lot of code out there that would no longer
compile if they truly enforced the new `-Wdangling-pointer` rule
correctly.

With all that, here is my ACK.

Ciao,
Dscho