diff mbox series

[3/4] http.c: avoid danging pointer to local variable `finished`

Message ID 4a4e0aa0a49a54eea88f9c2d8e1db6a697012718.1653351786.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series ci: fix windows-build with GCC v12.x | expand

Commit Message

Johannes Schindelin May 24, 2022, 12:23 a.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

In http.c, the run_active_slot() function allows the given "slot" to
make progress by calling step_active_slots() in a loop repeatedly, and
the loop is not left until the request held in the slot completes.

Ages ago, we used to use the slot->in_use member to get out of the loop,
which misbehaved when the request in "slot" completes (at which time,
the result of the request is copied away from the slot, and the in_use
member is cleared, making the slot ready to be reused), and the "slot"
gets reused to service a different request (at which time, the "slot"
becomes in_use again, even though it is for a different request).  The
loop terminating condition mistakenly thought that the original request
has yet to be completed.

Today's code, after baa7b67d (HTTP slot reuse fixes, 2006-03-10) fixed
this issue, uses a separate "slot->finished" member that is set in
run_active_slot() to point to an on-stack variable, and the code that
completes the request in finish_active_slot() clears the on-stack
variable via the pointer to signal that the particular request held by
the slot has completed.  It also clears the in_use member (as before
that fix), so that the slot itself can safely be reused for an unrelated
request.

One thing that is not quite clean in this arrangement is that, unless
the slot gets reused, at which point the finished member is reset to
NULL, the member keeps the value of &finished, which becomes a dangling
pointer into the stack when run_active_slot() returns.

Let's drop that local variable and introduce a new flag in the slot that
is used to indicate that even while the slot is no longer in use, it is
still reserved until further notice. It is the responsibility of
`run_active_slot()` to clear that flag once it is done with that slot.

Initial-patch-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 http-walker.c |  4 ----
 http.c        | 15 +++++++--------
 http.h        |  2 +-
 3 files changed, 8 insertions(+), 13 deletions(-)

Comments

Ævar Arnfjörð Bjarmason May 24, 2022, 7:58 a.m. UTC | #1
On Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [...]
> Let's drop that local variable and introduce a new flag in the slot that
> is used to indicate that even while the slot is no longer in use, it is
> still reserved until further notice. It is the responsibility of
> `run_active_slot()` to clear that flag once it is done with that slot.
>
> Initial-patch-by: Junio C Hamano <gitster@pobox.com>

Don't you mean by me?
I.e. https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/

This seems to be derived from that, or perhaps you just came up with
something similar independently. Junio then came up with the smaller
https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/
Junio C Hamano May 24, 2022, 8:06 p.m. UTC | #2
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> [...]
>> Let's drop that local variable and introduce a new flag in the slot that
>> is used to indicate that even while the slot is no longer in use, it is
>> still reserved until further notice. It is the responsibility of
>> `run_active_slot()` to clear that flag once it is done with that slot.
>>
>> Initial-patch-by: Junio C Hamano <gitster@pobox.com>
>
> Don't you mean by me?
> I.e. https://lore.kernel.org/git/patch-1.1-1cec367e805-20220126T212921Z-avarab@gmail.com/

Most likely, but this version is so distant from the "clear
slot->finished before leaving run_active_slot()" Dscho and I were
recently discussing, that I do not think it can be said to have been
derived from that one.  This is completely a different patch that
makes different changes.

The "clear slot->finished", by the way, is what I think is the right
thing to do, especially that the objective is to squelch the false
positive warning from a new compiler.  If there is a way to annotate
the line for the compiler to tell it not to warn about it, that would
have been even better.

> This seems to be derived from that, or perhaps you just came up with
> something similar independently. Junio then came up with the smaller
> https://lore.kernel.org/git/xmqq8rv2nggn.fsf@gitster.g/

I actually do not think so.  Yours is revert of the existing fix the
compiler is confused about, and I have a feeling that if the original
fix is still relevant, the problem the original fix wanted to address
will resurface as a regression.

If I am reading the patch correctly, Dscho's is to avoid [*] reusing
a slot while any run_active_slot() is still waiting for its
completion.  The approach would solve the problem the original fix
wanted to solve in a different way.  Personally I do not think such
a surgery is necessary only to squelch false positives from a new
warning compiler, though.


