git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
May 25, 2022, 12:48 UTC
Message-ID
<220525.86k0a9u5rw.gmgdl@evledraar.gmail.com>
In-Reply-To
<nycvar.QRO.7.76.6.2205251208560.352@tvgsbejvaqbjf.bet>
On Wed, May 25 2022, Johannes Schindelin wrote:
Show 49 quoted lines
> 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... :)

Show 6 quoted lines
> 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.

Show 39 quoted lines
>> 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);
Previous: Johannes SchindelinNext: Johannes Schindelin via GitGitGadget
Message 17 of 23 in “ci: fix windows-build with GCC v12.x”
  1. 0/4 ci: fix windows-build with GCC v12.xJohannes Schindelin via GitGitGadget, May 24, 2022
  2. 1/4 compat/win32/syslog: fix use-after-reallocJohannes Schindelin via GitGitGadget, May 24, 2022
  3. Johannes SchindelinMay 24, 2022
  4. Junio C HamanoMay 24, 2022
  5. 2/4 nedmalloc: avoid new compile errorJohannes Schindelin via GitGitGadget, May 24, 2022
  6. Ævar Arnfjörð BjarmasonMay 24, 2022
  7. René ScharfeMay 24, 2022
  8. Johannes SchindelinMay 24, 2022
  9. Ævar Arnfjörð BjarmasonMay 24, 2022
  10. 3/4 http.c: avoid danging pointer to local variable `finished`Johannes Schindelin via GitGitGadget, May 24, 2022
  11. Ævar Arnfjörð BjarmasonMay 24, 2022
  12. Junio C HamanoMay 24, 2022
  13. Johannes SchindelinMay 24, 2022
  14. Ævar Arnfjörð BjarmasonMay 24, 2022
  15. Junio C HamanoMay 24, 2022
  16. Johannes SchindelinMay 25, 2022
  17. Ævar Arnfjörð BjarmasonMay 25, 2022
  18. 4/4 dir.c: avoid "exceeds maximum object size" error with GCC v12.xJohannes Schindelin via GitGitGadget, May 24, 2022
  19. Ævar Arnfjörð BjarmasonMay 24, 2022
  20. Johannes SchindelinMay 24, 2022
  21. Derrick StoleeMay 25, 2022
  22. Junio C HamanoMay 25, 2022
  23. Jeff HostetlerMay 24, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.