From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Taylor Blau" <me@ttaylorr.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2] http API: fix dangling pointer issue noted by GCC 12.0
Date: Fri, 25 Feb 2022 10:09:11 +0100 [thread overview]
Message-ID: <patch-v2-1.1-777838267a5-20220225T090816Z-avarab@gmail.com> (raw)
In-Reply-To: <patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com>
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
next prev parent reply other threads:[~2022-02-25 9:09 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
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-01-27 3:45 ` Ævar Arnfjörð Bjarmason
2022-01-27 18:23 ` Junio C Hamano
2022-02-25 9:09 ` Ævar Arnfjörð Bjarmason [this message]
2022-02-25 22:58 ` [PATCH v2] " Junio C Hamano
2022-02-26 18:01 ` Taylor Blau
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
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=patch-v2-1.1-777838267a5-20220225T090816Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).