bug-gnulib@gnu.org mirror (unofficial)
 help / color / mirror / Atom feed
* [PATCH] stdlib: avoid canonicalize_file_name contradiction
@ 2020-01-05  6:24 Jim Meyering
  2020-01-05  8:15 ` Bruno Haible
  0 siblings, 1 reply; 7+ messages in thread
From: Jim Meyering @ 2020-01-05  6:24 UTC (permalink / raw)
  To: bug-gnulib@gnu.org List

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

I expect to push something like the attached tomorrow:

stdlib: avoid canonicalize_file_name contradiction
* lib/stdlib.in.h (canonicalize_file_name): Remove the nonnull
attribute from its declaration. tests/test-canonicalize-lgpl.c
passes null_ptr () to it, which (via this contradiction) would
provoke a segfault from GCC 10. See a small reproducer and
discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156

[-- Attachment #2: canonicalize_file_name-nonnull-vs-gcc10-segfault.diff --]
[-- Type: application/octet-stream, Size: 2276 bytes --]

From 0f7cfbf5c2b3403effc5910f57f420b58ed91330 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@fb.com>
Date: Sat, 4 Jan 2020 22:21:17 -0800
Subject: [PATCH] stdlib: avoid canonicalize_file_name contradiction

* lib/stdlib.in.h (canonicalize_file_name): Remove the nonnull
attribute from its declaration. tests/test-canonicalize-lgpl.c
passes null_ptr () to it, which (via this contradiction) would
provoke a segfault from GCC 10. See a small reproducer and
discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156
---
 ChangeLog       | 9 +++++++++
 lib/stdlib.in.h | 6 ++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 6d9ea9e52..730f53727 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-01-04  Jim Meyering  <meyering@fb.com>
+
+	stdlib: avoid canonicalize_file_name contradiction
+	* lib/stdlib.in.h (canonicalize_file_name): Remove the nonnull
+	attribute from its declaration. tests/test-canonicalize-lgpl.c
+	passes null_ptr () to it, which (via this contradiction) would
+	provoke a segfault from GCC 10. See a small reproducer and
+	discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156
+
 2020-01-04  Bruno Haible  <bruno@clisp.org>

 	mbsnrtoc32s: Add tests.
diff --git a/lib/stdlib.in.h b/lib/stdlib.in.h
index e088959b2..938ec37b1 100644
--- a/lib/stdlib.in.h
+++ b/lib/stdlib.in.h
@@ -191,13 +191,11 @@ _GL_WARN_ON_USE (calloc, "calloc is not POSIX compliant everywhere - "
 #  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
 #   define canonicalize_file_name rpl_canonicalize_file_name
 #  endif
-_GL_FUNCDECL_RPL (canonicalize_file_name, char *, (const char *name)
-                                                  _GL_ARG_NONNULL ((1)));
+_GL_FUNCDECL_RPL (canonicalize_file_name, char *, (const char *name));
 _GL_CXXALIAS_RPL (canonicalize_file_name, char *, (const char *name));
 # else
 #  if !@HAVE_CANONICALIZE_FILE_NAME@
-_GL_FUNCDECL_SYS (canonicalize_file_name, char *, (const char *name)
-                                                  _GL_ARG_NONNULL ((1)));
+_GL_FUNCDECL_SYS (canonicalize_file_name, char *, (const char *name));
 #  endif
 _GL_CXXALIAS_SYS (canonicalize_file_name, char *, (const char *name));
 # endif
-- 
2.24.0.390.g083378cc35


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction
  2020-01-05  6:24 [PATCH] stdlib: avoid canonicalize_file_name contradiction Jim Meyering
@ 2020-01-05  8:15 ` Bruno Haible
  2020-01-05 16:41   ` Jim Meyering
  0 siblings, 1 reply; 7+ messages in thread
From: Bruno Haible @ 2020-01-05  8:15 UTC (permalink / raw)
  To: bug-gnulib; +Cc: Jim Meyering

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

Hi Jim,

> stdlib: avoid canonicalize_file_name contradiction
> * lib/stdlib.in.h (canonicalize_file_name): Remove the nonnull
> attribute from its declaration.

I'm not sure this is right. The patch removes a diagnostic (from GCC and
possibly other static analyzers) when some code is, by mistake, passing
NULL to canonicalize_file_name.

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.

> tests/test-canonicalize-lgpl.c
> passes null_ptr () to it, which (via this contradiction) would
> provoke a segfault from GCC 10. See a small reproducer and
> discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156

I would prefer to modify the tests, to avoid GCC's over-optimization.
There are two ways to do that:

  * Use 'volatile', as shown by Andrew Pinski in
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156#c2
    I am not in favour of this, because the semantics of 'volatile'
    changes over time. Currently there is a proposed change in
    http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2244.htm#dr_476 .

  * The usual technique for this, that we already use in
      lib/canonicalize-lgpl.c
      lib/getaddrinfo.c
      lib/getdelim.c
      lib/glob.c
      lib/random_r.c
      lib/setenv.c
      lib/tsearch.c
      lib/unsetenv.c
    , is to add a line
      #define _GL_ARG_NONNULL(params)
    before '#include <config.h>'.

Does the attached patch fix the problem for you?

Bruno

[-- Attachment #2: 0001-tests-Avoid-GCC-over-optimization-caused-by-_GL_ARG_.patch --]
[-- Type: text/x-patch, Size: 3073 bytes --]

From 826a16e3da3d42454417e433774dda6e5003ad04 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Sun, 5 Jan 2020 09:13:25 +0100
Subject: [PATCH] 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>.

* tests/test-canonicalize.c (_GL_ARG_NONNULL): Define to empty.
* tests/test-canonicalize-lgpl.c (_GL_ARG_NONNULL): Likewise.
* tests/test-ptsname_r.c (_GL_ARG_NONNULL): Likewise.
---
 ChangeLog                      | 9 +++++++++
 tests/test-canonicalize-lgpl.c | 5 +++++
 tests/test-canonicalize.c      | 5 +++++
 tests/test-ptsname_r.c         | 5 +++++
 4 files changed, 24 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 6d9ea9e..556549c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2020-01-05  Bruno Haible  <bruno@clisp.org>
+
+	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>.
+	* tests/test-canonicalize.c (_GL_ARG_NONNULL): Define to empty.
+	* tests/test-canonicalize-lgpl.c (_GL_ARG_NONNULL): Likewise.
+	* tests/test-ptsname_r.c (_GL_ARG_NONNULL): Likewise.
+
 2020-01-04  Bruno Haible  <bruno@clisp.org>
 
 	mbsnrtoc32s: Add tests.
diff --git a/tests/test-canonicalize-lgpl.c b/tests/test-canonicalize-lgpl.c
index b46ad87..2c743af 100644
--- a/tests/test-canonicalize-lgpl.c
+++ b/tests/test-canonicalize-lgpl.c
@@ -16,6 +16,11 @@
 
 /* Written by Bruno Haible <bruno@clisp.org>, 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>
diff --git a/tests/test-canonicalize.c b/tests/test-canonicalize.c
index e7138a6..1df3092 100644
--- a/tests/test-canonicalize.c
+++ b/tests/test-canonicalize.c
@@ -16,6 +16,11 @@
 
 /* Written by Bruno Haible <bruno@clisp.org>, 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"
diff --git a/tests/test-ptsname_r.c b/tests/test-ptsname_r.c
index 19e33ea..9e1471c 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>
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction
  2020-01-05  8:15 ` Bruno Haible
