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

[Paul Eggert <eggert@cs.ucla.edu>]

[-- 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