git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH] http API: fix dangling pointer issue noted by GCC 12.0
Date: Wed, 26 Jan 2022 22:30:40 +0100	[thread overview]
Message-ID: <patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com> (raw)

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).

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".

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A look at GCC's release history suggests that GCC 12.0 will be
released sometime in the next 3-6 months:
https://gcc.gnu.org/releases.html

By addressing this new -Wall warning early we can get ahead of the
annoying compilation error we'll run into with DEVELOPER=1 once it's
released.

 http-walker.c | 4 ----
 http.c        | 8 +-------
 http.h        | 1 -
 3 files changed, 1 insertion(+), 12 deletions(-)

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;
 			if (!start_active_slot(slot)) {
 				cdata->got_alternates = -1;
 				slot->in_use = 0;
-				if (slot->finished != NULL)
-					(*slot->finished) = 1;
 			}
 			return;
 		}
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;
-
 	/* Store slot results so they can be read after the slot is reused */
 	if (slot->results != NULL) {
 		slot->results->curl_result = slot->curl_result;
@@ -1204,7 +1201,6 @@ 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);
@@ -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) {
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);
-- 
2.35.0.890.gd7e422415d9


             reply	other threads:[~2022-01-26 21:30 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-26 21:30 Ævar Arnfjörð Bjarmason [this message]
2022-01-26 21:59 ` [PATCH] http API: fix dangling pointer issue noted by GCC 12.0 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 ` [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
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-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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).