Taylor Blau <me@xxxxxxxxxxxx> writes:
> 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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
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
