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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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