@ 2020-01-05 16:41   ` Jim Meyering
  2020-01-05 16:46     ` Jim Meyering
  2020-01-05 20:46     ` Bruno Haible
  0 siblings, 2 replies; 7+ messages in thread
From: Jim Meyering @ 2020-01-05 16:41 UTC (permalink / raw)
  To: Bruno Haible; +Cc: bug-gnulib@gnu.org List

On Sun, Jan 5, 2020 at 12:16 AM Bruno Haible <bruno@clisp.org> wrote:
> Hi Jim,

Hi Bruno,
Thanks for investigating.

> > stdlib: avoid canonicalize_file_name contradiction
> > * lib/stdlib.in.h (canonicalize_file_name): Remove the nonnull
> > attribute from its declaration.
>
> I'm not sure this is right. The patch removes a diagnostic (from GCC and
> possibly other static analyzers) when some code is, by mistake, passing
> NULL to canonicalize_file_name.
>
> 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. I noticed this implementation can forward to
realpath, which does allow NULL as first argument, then did what must
have been an erroneous one-line search for attributes on a glibc
system.

> > tests/test-canonicalize-lgpl.c
> > passes null_ptr () to it, which (via this contradiction) would
> > provoke a segfault from GCC 10. See a small reproducer and
> > discussion in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156
>
> I would prefer to modify the tests, to avoid GCC's over-optimization.
> There are two ways to do that:
>
>   * Use 'volatile', as shown by Andrew Pinski in
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156#c2
>     I am not in favour of this, because the semantics of 'volatile'
>     changes over time. Currently there is a proposed change in
>     http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2244.htm#dr_476 .
>
>   * The usual technique for this, that we already use in
>       lib/canonicalize-lgpl.c
>       lib/getaddrinfo.c
>       lib/getdelim.c
>       lib/glob.c
>       lib/random_r.c
>       lib/setenv.c
>       lib/tsearch.c
>       lib/unsetenv.c
>     , is to add a line
>       #define _GL_ARG_NONNULL(params)
>     before '#include <config.h>'.
>
> Does the attached patch fix the problem for you?

Thanks for working on that. However, it did not help, because at least
on Fedora 30, we're using the system declaration, per this: (run from
a test dir prepared by "./gnulib-tool --test --dir /tmp/x --with-tests
canonicalize-lgpl", which still segfaults that test)

$ rm test-canonicalize-lgpl.o
$ make test-canonicalize-lgpl.o CFLAGS='-dD -E'
...
$ grep -A2 ze_file test-canonicalize-lgpl.o|head -3
extern char *canonicalize_file_name (const char *__name)
     __attribute__ ((__nothrow__ , __leaf__)) __attribute__
((__nonnull__ (1))) ;
# 797 "/usr/include/stdlib.h" 3 4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction
  2020-01-05 16:41   ` Jim Meyering
