Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction

[Jim Meyering <jim@meyering.net>]

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