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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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