[Footnote] 

 * I said "is to avoid", not "avoids", because I haven't studied the
   patch with sufficient degree of carefulness to say for sure, even
   though I can see that is the intent.
Johannes Schindelin May 24, 2022, 9:15 p.m. UTC | #3
Hi Junio,

On Tue, 24 May 2022, Junio C Hamano wrote:

> The "clear slot->finished", by the way, is what I think is the right
> thing to do, especially that the objective is to squelch the false
> positive warning from a new compiler.  If there is a way to annotate
> the line for the compiler to tell it not to warn about it, that would
> have been even better.

We could do something like this:

-- snip --
diff --git a/http.c b/http.c
index b08795715f8a..2ac8d51d3668 100644
--- a/http.c
+++ b/http.c
@@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
 	struct timeval select_timeout;
 	int finished = 0;

+#if __GNUC__ >= 12
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdangling-pointer"
+#endif
 	slot->finished = &finished;
+#if __GNUC__ >= 12
+#pragma GCC diagnostic pop
+#endif
 	while (!finished) {
 		step_active_slots();
-- snap --

That's quite ugly, though. And what's worse, it is pretty unreadable, too.

Ciao,
Dscho
Ævar Arnfjörð Bjarmason May 24, 2022, 9:45 p.m. UTC | #4
On Tue, May 24 2022, Johannes Schindelin wrote:

> Hi Junio,
>
> On Tue, 24 May 2022, Junio C Hamano wrote:
>
>> The "clear slot->finished", by the way, is what I think is the right
>> thing to do, especially that the objective is to squelch the false
>> positive warning from a new compiler.  If there is a way to annotate
>> the line for the compiler to tell it not to warn about it, that would
>> have been even better.
>
> We could do something like this:
>
> -- snip --
> diff --git a/http.c b/http.c
> index b08795715f8a..2ac8d51d3668 100644
> --- a/http.c
> +++ b/http.c
> @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
>  	struct timeval select_timeout;
>  	int finished = 0;
>
> +#if __GNUC__ >= 12
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wdangling-pointer"
> +#endif
>  	slot->finished = &finished;
> +#if __GNUC__ >= 12
> +#pragma GCC diagnostic pop
> +#endif
>  	while (!finished) {
>  		step_active_slots();
> -- snap --
>
> That's quite ugly, though. And what's worse, it is pretty unreadable, too.

Unfortunately that sort of thing is a logic error as clang, ICC and
probably others are on a mission to make __GNUC__ as useless as
possible:
https://stackoverflow.com/questions/38499462/how-to-tell-clang-to-stop-pretending-to-be-other-compilers

I think it *might* work in practice though, my local clang claims to be
gcc 4, so maybe everyone faking it stops at a low enough version?

But did you spot 9c539d1027d (config.mak.dev: alternative workaround to
gcc 12 warning in http.c, 2022-04-15)? We already disable this file-wide
in config.mak.dev, but I didn't check if the Windows code was using that
(or if you were targeting those without DEVELOPER=1).

We could move that to thake main Makefile, but then we'd have to call
detect-compiler there. I have some local patches to do something like
that if there's interest (rather, to bootstrap compilation by compiling
a C object and getting the macro values, instead of relying on that
shellscript).
Junio C Hamano May 24, 2022, 10:38 p.m. UTC | #5
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Tue, 24 May 2022, Junio C Hamano wrote:
>
>> The "clear slot->finished", by the way, is what I think is the right
>> thing to do, especially that the objective is to squelch the false
>> positive warning from a new compiler.  If there is a way to annotate
>> the line for the compiler to tell it not to warn about it, that would
>> have been even better.
>
> We could do something like this:

Yuck.

> -- snip --
> diff --git a/http.c b/http.c
> index b08795715f8a..2ac8d51d3668 100644
> --- a/http.c
> +++ b/http.c
> @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
>  	struct timeval select_timeout;
>  	int finished = 0;
>
> +#if __GNUC__ >= 12
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wdangling-pointer"
> +#endif
>  	slot->finished = &finished;
> +#if __GNUC__ >= 12
> +#pragma GCC diagnostic pop
> +#endif
>  	while (!finished) {
>  		step_active_slots();
> -- snap --
>
> That's quite ugly, though. And what's worse, it is pretty unreadable, too.

Yes, very ugly.  Would an unconditional

	slot->finished = NULL;

at the end squelch the warning?

Or there is a way to say "we make all warnings into errors with
-Werror, but we do not want to turn this dangling-pointer warning to
an error, because it has false positives"?

Or we could add "-Wno-dangling-pointer" globally, perhaps.
Johannes Schindelin May 25, 2022, 10:16 a.m. UTC | #6
Hi Junio,

On Tue, 24 May 2022, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Tue, 24 May 2022, Junio C Hamano wrote:
> >
> >> The "clear slot->finished", by the way, is what I think is the right
> >> thing to do, especially that the objective is to squelch the false
> >> positive warning from a new compiler.  If there is a way to annotate
> >> the line for the compiler to tell it not to warn about it, that would
> >> have been even better.
> >
> > We could do something like this:
>
> Yuck.
>
> > -- snip --
> > diff --git a/http.c b/http.c
> > index b08795715f8a..2ac8d51d3668 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
> >  	struct timeval select_timeout;
> >  	int finished = 0;
> >
> > +#if __GNUC__ >= 12
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wdangling-pointer"
> > +#endif
> >  	slot->finished = &finished;
> > +#if __GNUC__ >= 12
> > +#pragma GCC diagnostic pop
> > +#endif
> >  	while (!finished) {
> >  		step_active_slots();
> > -- snap --
> >
> > That's quite ugly, though. And what's worse, it is pretty unreadable, too.
>
> Yes, very ugly.  Would an unconditional
>
> 	slot->finished = NULL;
>
> at the end squelch the warning?

No, unfortunately not:
https://github.com/dscho/git/actions/runs/2383492484

As you mentioned elsewhere, the error is clearly about the assignment in
the first place, and it does not matter that the function will rectify the
situation. It's the correct thing to do for the compiler, too: since
`slot->finished` already has the pointer, and since the
`active_request_slot` struct is declared globally, code outside the
current file might squirrel that pointer away for later use.

> Or there is a way to say "we make all warnings into errors with
> -Werror, but we do not want to turn this dangling-pointer warning to
> an error, because it has false positives"?
>
> Or we could add "-Wno-dangling-pointer" globally, perhaps.

I'd like to avoid that because it would quite likely hide legitimate
issues elsewhere.

It currently seems to be the easiest solution to simply turn the local
variable into a heap variable:

-- snip --
diff --git a/http.c b/http.c
index f92859f43fa..0712debd558 100644
--- a/http.c
+++ b/http.c
@@ -1327,10 +1327,10 @@ void run_active_slot(struct active_request_slot *slot)
 	fd_set excfds;
 	int max_fd;
 	struct timeval select_timeout;
-	int finished = 0;
+	int *finished = xcalloc(1, sizeof(int));

-	slot->finished = &finished;
-	while (!finished) {
+	slot->finished = finished;
+	while (!*finished) {
 		step_active_slots();

 		if (slot->in_use) {
@@ -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;
+	free(finished);
 }

 static void release_active_slot(struct active_request_slot *slot)
-- snap --

This pacifies GCC (https://github.com/dscho/git/runs/6589617700) and is
the most minimally-intrusive work-around of which I am currently aware.

Ciao,
Dscho
Ævar Arnfjörð Bjarmason May 25, 2022, 12:48 p.m. UTC | #7
On Wed, May 25 2022, Johannes Schindelin wrote:

> Hi Junio,
>
> On Tue, 24 May 2022, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > On Tue, 24 May 2022, Junio C Hamano wrote:
>> >
>> >> The "clear slot->finished", by the way, is what I think is the right
>> >> thing to do, especially that the objective is to squelch the false
>> >> positive warning from a new compiler.  If there is a way to annotate
>> >> the line for the compiler to tell it not to warn about it, that would
>> >> have been even better.
>> >
>> > We could do something like this:
>>
>> Yuck.
>>
>> > -- snip --
>> > diff --git a/http.c b/http.c
>> > index b08795715f8a..2ac8d51d3668 100644
>> > --- a/http.c
>> > +++ b/http.c
>> > @@ -1365,7 +1365,14 @@ void run_active_slot(struct active_request_slot *slot)
>> >  	struct timeval select_timeout;
>> >  	int finished = 0;
>> >
>> > +#if __GNUC__ >= 12
>> > +#pragma GCC diagnostic push
>> > +#pragma GCC diagnostic ignored "-Wdangling-pointer"
>> > +#endif
>> >  	slot->finished = &finished;
>> > +#if __GNUC__ >= 12
>> > +#pragma GCC diagnostic pop
>> > +#endif
>> >  	while (!finished) {
>> >  		step_active_slots();
>> > -- snap --
>> >
>> > That's quite ugly, though. And what's worse, it is pretty unreadable, too.
>>
>> Yes, very ugly.  Would an unconditional
>>
>> 	slot->finished = NULL;
>>
>> at the end squelch the warning?
>
> No, unfortunately not:
> https://github.com/dscho/git/actions/runs/2383492484 

Yes it does. Didn't you just have a PBCAK between writing that test &
pushing it? Your
https://github.com/dscho/git/blob/tmp/http.c#L1370-L1371 shows that you
didn't make that edit.

This on top of "seen" makes the warning go away:
	
	-       if (slot->finished == &finished)
	-               slot->finished = NULL;
	+       slot->finished = NULL;

This is also all covered in the initial thread from back in January,
which if you're slowly re-discovering the learnings from there I
encourage you to read in more detail... :)

> As you mentioned elsewhere, the error is clearly about the assignment in
> the first place, and it does not matter that the function will rectify the
> situation. It's the correct thing to do for the compiler, too: since
> `slot->finished` already has the pointer, and since the
> `active_request_slot` struct is declared globally, code outside the
> current file might squirrel that pointer away for later use.

Per the above this isn't true, and in some side-thread replies (and in
the initial thread) I've linked to the GCC code in question. NULL-ing
the slot is sufficient, it doesn't matter that the struct it's in
survives the function, just that the pointer isn't exposed.

>> Or there is a way to say "we make all warnings into errors with
>> -Werror, but we do not want to turn this dangling-pointer warning to
>> an error, because it has false positives"?
>>
>> Or we could add "-Wno-dangling-pointer" globally, perhaps.
>
> I'd like to avoid that because it would quite likely hide legitimate
> issues elsewhere.
>
> It currently seems to be the easiest solution to simply turn the local
> variable into a heap variable:
>
> -- snip --
> diff --git a/http.c b/http.c
> index f92859f43fa..0712debd558 100644
> --- a/http.c
> +++ b/http.c
> @@ -1327,10 +1327,10 @@ void run_active_slot(struct active_request_slot *slot)
>  	fd_set excfds;
>  	int max_fd;
>  	struct timeval select_timeout;
> -	int finished = 0;
> +	int *finished = xcalloc(1, sizeof(int));
>
> -	slot->finished = &finished;
> -	while (!finished) {
> +	slot->finished = finished;
> +	while (!*finished) {
>  		step_active_slots();
>
>  		if (slot->in_use) {
> @@ -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;
> +	free(finished);
>  }

Also, if we were going to tweak this extensively I'd think this slightly
larger POC patch would be a better direction, i.e. we don't need a
pointer at all, we're just (ab)using it for a tri-state NULL/0/1, why
not just use an enum?

I think the "if it's what we just set it to" is just cargo-culting, is
there anything to show that run_active_slot() is reentrant? Wouldn't we
be better off with a static variable + BUG() that we increment/decremant
and panic if it's anything but 0 and 1 if we'd like to add paranoia
around that?

diff --git a/http-walker.c b/http-walker.c
index b8f0f98ae14..26184a82ddb 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -225,13 +225,13 @@ static void process_alternates_response(void *callback_data)
 					 alt_req->url->buf);
 			active_requests++;
 			slot->in_use = 1;
-			if (slot->finished)
-				(*slot->finished) = 0;
+			if (slot->f != FIN_UNSET)
+                            slot->f = FIN_NO;
 			if (!start_active_slot(slot)) {
 				cdata->got_alternates = -1;
 				slot->in_use = 0;
-				if (slot->finished)
-					(*slot->finished) = 1;
+                                if (slot->f != FIN_UNSET)
+                                    slot->f = FIN_YES;
 			}
 			return;
 		}
diff --git a/http.c b/http.c
index b148468b267..845dd40768c 100644
--- a/http.c
+++ b/http.c
@@ -197,8 +197,8 @@ 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)
-		(*slot->finished) = 1;
+	if (slot->f != FIN_UNSET)
+		slot->f = FIN_YES;
 
 	/* Store slot results so they can be read after the slot is reused */
 	if (slot->results) {
@@ -1204,7 +1204,7 @@ struct active_request_slot *get_active_slot(void)
 	active_requests++;
 	slot->in_use = 1;
 	slot->results = NULL;
-	slot->finished = NULL;
+	slot->f = FIN_UNSET;
 	slot->callback_data = NULL;
 	slot->callback_func = NULL;
 	curl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);
@@ -1327,10 +1327,9 @@ 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) {
+	slot->f = FIN_NO;
+	while (slot->f == FIN_NO) {
 		step_active_slots();
 
 		if (slot->in_use) {
diff --git a/http.h b/http.h
index df1590e53a4..fc664d90bc9 100644
--- a/http.h
+++ b/http.h
@@ -19,12 +19,13 @@ struct slot_results {
 	long http_connectcode;
 };
 
+enum fin { FIN_UNSET, FIN_NO, FIN_YES };
 struct active_request_slot {
 	CURL *curl;
 	int in_use;
 	CURLcode curl_result;
 	long http_code;
-	int *finished;
+	enum fin f;
 	struct slot_results *results;
 	void *callback_data;
 	void (*callback_func)(void *data);
diff mbox series

Patch

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 f92859f43fa..00206676597 100644
--- a/http.c
+++ b/http.c
@@ -197,8 +197,7 @@  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;
+	slot->in_use = 0;
 
 	/* Store slot results so they can be read after the slot is reused */
 	if (slot->results != NULL) {
@@ -1176,13 +1175,14 @@  struct active_request_slot *get_active_slot(void)
 			process_curl_messages();
 	}
 
-	while (slot != NULL && slot->in_use)
+	while (slot != NULL && (slot->in_use || slot->reserved_for_use))
 		slot = slot->next;
 
 	if (slot == NULL) {
 		newslot = xmalloc(sizeof(*newslot));
 		newslot->curl = NULL;
 		newslot->in_use = 0;
+		newslot->reserved_for_use = 0;
 		newslot->next = NULL;
 
 		slot = active_queue_head;
@@ -1204,7 +1204,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);
@@ -1296,7 +1295,7 @@  void fill_active_slots(void)
 	}
 
 	while (slot != NULL) {
-		if (!slot->in_use && slot->curl != NULL
+		if (!slot->in_use && !slot->reserved_for_use && slot->curl
 			&& curl_session_count > min_curl_sessions) {
 			curl_easy_cleanup(slot->curl);
 			slot->curl = NULL;
@@ -1327,10 +1326,9 @@  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) {
+	slot->reserved_for_use = 1;
+	while (slot->in_use) {
 		step_active_slots();
 
 		if (slot->in_use) {
@@ -1367,6 +1365,7 @@  void run_active_slot(struct active_request_slot *slot)
 			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
 		}
 	}
+	slot->reserved_for_use = 0;
 }
 
 static void release_active_slot(struct active_request_slot *slot)
diff --git a/http.h b/http.h
index df1590e53a4..3b2f6da570c 100644
--- a/http.h
+++ b/http.h
@@ -22,9 +22,9 @@  struct slot_results {
 struct active_request_slot {
 	CURL *curl;
 	int in_use;
+	int reserved_for_use;
 	CURLcode curl_result;
 	long http_code;
-	int *finished;
 	struct slot_results *results;
 	void *callback_data;
 	void (*callback_func)(void *data);