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

Re: [PATCH v2] http API: fix dangling pointer issue noted by GCC 12.0

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 25, 2022, 22:58 UTC
Message-ID
<xmqqsfs68sf5.fsf@gitster.g>
In-Reply-To
<patch-v2-1.1-777838267a5-20220225T090816Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 30 quoted lines
> 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).
>
> The GCC warning is specifically about pointers that survive the exit
> of the function. See a comment added to
> "pass_waccess::use_after_inval_p" in the GCC commit that added the
> warning, or:
>
>     /* The use is one of a dangling pointer if a clobber of the variable
>       [the pointer points to] has not been found before the function exit
>       point.  */
>     [...]
>
> There's a few possible ways to fix this, but the simplest is to assign
> NULL to "slot->finished" at the end of run_active_slot(), it's the
> only caller that ever assigns non-NULL to it. It was suggested[2] to
> guard that with "if (slot->finished == &finished)", but that'll still
> trigger the warning.

Does that mean we can clobber the finished member of a slot that was in use, not the one we prepared, because we do not make sure we do not clobber slot->finished that other people set up?

I think this change is safe, but it's been quite a while since I played with dumb HTTP walker the last time, so I no longer trust my own reading of this code X-<.

Show 10 quoted lines
> diff --git a/http.c b/http.c
> index 229da4d1488..2f67fbb33cd 100644
> --- a/http.c
> +++ b/http.c
> @@ -1367,6 +1367,7 @@ void run_active_slot(struct active_request_slot *slot)
>  			select(max_fd+1, &readfds, &writefds, &excfds, &select_timeout);
>  		}
>  	}
> +	slot->finished = NULL;
>  }

Will queue, but it would be nice if GCC can get fixed before we have to advance this to 'next' and below

Thanks. .

Previous: Ævar Arnfjörð BjarmasonNext: Taylor Blau
Message 8 of 15 in “http API: fix dangling pointer issue noted by GCC 12.0”
  1. http API: fix dangling pointer issue noted by GCC 12.0Ævar Arnfjörð Bjarmason, Jan 26, 2022
  2. Taylor BlauJan 26, 2022
  3. Junio C HamanoJan 27, 2022
  4. Junio C HamanoJan 27, 2022
  5. Ævar Arnfjörð BjarmasonJan 27, 2022
  6. Junio C HamanoJan 27, 2022
  7. http API: fix dangling pointer issue noted by GCC 12.0Ævar Arnfjörð Bjarmason, Feb 25, 2022
  8. Junio C HamanoFeb 25, 2022
  9. Taylor BlauFeb 26, 2022
  10. http API: fix dangling pointer issue noted by GCC 12.0Ævar Arnfjörð Bjarmason, Mar 25, 2022
  11. Taylor BlauMar 25, 2022
  12. Junio C HamanoMar 26, 2022
  13. Ævar Arnfjörð BjarmasonApr 14, 2022
  14. Junio C HamanoApr 14, 2022
  15. Ævar Arnfjörð BjarmasonApr 15, 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.