Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction

[Bruno Haible]

Jim Meyering wrote:
> > The question is: Is passing NULL to canonicalize_file_name valid? If not,
> > then the nonnull attribute should stay.
> >
> > On one hand, in glibc's stdlib.h we have:
> >
> > extern char *canonicalize_file_name (const char *__name)
> >      __THROW __nonnull ((1)) __wur;
> >
> > On the other hand, in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156#c3
> > you say "Note that at least some versions of canonicalize_file_name *can*
> > take a NULL argument". What are there versions? Even if Cygwin and/or
> > Solaris 11 have a function of the same name which allows passing NULL,
> > portable code should not pass NULL since that would not work on glibc
> > systems. Therefore the diagnostic is useful.
> 
> It is useful indeed.

OK, then let's preserve it.

> Thanks for working on that. However, it did not help, because at least
> on Fedora 30, we're using the system declaration

In this case, since the glibc headers (in particular <sys/cdefs.h>) do not
give us the means to influence what __nonnull expands to, we have no choice
than to disable the test when the function comes from the system.

We do try to check the system functions just like we check the gnulib functions;
this strategy has led us to find numerous libc bugs on various systems. But
here, there's no way. Since canonicalize_file_name and ptsname_r are glibc
inventions, not POSIX functions, there is no specification that tells what
should happen if someone passes a NULL pointer to them; therefore the
__nonnull declaration and the effect that it has (from GCC), namely execute
arbitrary code, cannot be formally disputed. They can be disputed from the
point of view of practical usefulness, though.

I've pushed this. I hope it fixes the problem.


2020-01-05  Bruno Haible  <address@hidden>

        tests: Avoid GCC over-optimization caused by _GL_ARG_NONNULL attributes.
        Reported by Jim Meyering in
        <https://lists.gnu.org/archive/html/bug-gnulib/2020-01/msg00040.html>.
        * lib/stdlib.in.h (GNULIB_defined_canonicalize_file_name): New macro.
        (GNULIB_defined_ptsname_r): New macro.
        * tests/test-canonicalize.c (_GL_ARG_NONNULL): Define to empty.
        (main): Disable the NULL argument test if canonicalize_file_name does
        not come from gnulib.
        * tests/test-canonicalize-lgpl.c (_GL_ARG_NONNULL): Define to empty.
        (main): Disable the NULL argument test if canonicalize_file_name does
        not come from gnulib.
        * tests/test-ptsname_r.c (_GL_ARG_NONNULL): Define to empty.
        (test_errors): Disable the NULL argument test if ptsname_r does not come
        from gnulib.

diff --git a/lib/stdlib.in.h b/lib/stdlib.in.h
index e088959..49bbf6f 100644
--- a/lib/stdlib.in.h
+++ b/lib/stdlib.in.h
@@ -201,6 +201,10 @@ _GL_FUNCDECL_SYS (canonicalize_file_name, char *, (const 
char *name)
 #  endif
 _GL_CXXALIAS_SYS (canonicalize_file_name, char *, (const char *name));
 # endif
+# ifndef GNULIB_defined_canonicalize_file_name
+#  define GNULIB_defined_canonicalize_file_name \
+     (!@HAVE_CANONICALIZE_FILE_NAME@ || @REPLACE_CANONICALIZE_FILE_NAME@)
+# endif
 _GL_CXXALIASWARN (canonicalize_file_name);
 #elif defined GNULIB_POSIXCHECK
 # undef canonicalize_file_name
@@ -516,6 +520,9 @@ _GL_FUNCDECL_SYS (ptsname_r, int, (int fd, char *buf, 
size_t len));
 #  endif
 _GL_CXXALIAS_SYS (ptsname_r, int, (int fd, char *buf, size_t len));
 # endif
+# ifndef GNULIB_defined_ptsname_r
+#  define GNULIB_defined_ptsname_r (!@HAVE_PTSNAME_R@ || @REPLACE_PTSNAME_R@)
+# endif
 _GL_CXXALIASWARN (ptsname_r);
 #elif defined GNULIB_POSIXCHECK
 # undef ptsname_r
diff --git a/tests/test-canonicalize-lgpl.c b/tests/test-canonicalize-lgpl.c
index b46ad87..fb49b20 100644
--- a/tests/test-canonicalize-lgpl.c
+++ b/tests/test-canonicalize-lgpl.c
@@ -1,4 +1,4 @@
-/* Test of execution of program termination handlers.
+/* Test of execution of file name canonicalization.
    Copyright (C) 2007-2020 Free Software Foundation, Inc.
 
    This program is free software: you can redistribute it and/or modify
@@ -16,6 +16,11 @@
 
 /* Written by Bruno Haible <address@hidden>, 2007.  */
 
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   may "optimize" the null_ptr function, when its result gets passed to a
+   function that has an argument declared as _GL_ARG_NONNULL.  */
+#define _GL_ARG_NONNULL(params)
+
 #include <config.h>
 
 #include <stdlib.h>
@@ -66,14 +71,22 @@ main (void)
     ASSERT (strstr (result, "/" BASE "/tra")
             == result + strlen (result) - strlen ("/" BASE "/tra"));
     free (result);
+
     errno = 0;
     result = canonicalize_file_name ("");
     ASSERT (result == NULL);
     ASSERT (errno == ENOENT);
+
+    /* This test works only if the canonicalize_file_name implementation
+       comes from gnulib.  If it comes from libc, we have no way to prevent
+       gcc from "optimizing" the null_ptr function in invalid ways.  See
+       <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156>.  */
+#if GNULIB_defined_canonicalize_file_name
     errno = 0;
     result = canonicalize_file_name (null_ptr ());
     ASSERT (result == NULL);
     ASSERT (errno == EINVAL);
+#endif
   }
 
   /* Check that a non-directory with trailing slash yields NULL.  */
