Re: [PATCH v2] 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).
>
> The GCC warning is specifically about pointers that survive the exit
> of the function. See a comment added to
> "pass_waccess::use_after_inval_p" in the GCC commit that added the
> warning, or:
>
>     /* The use is one of a dangling pointer if a clobber of the variable
>       [the pointer points to] has not been found before the function exit
>       point.  */
>     [...]
>
> There's a few possible ways to fix this, but the simplest is to assign
> NULL to "slot->finished" at the end of run_active_slot(), it's the
> only caller that ever assigns non-NULL to it. It was suggested[2] to
> guard that with "if (slot->finished == &finished)", but that'll still
> trigger the warning.

Does that mean we can clobber the finished member of a slot that was
in use, not the one we prepared, because we do not make sure we do
not clobber slot->finished that other people set up?

I think this change is safe, but it's been quite a while since I
played with dumb HTTP walker the last time, so I no longer trust my
own reading of this code X-<.

> diff --git a/http.c b/http.c
> index 229da4d1488..2f67fbb33cd 100644
> --- a/http.c
> +++ b/http.c
> @@ -1367,6 +1367,7 @@ void run_active_slot(struct active_request_slot *slot)
>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>  		}
>  	}
> +	slot->finished = NULL;
>  }

Will queue, but it would be nice if GCC can get fixed before we have
to advance this to 'next' and below

Thanks.
.