coreutils/gnulib - fts.c dangling pointers & gcc 13.1

coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Peter Frazier]

dangling pointers & gcc 13.1

problem with:
coreutils/gnulib, fts.c

compilation failure:
-Werror=use-after-free error

approach to resolution (thus far):
post free of vars, I set the vars to NULL, but it did not help

Best,

1pf1

Re: coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Paul Eggert]

On 2023-02-03 12:24, Peter Frazier wrote:
> dangling pointers & gcc 13.1
> 
> problem with:
> coreutils/gnulib, fts.c
> 
> compilation failure:
> -Werror=use-after-free error
> 
> approach to resolution (thus far):
> post free of vars, I set the vars to NULL, but it did not help

Could you be more specific about the symptoms you observed, and how to 
reproduce them? For one thing, GCC 13.1 has not been released yet, as 
far as I can see.

Re: coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]



> On 3 Feb 2023, at 22:11, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
> On 2023-02-03 12:24, Peter Frazier wrote:
>> dangling pointers & gcc 13.1
>> problem with:
>> coreutils/gnulib, fts.c
>> compilation failure:
>> -Werror=use-after-free error
>> approach to resolution (thus far):
>> post free of vars, I set the vars to NULL, but it did not help
> 
> Could you be more specific about the symptoms you observed, and how to reproduce them? For one thing, GCC 13.1 has not been released yet, as far as I can see.
> 

Yes, we need the full build log, and the commands run to obtain them. GCC 13 has not yet been
released so we need the 'gcc -v' output too.

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 304 bytes --]

I manually inspected fts.c to look for violations of the C standard that 
might draw GCC's attention, and installed the attached patches into 
Gnulib. As you can see, they don't fix the technical violations of the C 
standard. However, I hope they keep GCC happy. Please give them a try 
with "GCC 13.1".