diff --git a/tests/test-canonicalize.c b/tests/test-canonicalize.c
index e7138a6..81580c5 100644
--- a/tests/test-canonicalize.c
+++ b/tests/test-canonicalize.c
@@ -16,6 +16,11 @@
 
 /* Written by Bruno Haible <address@hidden>, 2007.  */
 
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   may "optimize" the null_ptr function, when its result gets passed to a
+   function that has an argument declared as _GL_ARG_NONNULL.  */
+#define _GL_ARG_NONNULL(params)
+
 #include <config.h>
 
 #include "canonicalize.h"
@@ -62,22 +67,34 @@ main (void)
             == result1 + strlen (result1) - strlen ("/" BASE "/tra"));
     free (result1);
     free (result2);
+
     errno = 0;
     result1 = canonicalize_file_name ("");
     ASSERT (result1 == NULL);
     ASSERT (errno == ENOENT);
+
     errno = 0;
     result2 = canonicalize_filename_mode ("", CAN_EXISTING);
     ASSERT (result2 == NULL);
     ASSERT (errno == ENOENT);
+
+    /* This test works only if the canonicalize_file_name implementation
+       comes from gnulib.  If it comes from libc, we have no way to prevent
+       gcc from "optimizing" the null_ptr function in invalid ways.  See
+       <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156>.  */
+#if GNULIB_defined_canonicalize_file_name
     errno = 0;
     result1 = canonicalize_file_name (null_ptr ());
     ASSERT (result1 == NULL);
     ASSERT (errno == EINVAL);
+#endif
+
     errno = 0;
     result2 = canonicalize_filename_mode (NULL, CAN_EXISTING);
     ASSERT (result2 == NULL);
     ASSERT (errno == EINVAL);
+
+    errno = 0;
     result2 = canonicalize_filename_mode (".", CAN_MISSING | CAN_ALL_BUT_LAST);
     ASSERT (result2 == NULL);
     ASSERT (errno == EINVAL);
diff --git a/tests/test-ptsname_r.c b/tests/test-ptsname_r.c
index 19e33ea..24be52f 100644
--- a/tests/test-ptsname_r.c
+++ b/tests/test-ptsname_r.c
@@ -14,6 +14,11 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <https://www.gnu.org/licenses/>.  */
 
+/* Don't use __attribute__ __nonnull__ in this compilation unit.  Otherwise gcc
+   may "optimize" the null_ptr function, when its result gets passed to a
+   function that has an argument declared as _GL_ARG_NONNULL.  */
+#define _GL_ARG_NONNULL(params)
+
 #include <config.h>
 
 #include <stdlib.h>
@@ -84,9 +89,15 @@ test_errors (int fd, const char *slave)
         }
     }
 
+  /* This test works only if the ptsname_r implementation comes from gnulib.
+     If it comes from libc, we have no way to prevent gcc from "optimizing"
+     the null_ptr function in invalid ways.  See
+     <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156>.  */
+#if GNULIB_defined_ptsname_r
   result = ptsname_r (fd, null_ptr (), 0);
   ASSERT (result != 0);
   ASSERT (result == EINVAL);
+#endif
 }
 
 int