* Re: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0
2022-01-26 21:30 [PATCH] http API: fix dangling pointer issue noted by GCC 12.0 Ævar Arnfjörð Bjarmason
@ 2022-01-26 21:59 ` Taylor Blau
2022-01-27 0:50 ` Junio C Hamano
2022-02-25 9:09 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2 siblings, 0 replies; 15+ messages in thread
From: Taylor Blau @ 2022-01-26 21:59 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano
On Wed, Jan 26, 2022 at 10:30:40PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.
This sort of thing always makes me a little nervous, regardless of how
carefully it's done. I'm not sure I quite follow the above reasoning,
but let's take a look at the code...
> diff --git a/http-walker.c b/http-walker.c
> index 910fae539b8..5cc369dea85 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -225,13 +225,9 @@ static void process_alternates_response(void *callback_data)
> alt_req->url->buf);
> active_requests++;
> slot->in_use = 1;
> - if (slot->finished != NULL)
> - (*slot->finished) = 0;
Makes sense; here we set slot->in_use to 1, and would have set
slot->finished to 0.
> if (!start_active_slot(slot)) {
> cdata->got_alternates = -1;
> slot->in_use = 0;
> - if (slot->finished != NULL)
> - (*slot->finished) = 1;
Vice-versa here, OK.
> diff --git a/http.c b/http.c
> index 229da4d1488..4507c9ac9c7 100644
> --- a/http.c
> +++ b/http.c
> @@ -197,9 +197,6 @@ static void finish_active_slot(struct active_request_slot *slot)
> closedown_active_slot(slot);
> curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);
>
> - if (slot->finished != NULL)
> - (*slot->finished) = 1;
> -
But this one I don't quite follow. Or, at least, I don't readily see
that slot->in_use is necessarily going to be 0 here, or (if it isn't)
that we somehow don't care.
Could you walk me through your reasoning on why this particular hunk is
OK?
> @@ -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 part of the diff looks OK to me; you're just swapping the use of
'!finished' with 'slot->in_use', which makes sense *assuming* that they
are truly mirror images of each other.
The rest of the diff looks good to me, but I do think we should nail
down an answer to the question that I posed earlier in this message
first.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0
2022-01-26 21:30 [PATCH] http API: fix dangling pointer issue noted by GCC 12.0 Ævar Arnfjörð Bjarmason
2022-01-26 21:59 ` Taylor Blau
@ 2022-01-27 0:50 ` Junio C Hamano
2022-01-27 0:57 ` Junio C Hamano
2022-02-25 9:09 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2022-01-27 0:50 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git
Æ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);
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0
2022-01-27 0:50 ` Junio C Hamano
@ 2022-01-27 0:57 ` Junio C Hamano
2022-01-27 3:45 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2022-01-27 0:57 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git
Junio C Hamano <gitster@pobox.com> writes:
> 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.
IOW, I am wondering if this is a mistaken compiler that needs to be
told not to raise a false warning.
If the motivation behind the original "do not get fooled by a reused
slot still working on somebody else's request---instead return when
our request is done" was indeed what I speculated, then the pointer
slot->finished when we leave this function should not matter to
anybody. Would the following patch make the compiler realize that
we never smuggle a local variable's address out of this function via
a pointer in the structure?
http.c | 3 +++
1 file changed, 3 insertions(+)
diff --git c/http.c w/http.c
index 229da4d148..85437b1980 100644
--- c/http.c
+++ w/http.c
@@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)
select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
}
}
+
+ if (slot->finished == &finished)
+ slot->finished = NULL;
}
static void release_active_slot(struct active_request_slot *slot)
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0
2022-01-27 0:57 ` Junio C Hamano
@ 2022-01-27 3:45 ` Ævar Arnfjörð Bjarmason
2022-01-27 18:23 ` Junio C Hamano
0 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 3:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Wed, Jan 26 2022, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.
>
> IOW, I am wondering if this is a mistaken compiler that needs to be
> told not to raise a false warning.
>
> If the motivation behind the original "do not get fooled by a reused
> slot still working on somebody else's request---instead return when
> our request is done" was indeed what I speculated, then the pointer
> slot->finished when we leave this function should not matter to
> anybody. Would the following patch make the compiler realize that
> we never smuggle a local variable's address out of this function via
> a pointer in the structure?
>
> http.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git c/http.c w/http.c
> index 229da4d148..85437b1980 100644
> --- c/http.c
> +++ w/http.c
> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)
> select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
> }
> }
> +
> + if (slot->finished == &finished)
> + slot->finished = NULL;
> }
>
> static void release_active_slot(struct active_request_slot *slot)
Yes, that does quiet it. The GCC warning is specifically about pointers
that survive the exit of the function. From the commit that added it to
gcc.git:
+ /* 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. */
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0
2022-01-27 3:45 ` Ævar Arnfjörð Bjarmason
@ 2022-01-27 18:23 ` Junio C Hamano
0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2022-01-27 18:23 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> If the motivation behind the original "do not get fooled by a reused
>> slot still working on somebody else's request---instead return when
>> our request is done" was indeed what I speculated, then the pointer
>> slot->finished when we leave this function should not matter to
>> anybody. Would the following patch make the compiler realize that
>> we never smuggle a local variable's address out of this function via
>> a pointer in the structure?
>>
>> http.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git c/http.c w/http.c
>> index 229da4d148..85437b1980 100644
>> --- c/http.c
>> +++ w/http.c
>> @@ -1367,6 +1367,9 @@ void run_active_slot(struct active_request_slot *slot)
>> select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>> }
>> }
>> +
>> + if (slot->finished == &finished)
>> + slot->finished = NULL;
>> }
>>
>> static void release_active_slot(struct active_request_slot *slot)
>
> Yes, that does quiet it. The GCC warning is specifically about pointers
> that survive the exit of the function.
Thanks for a quick testing.
The real question is if this breaks anything. By definition, if
this changes the behaviour, it is an indication that the code has
been somehow depending on having a pointer to a stackframe that has
already gone out of scope, which is a more serious problem. I am
wondering if we need to dig further to find it out, and if so, how.
In any case, it looks like a more correct fix to address the "early
GCC 12" problem, than reverting baa7b67d (HTTP slot reuse fixes,
2006-03-10), at least to me.
Unless we devise another way to address the "slot reuse" issue, or
we come up with an explanation that the "slot reuse" issue is no
longer possible in todays code, that is.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] http API: fix dangling pointer issue noted by GCC 12.0
2022-01-26 21:30 [PATCH] http API: fix dangling pointer issue noted by GCC 12.0 Ævar Arnfjörð Bjarmason
2022-01-26 21:59 ` Taylor Blau
2022-01-27 0:50 ` Junio C Hamano
@ 2022-02-25 9:09 ` Ævar Arnfjörð Bjarmason
2022-02-25 22:58 ` Junio C Hamano
` (2 more replies)
2 siblings, 3 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-02-25 9:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau, Ævar Arnfjörð Bjarmason
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.
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>
---
A much simpler fix for a warning new in the GCC v12 pre-release.
Range-diff against v1:
1: 1cec367e805 ! 1: 777838267a5 http API: fix dangling pointer issue noted by GCC 12.0
@@ Commit message
built from gcc.git's 8bc700f4c3f (Enhance vec_pack_trunc for integral
mode mask., 2022-01-17).
- To fix this I first simply made the member "int finished",
- i.e. removing the pointer indirection. It turns out that nothing cared
- about the state of it being a NULL pointer v.s. "*ptr == 0".
+ 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:
- 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:
+ /* 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. */
+ [...]
- 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");
+ 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.
- 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.
+ 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>
- ## http-walker.c ##
-@@ http-walker.c: static void process_alternates_response(void *callback_data)
- alt_req->url->buf);
- active_requests++;
- slot->in_use = 1;
-- if (slot->finished != NULL)
-- (*slot->finished) = 0;
- if (!start_active_slot(slot)) {
- cdata->got_alternates = -1;
- slot->in_use = 0;
-- if (slot->finished != NULL)
-- (*slot->finished) = 1;
- }
- return;
- }
-
## http.c ##
-@@ http.c: static void finish_active_slot(struct active_request_slot *slot)
- closedown_active_slot(slot);
- curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);
-
-- if (slot->finished != NULL)
-- (*slot->finished) = 1;
--
- /* Store slot results so they can be read after the slot is reused */
- if (slot->results != NULL) {
- slot->results->curl_result = slot->curl_result;
-@@ http.c: struct active_request_slot *get_active_slot(void)
- active_requests++;
- slot->in_use = 1;
- slot->results = NULL;
-- slot->finished = NULL;
- slot->callback_data = NULL;
- slot->callback_func = NULL;
- curl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);
@@ http.c: 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();
+ select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
+ }
+ }
++ slot->finished = NULL;
+ }
- if (slot->in_use) {
-
- ## http.h ##
-@@ http.h: 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);
+ static void release_active_slot(struct active_request_slot *slot)
http.c | 1 +
1 file changed, 1 insertion(+)
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;
}
static void release_active_slot(struct active_request_slot *slot)
--
2.35.1.1175.gf9e1b23ea35
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2] http API: fix dangling pointer issue noted by GCC 12.0
2022-02-25 9:09 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
@ 2022-02-25 22:58 ` Junio C Hamano
2022-02-26 18:01 ` Taylor Blau
2022-03-25 14:34 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2022-02-25 22:58 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Taylor Blau
Æ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.
.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] http API: fix dangling pointer issue noted by GCC 12.0
2022-02-25 9:09 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2022-02-25 22:58 ` Junio C Hamano
@ 2022-02-26 18:01 ` Taylor Blau
2022-03-25 14:34 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2 siblings, 0 replies; 15+ messages in thread
From: Taylor Blau @ 2022-02-26 18:01 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano
On Fri, Feb 25, 2022 at 10:09:11AM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.
I'm not quite sure that I follow this. This isn't the only spot that
assigns non-NULL to "slot->finished", see the assignments in
http-walker.c:process_alternates_response() and
http.c:finish_active_slot().
But even if it were, I'm not sure how this being the only spot that
*writes* non-NULL matters from a reader's perspective.
Looking more at process_alternates_response(), it really looks like this
variable wants to hold a tri-state value. I wonder if it would be
clearer to replace the NULL/(pointer to) 0/(pointer to) 1 with a
UNKNOWN/TRUE/FALSE enum.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] http API: fix dangling pointer issue noted by GCC 12.0
2022-02-25 9:09 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2022-02-25 22:58 ` Junio C Hamano
2022-02-26 18:01 ` Taylor Blau
@ 2022-03-25 14:34 ` Ævar Arnfjörð Bjarmason
2022-03-25 18:11 ` Taylor Blau
2 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-03-25 14:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Taylor Blau, Ævar Arnfjörð Bjarmason
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().
This isn't the only caller that assigns to "slot->finished", 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.
It was suggested[2] to guard that with "if (slot->finished ==
&finished)", but that'll still trigger the warning.
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>
---
This should clarify the feedback on v2, sorry about the very late
re-roll.
I.e. I *meant* in v2 that it's the only assignment to slot->finished
itself is here, the other assignments in http-walker.c are to the
pointer to our variable here.
Range-diff against v2:
1: 777838267a5 ! 1: 69190804c67 http API: fix dangling pointer issue noted by GCC 12.0
@@ Commit message
[...]
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.
+ NULL to "slot->finished" at the end of run_active_slot().
+
+ This isn't the only caller that assigns to "slot->finished", 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.
+
+ It was suggested[2] to guard that with "if (slot->finished ==
+ &finished)", but that'll still trigger the warning.
1. https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9d6a0f388eb048f8d87f47af78f07b5ce513bfe6
2. https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/
http.c | 1 +
1 file changed, 1 insertion(+)
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;
}
static void release_active_slot(struct active_request_slot *slot)
--
2.35.1.1509.ge4eeb5bd39e
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3] http API: fix dangling pointer issue noted by GCC 12.0
2022-03-25 14:34 ` [PATCH v3] " Ævar Arnfjörð Bjarmason
@ 2022-03-25 18:11 ` Taylor Blau
2022-03-26 0:13 ` Junio C Hamano
0 siblings, 1 reply; 15+ messages in thread
From: Taylor Blau @ 2022-03-25 18:11 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano, Taylor Blau
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.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] http API: fix dangling pointer issue noted by GCC 12.0
2022-03-25 18:11 ` Taylor Blau
@ 2022-03-26 0:13 ` Junio C Hamano
2022-04-14 15:27 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2022-03-26 0:13 UTC (permalink / raw)
To: Taylor Blau; +Cc: Ævar Arnfjörð Bjarmason, git
Taylor Blau <me@ttaylorr.com> 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@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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3] http API: fix dangling pointer issue noted by GCC 12.0
2022-03-26 0:13 ` Junio C Hamano
@ 2022-04-14 15:27 ` Ævar Arnfjörð Bjarmason
2022-04-14 17:04 ` Junio C Hamano
0 siblings, 1 reply; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-04-14 15:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
On Fri, Mar 25 2022, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> 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.
Late reply, sorry.
I don't think the compiler is broken in this case.
Having spelunked in the GCC docs, source, commits involved & in their
bugtracker I don't think they'd consider this broken. It's working as
designed.
Now, of course as with any new compiler warning you might argue that
it's too overzealous, but in this case it's included it a -Wall in GCC
12.0.
The warning is specifically about the local pointer being stored in a
structure that survives the exit of function, and therefore if you were
to dereference that pointer after the function exists the results are
undefined.
Now, we happen to be able to carefully read the code in this case and
reason that we won't be looking at it except in the code that's called
within this function, so in practice we're OK.
But we'd have a bug sneak up on us if that wasn't the case, so it's
safer to NULL this out, which is exactly what the new GCC warning is
concerning itself with.
I.e. it's not promising to narrow itself to only those cases where GCC
can know for absolute certain that it's a *practical* issue.
Which, basically would be asking for it to do as much work to emit it as
it has to do on -fanalyzer, which makes compilation of git.git take
somewhere in the mid-range of double digit minutes for me, instead of
~20s.
So I'd prefer:
1. Adjust for release etc., but that what I submitted in
https://lore.kernel.org/git/patch-v3-1.1-69190804c67-20220325T143322Z-avarab@gmail.com/
go in as-is.
2. If you're convinced this is a compiler bug could you please either
drop the current version, or rewrite the commit envelope so that
you're the author?
I very much appreciate when you do local commit fix-ups for
rewording, typos, or even rewriting something I did for the better.
But in this case it's got my name on the envelope & has been
reworded to say pretty much the exact opposite of what I
think/believe about this warning.
Which isn't something wrong or not permitted by the
license/SOB/whatever.
I'd just find it confusing when I'll dig this up at some point in
the future to find my name on it, read the explanation, and then be
perplexed why I ever thought that about this particular warning
... :)
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3] http API: fix dangling pointer issue noted by GCC 12.0
2022-04-14 15:27 ` Ævar Arnfjörð Bjarmason
@ 2022-04-14 17:04 ` Junio C Hamano
2022-04-15 13:30 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2022-04-14 17:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Taylor Blau, git
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Having spelunked in the GCC docs, source, commits involved & in their
> bugtracker I don't think they'd consider this broken. It's working as
> designed.
>
> Now, of course as with any new compiler warning you might argue that
> it's too overzealous, but in this case it's included it a -Wall in GCC
> 12.0.
Whatever. I do not care if this is a broken or wai from GCC's point
of view.
I care more about the correct operation of the production code of
ours, than a workaround to squelch a warning, overzealous or not, by
a compiler, and if it is not clear that the workaround is obviously
free of negative side effect, we do not want to deliberately introduce
potential breakage in our code base just for that.
And I still do not see how it is safe to unconditionally clearing
the pointer in the slot instance to NULL. It was asked late January
in https://lore.kernel.org/git/xmqqv8y52g3a.fsf@gitster.g/
In other words, what we should have been spelunking is *not* in the
GCC docs and code, but the http codepath in our code that depends on
the slot not being cleared when we didn't set up the pointer in the
current recursion of that function. With a clear description on how
this change is safe, with a clear understanding on why the pointer
member "finished" was added in the first place to avoid the same
mistake as an earlier round of this patch [*1*], it would become an
acceptable patch, with or without GCC warning.
Namely, the finding in this part of a review comment [*2*]
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.
that was made to explain why the pointer member is there, and a
possible case that the code before the introduction of the pointer
would misbehave and today's code would behave better, worries me the
most, as unconditionally assigning NULL there like this patch does
without any guard would mean that we are breaking the code exactly
in such a case, I would think.
In short, I do not care who takes the credit, I care more about the
correctness of the code than a warning by a version of a compiler, I
do not care at all if the compiler writers considers the warning a
bug, and I worry that the change proposed, while it may certainly
squelch the bug, may break the code that has been working happily,
and I haven't seen a clear explanation why it is not the case.
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. Nobody seems to have
explained how it is the case.
[References]
*1* https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/
*2* https://lore.kernel.org/git/xmqqczkengsg.fsf@gitster.g/
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3] http API: fix dangling pointer issue noted by GCC 12.0
2022-04-14 17:04 ` Junio C Hamano
@ 2022-04-15 13:30 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 15+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-04-15 13:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
On Thu, Apr 14 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Having spelunked in the GCC docs, source, commits involved & in their
>> bugtracker I don't think they'd consider this broken. It's working as
>> designed.
>>
>> Now, of course as with any new compiler warning you might argue that
>> it's too overzealous, but in this case it's included it a -Wall in GCC
>> 12.0.
>
> Whatever. I do not care if this is a broken or wai from GCC's point
> of view.
>
> I care more about the correct operation of the production code of
> ours, than a workaround to squelch a warning, overzealous or not, by
> a compiler, and if it is not clear that the workaround is obviously
> free of negative side effect, we do not want to deliberately introduce
> potential breakage in our code base just for that.
>
> And I still do not see how it is safe to unconditionally clearing
> the pointer in the slot instance to NULL. It was asked late January
> in https://lore.kernel.org/git/xmqqv8y52g3a.fsf@gitster.g/
The v3 re-roll at
https://lore.kernel.org/git/patch-v3-1.1-69190804c67-20220325T143322Z-avarab@gmail.com/
(see range diff) was intended to address both your question there &
Taylor's at https://lore.kernel.org/git/YhprAb1f1WYIktCV@nand.local/.
> In other words, what we should have been spelunking is *not* in the
> GCC docs and code, but the http codepath in our code that depends on
> the slot not being cleared when we didn't set up the pointer in the
> current recursion of that function. With a clear description on how
> this change is safe, with a clear understanding on why the pointer
> member "finished" was added in the first place to avoid the same
> mistake as an earlier round of this patch [*1*], it would become an
> acceptable patch, with or without GCC warning.
>
> Namely, the finding in this part of a review comment [*2*]
>
> 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.
>
> that was made to explain why the pointer member is there, and a
> possible case that the code before the introduction of the pointer
> would misbehave and today's code would behave better, worries me the
> most, as unconditionally assigning NULL there like this patch does
> without any guard would mean that we are breaking the code exactly
> in such a case, I would think.
I think that accurately describes a logic error in the v1 of this patch,
i.e. we can't remove the "finished" member, but per the v3 explanation I
believe (re-)setting it to NULL is safe.
> In short, I do not care who takes the credit, I care more about the
> correctness of the code than a warning by a version of a compiler, I
> do not care at all if the compiler writers considers the warning a
> bug, and I worry that the change proposed, while it may certainly
> squelch the bug, may break the code that has been working happily,
> and I haven't seen a clear explanation why it is not the case.
>
> 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. Nobody seems to have
> explained how it is the case.
I tried to explain that in the v3, but that was part of what you
edited/amended in your applied version of it.
I don't know how to answer your concerns/questions other than as I've
already done there.
^ permalink raw reply [flat|nested] 15+ messages in thread