@ 2020-01-05 16:46     ` Jim Meyering
  2020-01-05 20:46     ` Bruno Haible
  1 sibling, 0 replies; 7+ messages in thread
From: Jim Meyering @ 2020-01-05 16:46 UTC (permalink / raw)
  To: Bruno Haible; +Cc: bug-gnulib@gnu.org List

On Sun, Jan 5, 2020 at 8:41 AM Jim Meyering <jim@meyering.net> wrote:
...
> > Does the attached patch fix the problem for you?
>
> Thanks for working on that. However, it did not help, because at least
> on Fedora 30, we're using the system declaration, per this: (run from
> a test dir prepared by "./gnulib-tool --test --dir /tmp/x --with-tests
> canonicalize-lgpl", which still segfaults that test)
>
> $ rm test-canonicalize-lgpl.o
> $ make test-canonicalize-lgpl.o CFLAGS='-dD -E'
> ...
> $ grep -A2 ze_file test-canonicalize-lgpl.o|head -3
> extern char *canonicalize_file_name (const char *__name)
>      __attribute__ ((__nothrow__ , __leaf__)) __attribute__
> ((__nonnull__ (1))) ;
> # 797 "/usr/include/stdlib.h" 3 4

This makes me suspect I did not test my own patch.
I've just confirmed that. My patch didn't help, either.
Sorry about that.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction
  2020-01-05 16:41   ` Jim Meyering
  2020-01-05 16:46     ` Jim Meyering
@ 2020-01-05 20:46     ` Bruno Haible
  2020-01-05 21:52       ` Jim Meyering
  1 sibling, 1 reply; 7+ messages in thread
From: Bruno Haible @ 2020-01-05 20:46 UTC (permalink / raw)
  To: Jim Meyering; +Cc: bug-gnulib

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  <bruno@clisp.org>

	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 <bruno@clisp.org>, 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 <bruno@clisp.org>, 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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction
  2020-01-05 20:46     ` Bruno Haible
@ 2020-01-05 21:52       ` Jim Meyering
  2020-01-05 22:57         ` Bruno Haible
  0 siblings, 1 reply; 7+ messages in thread
From: Jim Meyering @ 2020-01-05 21:52 UTC (permalink / raw)
  To: Bruno Haible; +Cc: bug-gnulib@gnu.org List

On Sun, Jan 5, 2020 at 12:47 PM Bruno Haible <bruno@clisp.org> wrote:
>
> 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  <bruno@clisp.org>
>
>         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.

Thanks for those fixes. With those and the three
multithread-test-disabling changes, I confirm that all of GNU sed's
tests pass when built using the latest from gcc master (GCC10-pre).


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction
  2020-01-05 21:52       ` Jim Meyering
@ 2020-01-05 22:57         ` Bruno Haible
  0 siblings, 0 replies; 7+ messages in thread
From: Bruno Haible @ 2020-01-05 22:57 UTC (permalink / raw)
  To: Jim Meyering; +Cc: Martin Liška, bug-gnulib

Hi Jim,

> Thanks for those fixes. With those and the three
> multithread-test-disabling changes, I confirm that all of GNU sed's
> tests pass when built using the latest from gcc master (GCC10-pre).

Good. Two other reports of this issue can now be closed:
  - in sed: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=38280
  - in findutils: https://savannah.gnu.org/bugs/index.php?57277

Bruno



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-01-05 22:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-05  6:24 [PATCH] stdlib: avoid canonicalize_file_name contradiction Jim Meyering
2020-01-05  8:15 ` Bruno Haible
2020-01-05 16:41   ` Jim Meyering
2020-01-05 16:46     ` Jim Meyering
2020-01-05 20:46     ` Bruno Haible
2020-01-05 21:52       ` Jim Meyering
2020-01-05 22:57         ` Bruno Haible

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).