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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 26, 2022, 00:13 UTC
Message-ID: <xmqqy20x7eqv.fsf@gitster.g>
In-Reply-To: <Yj4FwuyEW0b5ImEC@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 15 quoted lines

> On Fri, Mar 25, 2022 at 03:34:49PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> This isn't the only caller that assigns to "slot->finished", see see
>
> s/see see/see ?
>
>> the assignments in http-walker.c:process_alternates_response() and
>> http.c:finish_active_slot().
>>
>> But those assignments are both to the pointer to our local variable
>> here, so this fix is correct. The only way that code in http-walker.c
>> could have done its assignments is to the pointer to this specific
>> variable.
>
> Got it; this is the key piece that I was missing in my earlier review.
> Sorry about that, this looks completely safe to me now.

It does not exactly look safe to me, though.

With a bit of rewrite, here is what I'd queue for now. I really hope that the pre-release compiler will be fixed soon so that we do not have to worry about this patch.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Fri, 25 Mar 2022 15:34:49 +0100

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 was suggested[2] to guard that with "if (slot->finished == &finished)", but that'll still trigger the bogus warning from the compiler.

This is the only place that assigns to "slot->finished"; other mention of slot->finished in http-walker.c:process_alternates_response() and http.c:finish_active_slot() are assignments through the pointer, and not moving where the pointer points at.

As long as the same slot is never passed to run_active_slot() recursively, clearing the member unconditionally when the control leaves the function should not break the code. Knock wood, as nobody seems to have made sure if that is the case.

We could add

	if (slot->finished)
		BUG("the uncoditional clearing at the end is wrong");

early in run_active_slot(), before we assign the pointer to our on-stack variable to this field, to give us such a guarantee, though.

1. https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9d6a0f388eb048f8d87f47af78f07b5ce513bfe6
2. https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/http.c b/http.c
index 229da4d148..2f67fbb33c 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;
 }
 
 static void release_active_slot(struct active_request_slot *slot)

-- 
2.35.1-832-gfc83ccf5d8