[-- Attachment #2: 0001-fts-pacify-GCC-13-Wuse-after-free.patch --]
[-- Type: text/x-patch, Size: 4715 bytes --]

From 6d488119c68989038faa05c9ee9d43c8c82487e4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 4 Feb 2023 10:07:11 -0800
Subject: [PATCH 1/2] fts: pacify GCC 13 -Wuse-after-free

Problem reported by Peter Frazier in:
https://lists.gnu.org/r/bug-gnulib/2023-02/msg00000.html
* lib/fts.c: Include stdint.h.
(fts_build): Do not access freed pointer directly; instead,
save its bit-pattern into a uintptr_t, and use that to compare.
(ADJUST): Likewise, but more trickily since this hack
puns pointer types and relies on undefined behavior.
* modules/fts (Depends-on): Add stdint.
---
 ChangeLog   | 18 ++++++++++++++++++
 lib/fts.c   | 15 ++++++++++-----
 modules/fts |  1 +
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0f8b53ff13..ed999a6d50 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,21 @@
+2023-02-04  Paul Eggert  <eggert@cs.ucla.edu>
+
+	fts: pacify GCC 13 -Wuse-after-free
+	Ordinarily I fix this sort of thing by using well-defined rather
+	than undefined behavior, but a straightforward patch along those
+	lines would change the fts_.h API since fts_accpath would change
+	from a pointer to an integer with a more-complex interpretation.
+	Instead, attempt to pacify GCC 13 with code that relies on
+	undefined but portable-in-practice behavior that GCC 13 does not
+	complain about.  GCC problem reported by Peter Frazier in:
+	https://lists.gnu.org/r/bug-gnulib/2023-02/msg00000.html
+	* lib/fts.c: Include stdint.h.
+	(fts_build): Do not access freed pointer directly; instead,
+	save its bit-pattern into a uintptr_t, and use that to compare.
+	(ADJUST): Likewise, but more trickily since this hack
+	puns pointer types and relies on undefined behavior.
+	* modules/fts (Depends-on): Add stdint.
+
 2023-02-04  Bruno Haible  <bruno@clisp.org>
 
 	assert-h, verify: Fix conflict with standard C++ header files on macOS.
diff --git a/lib/fts.c b/lib/fts.c
index 65a96e8add..3e5bb47aaf 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -63,6 +63,7 @@ static char sccsid[] = "@(#)fts.c       8.6 (Berkeley) 8/14/94";
 #include <fcntl.h>
 #include <errno.h>
 #include <stddef.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -1268,7 +1269,6 @@ fts_build (register FTS *sp, int type)
         register FTSENT *p, *head;
         register size_t nitems;
         FTSENT *tail;
-        void *oldaddr;
         int saved_errno;
         bool descend;
         bool doadjust;
@@ -1461,7 +1461,7 @@ fts_build (register FTS *sp, int type)
                         goto mem1;
                 if (d_namelen >= maxlen) {
                         /* include space for NUL */
-                        oldaddr = sp->fts_path;
+                        uintptr_t oldaddr = (uintptr_t) sp->fts_path;
                         if (! fts_palloc(sp, d_namelen + len + 1)) {
                                 /*
                                  * No more memory.  Save
@@ -1478,7 +1478,7 @@ mem1:                           saved_errno = errno;
                                 return (NULL);
                         }
                         /* Did realloc() change the pointer? */
-                        if (oldaddr != sp->fts_path) {
+                        if (oldaddr != (uintptr_t) sp->fts_path) {
                                 doadjust = true;
                                 if (ISSET(FTS_NOCHDIR))
                                         cp = sp->fts_path + len;
@@ -1988,10 +1988,15 @@ fts_padjust (FTS *sp, FTSENT *head)
         FTSENT *p;
         char *addr = sp->fts_path;
 
+        /* This code looks at bit-patterns of freed pointers to
+           relocate them, so it relies on undefined behavior.  If this
+           trick does not work on your platform, please report a bug.  */
+
 #define ADJUST(p) do {                                                  \
-        if ((p)->fts_accpath != (p)->fts_name) {                        \
+        uintptr_t old_accpath = *(uintptr_t *) &(p)->fts_accpath;       \
+        if (old_accpath != (uintptr_t) (p)->fts_name) {                 \
                 (p)->fts_accpath =                                      \
-                    (char *)addr + ((p)->fts_accpath - (p)->fts_path);  \
+                  addr + (old_accpath - *(uintptr_t *) &(p)->fts_path); \
         }                                                               \
         (p)->fts_path = addr;                                           \
 } while (0)
diff --git a/modules/fts b/modules/fts
index 18b10fac6e..fe56bae6e0 100644
--- a/modules/fts
+++ b/modules/fts
@@ -31,6 +31,7 @@ opendirat
 readdir
 stdbool
 stddef
+stdint
 
 configure.ac:
 gl_FUNC_FTS
-- 
2.37.2


[-- Attachment #3: 0002-fts-pacify-GCC-12-Wstrict-aliasing.patch --]
[-- Type: text/x-patch, Size: 2001 bytes --]

From 32c16c45d7378b014d9aac6130104c4d02a9acdb Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 4 Feb 2023 10:28:55 -0800
Subject: [PATCH 2/2] fts: pacify GCC 12 -Wstrict-aliasing

* lib/fts.c (ADJUST): Avoid -Wstrict-aliasing waring.
---
 ChangeLog | 5 ++++-
 lib/fts.c | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ed999a6d50..ab24baf6f9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -13,7 +13,10 @@
 	(fts_build): Do not access freed pointer directly; instead,
 	save its bit-pattern into a uintptr_t, and use that to compare.
 	(ADJUST): Likewise, but more trickily since this hack
-	puns pointer types and relies on undefined behavior.
+	actually accesses freed pointers, but does so in a way that
+	I hope GCC doesn’t notice.  Although using ‘*(uintptr_t *) &P’
+	instead of ‘(uintptr_t) P’ would avoid accessing freed pointers,
+	it would provoke a -Wstrict-aliasing diagnostic.
 	* modules/fts (Depends-on): Add stdint.
 
 2023-02-04  Bruno Haible  <bruno@clisp.org>
diff --git a/lib/fts.c b/lib/fts.c
index 3e5bb47aaf..78584b2902 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -1993,10 +1993,10 @@ fts_padjust (FTS *sp, FTSENT *head)
            trick does not work on your platform, please report a bug.  */
 
 #define ADJUST(p) do {                                                  \
-        uintptr_t old_accpath = *(uintptr_t *) &(p)->fts_accpath;       \
+        uintptr_t old_accpath = (uintptr_t) (p)->fts_accpath;           \
         if (old_accpath != (uintptr_t) (p)->fts_name) {                 \
                 (p)->fts_accpath =                                      \
-                  addr + (old_accpath - *(uintptr_t *) &(p)->fts_path); \
+                  addr + (old_accpath - (uintptr_t) (p)->fts_path);     \
         }                                                               \
         (p)->fts_path = addr;                                           \
 } while (0)
-- 
2.37.2

Re: coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]



> On 4 Feb 2023, at 18:46, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
> I manually inspected fts.c to look for violations of the C standard that might draw GCC's attention, and installed the attached patches into Gnulib. As you can see, they don't fix the technical violations of the C standard. However, I hope they keep GCC happy. Please give them a try with "GCC 13.1".<0001-fts-pacify-GCC-13-Wuse-after-free.patch><0002-fts-pacify-GCC-12-Wstrict-aliasing.patch>

For what it's worth, with GCC 13.0.1 20230129, I get no warnings with --enable-gcc-warnings,
and the test suite passes. Ditto =expensive.

But I don't get any with the previous commits either :)

As for the patches, I'd consider using #pragma GCC ... to suppress -Wuse-after-free
for the "problematic" lines instead. It'd avoid the risk of either optimisations or sanitisers
respectively causing us pain in future.


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Paul Eggert]

On 2023-02-04 11:53, Sam James wrote:
> I'd consider using #pragma GCC ... to suppress -Wuse-after-free
> for the "problematic" lines instead. It'd avoid the risk of either optimisations or sanitisers
> respectively causing us pain in future.

I don't see why that pragma would avoid those problems. All it would do 
is shut off the warnings; GCC's underlying analyses would be the same, 
and GCC would generate the same machine code. In that sense these 
warnings are useful - they're canaries in the coal mine.

Re: coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]



> On 4 Feb 2023, at 20:20, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
> On 2023-02-04 11:53, Sam James wrote:
>> I'd consider using #pragma GCC ... to suppress -Wuse-after-free
>> for the "problematic" lines instead. It'd avoid the risk of either optimisations or sanitisers
>> respectively causing us pain in future.
> 
> I don't see why that pragma would avoid those problems. All it would do is shut off the warnings; GCC's underlying analyses would be the same, and GCC would generate the same machine code. In that sense these warnings are useful - they're canaries in the coal mine.

I guess it's hard for me to say given I don't know what options allowed it to be reproduced and I couldn't hit it.

I assumed it must have been -Wstrict-aliasing=2 or lower which makes it more aggressive at the risk of false positives.

But if you reproduced it, then it's useful, I suppose.

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Paul Eggert]

On 2023-02-04 12:23, Sam James wrote:
> I guess it's hard for me to say given I don't know what options allowed it to be reproduced and I couldn't hit it.
> 
> I assumed it must have been -Wstrict-aliasing=2 or lower which makes it more aggressive at the risk of false positives.
> 
> But if you reproduced it, then it's useful, I suppose.

I didn't reproduce the warning, since I lack GCC "13.1". I merely looked 
at the Gnulib source and noticed a couple of places where it was not 
conforming to the C standard.  I have enough knowledge of GCC internals 
that I think I've changed the code so that it will pacify GCC "13.1".

Although the updated code still doesn't conform, it should work fine on 
real platforms. (The old code probably works too, for what it's worth.)

Re: coreutils/gnulib - fts.c dangling pointers & gcc 13.1

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]



> On 4 Feb 2023, at 22:03, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
> On 2023-02-04 12:23, Sam James wrote:
>> I guess it's hard for me to say given I don't know what options allowed it to be reproduced and I couldn't hit it.
>> I assumed it must have been -Wstrict-aliasing=2 or lower which makes it more aggressive at the risk of false positives.
>> But if you reproduced it, then it's useful, I suppose.
> 
> I didn't reproduce the warning, since I lack GCC "13.1". I merely looked at the Gnulib source and noticed a couple of places where it was not conforming to the C standard.  I have enough knowledge of GCC internals that I think I've changed the code so that it will pacify GCC "13.1".
> 
> Although the updated code still doesn't conform, it should work fine on real platforms. (The old code probably works too, for what it's worth.)

Alright, thanks for explaining - I follow now, cheers.

Maybe we'll get more information from the original reporter too.

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]