Re: [PATCH v2] http API: fix dangling pointer issue noted by GCC 12.0
From: Junio C Hamano <hidden>
Date: 2022-02-25 22:58:44
Ævar Arnfjörð Bjarmason [off-list ref] 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-<.
quoted hunk
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. .