Re: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0

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

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

> The pre-release GCC 12.0 development branch has a new warning about
> dangling pointers in -Wall:
>
>     http.c: In function ‘run_active_slot’:
>     http.c:1332:24: error: storing the address of local variable ‘finished’ in ‘*slot.finished’ [-Werror=dangling-pointer=]
>      1332 |         slot->finished = &finished;
>           |         ~~~~~~~~~~~~~~~^~~~~~~~~~~
>     http.c:1330:13: note: ‘finished’ declared here
>      1330 |         int finished = 0;
>           |             ^~~~~~~~
>
> This is on a locally built "gcc (GCC) 12.0.1 20220120 (experimental)",
> built from gcc.git's 8bc700f4c3f (Enhance vec_pack_trunc for integral
> mode mask., 2022-01-17).

I am puzzled by this error.  The assignment is the only one that
assigns a real pointer to the .finished member, and until
finish_active_slot() is called on the slot, the loop would not
leave.  I would understand the error if slot->finished is used after
the function returns to the caller, but I do not think it is the
case.

The original code is also curious in that finished is a pointer to
somewhere else other than a member that records a yes/no itself.

> But we can instead amend the code added in baa7b67d091 (HTTP slot
> reuse fixes, 2006-03-10) to get rid of "int *finished" entirely. I
> instrumented the code to add this after every use of slot->finished or
> slot->in_use:
>
>     if (slot->finished && slot->in_use == *slot->finished) BUG("in-use = %d and finished = %d disconnect", slot->in_use, *slot->finished);
>     if (!slot->finished && !slot->in_use) BUG("have !in-use and no finished pointer");
>
> Which never fires, but we would get occurrences of:
>
>     if (!slot->finished && slot->in_use) BUG("have in-use and no finished pointer");
>
> I.e. we can simply drop the field and rely on "slot->in_use" in cases
> where we used "finished" before. The two fields were mirror images of
> each other, and the tri-state nature of "finished" wasn't something we
> relied upon.

Sorry, but "instrument and ran test" does not give much confidence.

What was the original problem the "finished" member wanted to solve,
and why was the problem unsolveable by simply looking at the in_use
member that already existed back then?  Isn't the issue very much
timing dependent, influenced by the way the HTTP server we are
interacting with behaves?  Before baa7b67d091acf, the loop in
run_active_slot() checked _exactly_ the same "slot->in_use" field,
which the above analysis claims to be the mirror image of "finished",
so I find that ...

> @@ -1327,10 +1323,8 @@ void run_active_slot(struct active_request_slot *slot)
>  	fd_set excfds;
>  	int max_fd;
>  	struct timeval select_timeout;
> -	int finished = 0;
>  
> -	slot->finished = &finished;
> -	while (!finished) {
> +	while (slot->in_use) {
>  		step_active_slots();
>  
>  		if (slot->in_use) {

... this reversion of baa7b67d091acf is not very well explained.

The only way the separation could make a difference is while
step_active_slots(), the current slot is completed, our local
"finished" gets marked as such thanks to the pointer-ness of the
finished member, and then another pending request is started reusing
the same slot object (properly initialized for that new request).
In such a case, the while loop we want to see exit will see that
slot->in_use member is _still_ true, even though it is true only
because it is now about a separate and unrelated request that is
still waiting for completion, and the original request the caller is
waiting for has already finished.

And if that is the kind of racy interaction between requests the
original code wanted to fix, then I am not sure how this updated
code deals with the issue in a different way.  Can we safely tell if
the original request, held in slot when this function was entered,
has completed by looking at slot->in_use member?  When the while()
loop sees that slot->in_use member is true, how do we know if it is
true because nothing has been done to the request yet, or we've
completed the original request but another request in queue has
replacd the same slot object and the slot is now in-use again?

> diff --git a/http.h b/http.h
> index df1590e53a4..81418d5fd8b 100644
> --- a/http.h
> +++ b/http.h
> @@ -24,7 +24,6 @@ struct active_request_slot {
>  	int in_use;
>  	CURLcode curl_result;
>  	long http_code;
> -	int *finished;
>  	struct slot_results *results;
>  	void *callback_data;
>  	void (*callback_func)(void *data);