Re: [PATCH] non-null declarations

[Bruno Haible]

Hi Eric,

Thanks for the detailed review!

Eric Blake wrote:
> >   - Is it worth putting the macro definition (always the same 10 lines of 
> > code)
> >     into a separate file, like done with link-warning.h?
> 
> I'd go one step further - I'd like to see it in config.h

The reason for putting it into the header files, not config.h, is so that
the generated .h files can be installed in public places and used without
config.h, with only a minimum amount of workarounds.

> as was done with  
> _UNUSED_PARAMETER_ in the AH_VERBATIM block of gnulib-common.m4.

Actually this way of using gnulib-common.m4 leads to bugs in packages that
install header files from gnulib:
 <http://lists.gnu.org/archive/html/bug-libunistring/2009-12/msg00002.html>
I fixed that locally in libunistring, but maybe it would be better to fix it
in gnulib: by moving the definition of _UNUSED_PARAMETER_ to a file that
is treated like link-warning.h.

> >  extern int scandir (const char *dir, struct dirent ***namelist,
> >                 int (*filter) (const struct dirent *),
> > -               int (*cmp) (const struct dirent **, const struct dirent 
> > **));
> > +               int (*cmp) (const struct dirent **, const struct dirent **))
> > +     _GL_ARG_NONNULL ((1, 2, 4));
> 
> Is there any desire to provide guaranteed extension semantics that cmp can be 
> NULL in order to get an unsorted subset returned?

I don't think this would be portable. You can in any case get an unsorted
result by passing a cmp function that always returns 0:
  int cmp (const struct dirent **d1, const struct dirent **d2) { return 0; }

> > -extern locale_t duplocale (locale_t locale);
> > +extern locale_t duplocale (locale_t locale) _GL_ARG_NONNULL ((1));
> 
> Are we guaranteed that locale_t is a pointer type; or can it be a scalar on 
> some platforms, in which case _GL_ARG_NONNULL might cause grief?

It's not guaranteed, but in all implementations so far it is a pointer.
It's unlikely to be anything else.

> >  extern int fprintf (FILE *fp, const char *format, ...)
> > -       __attribute__ ((__format__ (__printf__, 2, 3)));
> > +       __attribute__ ((__format__ (__printf__, 2, 3))) _GL_ARG_NONNULL 
> > ((2));
> 
> Missed arg 1 (in two declarations of fprintf).

Oops. Corrected.

> > -extern FILE * freopen (const char *filename, const char *mode, FILE 
> > *stream);
> > +extern FILE * freopen (const char *filename, const char *mode, FILE 
> > *stream)
> > +     _GL_ARG_NONNULL ((1, 2, 3));
> 
> Bug: Arg 1 should not be listed.  Cygwin DEPENDS on being able to pass NULL 
> filename to change streams from text to binary.

Oops, I wasn't aware of this part of the freopen spec. Corrected.

> >  extern int snprintf (char *str, size_t size, const char *format, ...)
> > -       __attribute__ ((__format__ (__printf__, 3, 4)));
> > +       __attribute__ ((__format__ (__printf__, 3, 4)))
> > +       _GL_ARG_NONNULL ((1, 3));
> 
> Bug: Arg 1 should not be listed.  POSIX requires that str may be null if size 
> is 0.

Corrected.

> > -extern int setenv (const char *name, const char *value, int replace);
> > +extern int setenv (const char *name, const char *value, int replace)
> > +     _GL_ARG_NONNULL ((1));
> 
> POSIX and gnulib guarantee that calling this with NAME set to NULL will 
> gracefully fail with EINVAL, so maybe we shouldn't list this one.

But it's a dubious use of the setenv function, and therefore it will be
beneficial to warn for this case.

> >  extern void *memchr (void const *__s, int __c, size_t __n)
> > -  __attribute__ ((__pure__));
> > +     __attribute__ ((__pure__)) _GL_ARG_NONNULL ((1));
> 
> This one I'm fuzzy on.  On one hand, the POSIX definition is that the 
> function 
> reads at most __c bytes, so when __c is 0, it is safe for __s to be NULL.  On 
> the other hand, glibc already warns here, the gnulib testsuite already jumps 
> through hoops to avoid the warning, and the likelihood of meaning to pass 
> NULL 
> when __c is 0 is slim.

Yes. It is theoretically possible that, based on glibc's or our __nonnull__
declaration, a static analyzer program like 'clang' gives a warning for
correct code. But such code will be tricky to understand. The warning will
be justified in any case.

> > -extern int rpl_recvfrom (int, void *, int, int, struct sockaddr *, int *);
> > +extern int rpl_recvfrom (int, void *, int, int, struct sockaddr *, int *)
> > +     _GL_ARG_NONNULL ((2, 6));
> 
> Bug: Arg 6 can be NULL, since POSIX states that "The recv( ) function is 
> equivalent to recvfrom( ) with a zero address_len argument" (well, 
> technically, 
> a null address_len argument only makes sense when address [arg 5] is also 
> NULL).

Indeed, when the 5th argument is NULL, the 6th argument can be NULL as well.
Corrected.

> > -extern int rpl_sendto (int, const void *, int, int, struct sockaddr *, 
> > int);
> > +extern int rpl_sendto (int, const void *, int, int, struct sockaddr *, int)
> > +     _GL_ARG_NONNULL ((2, 5));
> 
> Bug: Arg 5 can be NULL, since POSIX states that "The send( ) function is 
> equivalent to sendto( ) with a null pointer dest_len argument" (well, that's 
> also a bug in POSIX, since dest_len is socklen_t; it obviously meant a null 
> dest_addr argument).

The only indication in the sendto specification that I can see is the
sentence "If the socket is connection-mode, dest_addr shall be ignored."
So, if a user knows that the socket is connection-mode, he may pass NULL.
But I would qualify this as dangerous practice, therefore I leave the warning
in.

> > -extern int futimens (int fd, struct timespec const times[2]);
> > +extern int futimens (int fd, struct timespec const times[2])
> > +     _GL_ARG_NONNULL ((2));
> 
> Bug: Arg 2 can be NULL, to specify 'now'.  This function should not have a 
> nonnull decoration.

Oops. Corrected.

> >     extern int utimensat (int fd, char const *name,
> > -                         struct timespec const times[2], int flag);
> > +                         struct timespec const times[2], int flag)
> > +        _GL_ARG_NONNULL ((2, 3));
> 
> Likewise, Arg 3 should not be listed.

Likewise, corrected.

Bruno