diff mbox series

[v7] libio: Add nonnull attribute for most FILE * arguments in stdio.h

Message ID 20230925115325.176968-2-xry111@xry111.site
State New
Headers show
Series [v7] libio: Add nonnull attribute for most FILE * arguments in stdio.h | expand

Commit Message

Xi Ruoyao Sept. 25, 2023, 11:53 a.m. UTC 
During the review of a GCC analyzer test case, we found most stdio
functions accepting a FILE * argument expect it to be nonnull and just
segfault when the argument is NULL.  Add nonnull attribute for them.

fflush and fflush_unlocked are well defined when __stream is NULL so
they are not touched.

For fputs, fgets, fread, fwrite, fprintf, vfprintf, and their unlocked
version, if __stream is empty but there is nothing to read or write,
they did not segfault.  But the standard disallow __stream to be empty
here, so nonnull attribute is also added for them.  Note that this may
blow up some old code already subtly broken.

Also add __nonnull for _chk variants and __fortify_function versions for
them.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>
---

No change since v6, resubmit for today's patch review.

IIRC our conclusion in 2.38 dev cycle was:

1. If this may be acceptable, we'll apply it early in 2.39 dev cycle
   and see if things goes well.
2. If this is not acceptable at all, we'll revert commit
   71d9e0fe766a3c22a730995b9d024960970670af.

Now it's the end of the second month in 2.39 dev cycle, so let's make a
decision.

 libio/bits/stdio2-decl.h |  15 ++--
 libio/bits/stdio2.h      |  14 ++--
 libio/stdio.h            | 148 ++++++++++++++++++++++-----------------
 3 files changed, 99 insertions(+), 78 deletions(-)

Comments

Alejandro Colomar Sept. 25, 2023, noon UTC | #1 
On Mon, Sep 25, 2023 at 07:53:26PM +0800, Xi Ruoyao wrote:
> During the review of a GCC analyzer test case, we found most stdio
> functions accepting a FILE * argument expect it to be nonnull and just
> segfault when the argument is NULL.  Add nonnull attribute for them.
> 
> fflush and fflush_unlocked are well defined when __stream is NULL so
> they are not touched.
> 
> For fputs, fgets, fread, fwrite, fprintf, vfprintf, and their unlocked
> version, if __stream is empty but there is nothing to read or write,
> they did not segfault.  But the standard disallow __stream to be empty
> here, so nonnull attribute is also added for them.  Note that this may
> blow up some old code already subtly broken.
> 
> Also add __nonnull for _chk variants and __fortify_function versions for
> them.
> 
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>

Acked-by: Alejandro Colomar <alx@kernel.org>

> ---
> 
> No change since v6, resubmit for today's patch review.
> 
> IIRC our conclusion in 2.38 dev cycle was:
> 
> 1. If this may be acceptable, we'll apply it early in 2.39 dev cycle
>    and see if things goes well.
> 2. If this is not acceptable at all, we'll revert commit
>    71d9e0fe766a3c22a730995b9d024960970670af.
> 
> Now it's the end of the second month in 2.39 dev cycle, so let's make a
> decision.

+1

Cheers,
Alex

> 
>  libio/bits/stdio2-decl.h |  15 ++--
>  libio/bits/stdio2.h      |  14 ++--
>  libio/stdio.h            | 148 ++++++++++++++++++++++-----------------
>  3 files changed, 99 insertions(+), 78 deletions(-)
> 
> diff --git a/libio/bits/stdio2-decl.h b/libio/bits/stdio2-decl.h
> index d7ef7283d6..3b60c72931 100644
> --- a/libio/bits/stdio2-decl.h
> +++ b/libio/bits/stdio2-decl.h
> @@ -47,10 +47,12 @@ extern int __vsnprintf_chk (char *__restrict __s, size_t __n, int __flag,
>  #if __USE_FORTIFY_LEVEL > 1
>  
>  extern int __fprintf_chk (FILE *__restrict __stream, int __flag,
> -			  const char *__restrict __format, ...);
> +			  const char *__restrict __format, ...)
> +    __nonnull ((1));
>  extern int __printf_chk (int __flag, const char *__restrict __format, ...);
>  extern int __vfprintf_chk (FILE *__restrict __stream, int __flag,
> -			   const char *__restrict __format, __gnuc_va_list __ap);
> +			   const char *__restrict __format,
> +			   __gnuc_va_list __ap) __nonnull ((1));
>  extern int __vprintf_chk (int __flag, const char *__restrict __format,
>  			  __gnuc_va_list __ap);
>  
> @@ -103,7 +105,7 @@ extern char *__REDIRECT (__fgets_chk_warn,
>  
>  extern char *__fgets_chk (char *__restrict __s, size_t __size, int __n,
>  			  FILE *__restrict __stream)
> -    __wur __attr_access ((__write_only__, 1, 3));
> +    __wur __attr_access ((__write_only__, 1, 3)) __nonnull ((4));
>  
>  extern size_t __REDIRECT (__fread_alias,
>  			  (void *__restrict __ptr, size_t __size,
> @@ -119,7 +121,7 @@ extern size_t __REDIRECT (__fread_chk_warn,
>  
>  extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen,
>  			   size_t __size, size_t __n,
> -			   FILE *__restrict __stream) __wur;
> +			   FILE *__restrict __stream) __wur __nonnull ((5));
>  
>  #ifdef __USE_GNU
>  extern char *__REDIRECT_FORTIFY (__fgets_unlocked_alias,
> @@ -135,7 +137,7 @@ extern char *__REDIRECT (__fgets_unlocked_chk_warn,
>  
>  extern char *__fgets_unlocked_chk (char *__restrict __s, size_t __size,
>  				   int __n, FILE *__restrict __stream)
> -    __wur __attr_access ((__write_only__, 1, 3));
> +    __wur __attr_access ((__write_only__, 1, 3)) __nonnull ((4));
>  #endif
>  
>  #ifdef __USE_MISC
> @@ -154,7 +156,8 @@ extern size_t __REDIRECT (__fread_unlocked_chk_warn,
>  
>  extern size_t __fread_unlocked_chk (void *__restrict __ptr, size_t __ptrlen,
>  				    size_t __size, size_t __n,
> -				    FILE *__restrict __stream) __wur;
> +				    FILE *__restrict __stream)
> +    __wur __nonnull ((5));
>  #endif
>  
>  #endif /* bits/stdio2-decl.h.  */
> diff --git a/libio/bits/stdio2.h b/libio/bits/stdio2.h
> index 71226408ab..266dccdd1e 100644
> --- a/libio/bits/stdio2.h
> +++ b/libio/bits/stdio2.h
> @@ -73,7 +73,7 @@ __NTH (vsnprintf (char *__restrict __s, size_t __n,
>  
>  #if __USE_FORTIFY_LEVEL > 1
>  # ifdef __va_arg_pack
> -__fortify_function int
> +__fortify_function __nonnull ((1)) int
>  fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
>  {
>    return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
> @@ -102,7 +102,7 @@ vprintf (const char *__restrict __fmt, __gnuc_va_list __ap)
>  #endif
>  }
>  
> -__fortify_function int
> +__fortify_function __nonnull ((1)) int
>  vfprintf (FILE *__restrict __stream,
>  	  const char *__restrict __fmt, __gnuc_va_list __ap)
>  {
> @@ -191,7 +191,8 @@ gets (char *__str)
>  }
>  #endif
>  
> -__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2) char *
> +__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2)
> +__nonnull ((3)) char *
>  fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
>  {
>    size_t sz = __glibc_objsize (__s);
> @@ -202,7 +203,7 @@ fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
>    return __fgets_chk (__s, sz, __n, __stream);
>  }
>  
> -__fortify_function __wur size_t
> +__fortify_function __wur __nonnull ((4)) size_t
>  fread (void *__restrict __ptr, size_t __size, size_t __n,
>         FILE *__restrict __stream)
>  {
> @@ -215,7 +216,8 @@ fread (void *__restrict __ptr, size_t __size, size_t __n,
>  }
>  
>  #ifdef __USE_GNU
> -__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2) char *
> +__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2)
> +__nonnull ((3)) char *
>  fgets_unlocked (char *__restrict __s, int __n, FILE *__restrict __stream)
>  {
>    size_t sz = __glibc_objsize (__s);
> @@ -229,7 +231,7 @@ fgets_unlocked (char *__restrict __s, int __n, FILE *__restrict __stream)
>  
>  #ifdef __USE_MISC
>  # undef fread_unlocked
> -__fortify_function __wur size_t
> +__fortify_function __wur __nonnull ((4)) size_t
>  fread_unlocked (void *__restrict __ptr, size_t __size, size_t __n,
>  		FILE *__restrict __stream)
>  {
> diff --git a/libio/stdio.h b/libio/stdio.h
> index 4cf9f1c012..a640e9beb5 100644
> --- a/libio/stdio.h
> +++ b/libio/stdio.h
> @@ -1,5 +1,6 @@
>  /* Define ISO C stdio on top of C++ iostreams.
>     Copyright (C) 1991-2023 Free Software Foundation, Inc.
> +   Copyright The GNU Toolchain Authors.
>     This file is part of the GNU C Library.
>  
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -278,7 +279,7 @@ extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
>  extern FILE *__REDIRECT (freopen, (const char *__restrict __filename,
>  				   const char *__restrict __modes,
>  				   FILE *__restrict __stream), freopen64)
> -  __wur;
> +  __wur __nonnull ((3));
>  # else
>  #  define fopen fopen64
>  #  define freopen freopen64
> @@ -330,21 +331,22 @@ extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t *__sizeloc) __THROW
>  
>  /* If BUF is NULL, make STREAM unbuffered.
>     Else make it use buffer BUF, of size BUFSIZ.  */
> -extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW;
> +extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW
> +  __nonnull ((1));
>  /* Make STREAM use buffering mode MODE.
>     If BUF is not NULL, use N bytes of it for buffering;
>     else allocate an internal buffer N bytes long.  */
>  extern int setvbuf (FILE *__restrict __stream, char *__restrict __buf,
> -		    int __modes, size_t __n) __THROW;
> +		    int __modes, size_t __n) __THROW __nonnull ((1));
>  
>  #ifdef	__USE_MISC
>  /* If BUF is NULL, make STREAM unbuffered.
>     Else make it use SIZE bytes of BUF for buffering.  */
>  extern void setbuffer (FILE *__restrict __stream, char *__restrict __buf,
> -		       size_t __size) __THROW;
> +		       size_t __size) __THROW __nonnull ((1));
>  
>  /* Make STREAM line-buffered.  */
> -extern void setlinebuf (FILE *__stream) __THROW;
> +extern void setlinebuf (FILE *__stream) __THROW __nonnull ((1));
>  #endif
>  
>  
> @@ -353,7 +355,7 @@ extern void setlinebuf (FILE *__stream) __THROW;
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
>  extern int fprintf (FILE *__restrict __stream,
> -		    const char *__restrict __format, ...);
> +		    const char *__restrict __format, ...) __nonnull ((1));
>  /* Write formatted output to stdout.
>  
>     This function is a possible cancellation point and therefore not
> @@ -368,7 +370,7 @@ extern int sprintf (char *__restrict __s,
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
>  extern int vfprintf (FILE *__restrict __s, const char *__restrict __format,
> -		     __gnuc_va_list __arg);
> +		     __gnuc_va_list __arg) __nonnull ((1));
>  /* Write formatted output to stdout from argument list ARG.
>  
>     This function is a possible cancellation point and therefore not
> @@ -418,7 +420,7 @@ extern int dprintf (int __fd, const char *__restrict __fmt, ...)
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
>  extern int fscanf (FILE *__restrict __stream,
> -		   const char *__restrict __format, ...) __wur;
> +		   const char *__restrict __format, ...) __wur __nonnull ((1));
>  /* Read formatted input from stdin.
>  
>     This function is a possible cancellation point and therefore not
> @@ -439,7 +441,7 @@ extern int sscanf (const char *__restrict __s,
>  #  ifdef __REDIRECT
>  extern int __REDIRECT (fscanf, (FILE *__restrict __stream,
>  				const char *__restrict __format, ...),
> -		       __isoc23_fscanf) __wur;
> +		       __isoc23_fscanf) __wur __nonnull ((1));
>  extern int __REDIRECT (scanf, (const char *__restrict __format, ...),
>  		       __isoc23_scanf) __wur;
>  extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
> @@ -447,7 +449,8 @@ extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
>  			   __isoc23_sscanf);
>  #  else
>  extern int __isoc23_fscanf (FILE *__restrict __stream,
> -			    const char *__restrict __format, ...) __wur;
> +			    const char *__restrict __format, ...) __wur
> +  __nonnull ((1));
>  extern int __isoc23_scanf (const char *__restrict __format, ...) __wur;
>  extern int __isoc23_sscanf (const char *__restrict __s,
>  			    const char *__restrict __format, ...) __THROW;
> @@ -459,7 +462,7 @@ extern int __isoc23_sscanf (const char *__restrict __s,
>  #  ifdef __REDIRECT
>  extern int __REDIRECT (fscanf, (FILE *__restrict __stream,
>  				const char *__restrict __format, ...),
> -		       __isoc99_fscanf) __wur;
> +		       __isoc99_fscanf) __wur __nonnull ((1));
>  extern int __REDIRECT (scanf, (const char *__restrict __format, ...),
>  		       __isoc99_scanf) __wur;
>  extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
> @@ -467,7 +470,8 @@ extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
>  			   __isoc99_sscanf);
>  #  else
>  extern int __isoc99_fscanf (FILE *__restrict __stream,
> -			    const char *__restrict __format, ...) __wur;
> +			    const char *__restrict __format, ...) __wur
> +  __nonnull ((1));
>  extern int __isoc99_scanf (const char *__restrict __format, ...) __wur;
>  extern int __isoc99_sscanf (const char *__restrict __s,
>  			    const char *__restrict __format, ...) __THROW;
> @@ -485,7 +489,7 @@ extern int __isoc99_sscanf (const char *__restrict __s,
>     marked with __THROW.  */
>  extern int vfscanf (FILE *__restrict __s, const char *__restrict __format,
>  		    __gnuc_va_list __arg)
> -     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
> +     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
>  
>  /* Read formatted input from stdin into argument list ARG.
>  
> @@ -508,7 +512,7 @@ extern int __REDIRECT (vfscanf,
>  		       (FILE *__restrict __s,
>  			const char *__restrict __format, __gnuc_va_list __arg),
>  		       __isoc23_vfscanf)
> -     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
> +     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
>  extern int __REDIRECT (vscanf, (const char *__restrict __format,
>  				__gnuc_va_list __arg), __isoc23_vscanf)
>       __attribute__ ((__format__ (__scanf__, 1, 0))) __wur;
> @@ -520,7 +524,7 @@ extern int __REDIRECT_NTH (vsscanf,
>  #   elif !defined __REDIRECT
>  extern int __isoc23_vfscanf (FILE *__restrict __s,
>  			     const char *__restrict __format,
> -			     __gnuc_va_list __arg) __wur;
> +			     __gnuc_va_list __arg) __wur __nonnull ((1));
>  extern int __isoc23_vscanf (const char *__restrict __format,
>  			    __gnuc_va_list __arg) __wur;
>  extern int __isoc23_vsscanf (const char *__restrict __s,
> @@ -537,7 +541,7 @@ extern int __REDIRECT (vfscanf,
>  		       (FILE *__restrict __s,
>  			const char *__restrict __format, __gnuc_va_list __arg),
>  		       __isoc99_vfscanf)
> -     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
> +     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
>  extern int __REDIRECT (vscanf, (const char *__restrict __format,
>  				__gnuc_va_list __arg), __isoc99_vscanf)
>       __attribute__ ((__format__ (__scanf__, 1, 0))) __wur;
> @@ -549,7 +553,7 @@ extern int __REDIRECT_NTH (vsscanf,
>  #   elif !defined __REDIRECT
>  extern int __isoc99_vfscanf (FILE *__restrict __s,
>  			     const char *__restrict __format,
> -			     __gnuc_va_list __arg) __wur;
> +			     __gnuc_va_list __arg) __wur __nonnull ((1));
>  extern int __isoc99_vscanf (const char *__restrict __format,
>  			    __gnuc_va_list __arg) __wur;
>  extern int __isoc99_vsscanf (const char *__restrict __s,
> @@ -568,8 +572,8 @@ extern int __isoc99_vsscanf (const char *__restrict __s,
>  
>     These functions are possible cancellation points and therefore not
>     marked with __THROW.  */
> -extern int fgetc (FILE *__stream);
> -extern int getc (FILE *__stream);
> +extern int fgetc (FILE *__stream) __nonnull ((1));
> +extern int getc (FILE *__stream) __nonnull ((1));
>  
>  /* Read a character from stdin.
>  
> @@ -582,7 +586,7 @@ extern int getchar (void);
>  
>     These functions are possible cancellation points and therefore not
>     marked with __THROW.  */
> -extern int getc_unlocked (FILE *__stream);
> +extern int getc_unlocked (FILE *__stream) __nonnull ((1));
>  extern int getchar_unlocked (void);
>  #endif /* Use POSIX.  */
>  
> @@ -593,7 +597,7 @@ extern int getchar_unlocked (void);
>     cancellation point.  But due to similarity with an POSIX interface
>     or due to the implementation it is a cancellation point and
>     therefore not marked with __THROW.  */
> -extern int fgetc_unlocked (FILE *__stream);
> +extern int fgetc_unlocked (FILE *__stream) __nonnull ((1));
>  #endif /* Use MISC.  */
>  
>  
> @@ -604,8 +608,8 @@ extern int fgetc_unlocked (FILE *__stream);
>  
>     These functions is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int fputc (int __c, FILE *__stream);
> -extern int putc (int __c, FILE *__stream);
> +extern int fputc (int __c, FILE *__stream) __nonnull ((2));
> +extern int putc (int __c, FILE *__stream) __nonnull ((2));
>  
>  /* Write a character to stdout.
>  
> @@ -620,7 +624,7 @@ extern int putchar (int __c);
>     cancellation point.  But due to similarity with an POSIX interface
>     or due to the implementation it is a cancellation point and
>     therefore not marked with __THROW.  */
> -extern int fputc_unlocked (int __c, FILE *__stream);
> +extern int fputc_unlocked (int __c, FILE *__stream) __nonnull ((2));
>  #endif /* Use MISC.  */
>  
>  #ifdef __USE_POSIX199506
> @@ -628,7 +632,7 @@ extern int fputc_unlocked (int __c, FILE *__stream);
>  
>     These functions are possible cancellation points and therefore not
>     marked with __THROW.  */
> -extern int putc_unlocked (int __c, FILE *__stream);
> +extern int putc_unlocked (int __c, FILE *__stream) __nonnull ((2));
>  extern int putchar_unlocked (int __c);
>  #endif /* Use POSIX.  */
>  
> @@ -636,10 +640,10 @@ extern int putchar_unlocked (int __c);
>  #if defined __USE_MISC \
>      || (defined __USE_XOPEN && !defined __USE_XOPEN2K)
>  /* Get a word (int) from STREAM.  */
> -extern int getw (FILE *__stream);
> +extern int getw (FILE *__stream) __nonnull ((1));
>  
>  /* Write a word (int) to STREAM.  */
> -extern int putw (int __w, FILE *__stream);
> +extern int putw (int __w, FILE *__stream) __nonnull ((2));
>  #endif
>  
>  
> @@ -648,7 +652,7 @@ extern int putw (int __w, FILE *__stream);
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
>  extern char *fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
> -     __wur __fortified_attr_access (__write_only__, 1, 2);
> +     __wur __fortified_attr_access (__write_only__, 1, 2) __nonnull ((3));
>  
>  #if __GLIBC_USE (DEPRECATED_GETS)
>  /* Get a newline-terminated string from stdin, removing the newline.
> @@ -672,7 +676,7 @@ extern char *gets (char *__s) __wur __attribute_deprecated__;
>     therefore not marked with __THROW.  */
>  extern char *fgets_unlocked (char *__restrict __s, int __n,
>  			     FILE *__restrict __stream) __wur
> -    __fortified_attr_access (__write_only__, 1, 2);
> +    __fortified_attr_access (__write_only__, 1, 2) __nonnull ((3));
>  #endif
>  
>  
> @@ -689,10 +693,10 @@ extern char *fgets_unlocked (char *__restrict __s, int __n,
>     therefore not marked with __THROW.  */
>  extern __ssize_t __getdelim (char **__restrict __lineptr,
>                               size_t *__restrict __n, int __delimiter,
> -                             FILE *__restrict __stream) __wur;
> +                             FILE *__restrict __stream) __wur __nonnull ((4));
>  extern __ssize_t getdelim (char **__restrict __lineptr,
>                             size_t *__restrict __n, int __delimiter,
> -                           FILE *__restrict __stream) __wur;
> +                           FILE *__restrict __stream) __wur __nonnull ((4));
>  
>  /* Like `getdelim', but reads up to a newline.
>  
> @@ -702,7 +706,7 @@ extern __ssize_t getdelim (char **__restrict __lineptr,
>     therefore not marked with __THROW.  */
>  extern __ssize_t getline (char **__restrict __lineptr,
>                            size_t *__restrict __n,
> -                          FILE *__restrict __stream) __wur;
> +                          FILE *__restrict __stream) __wur __nonnull ((3));
>  #endif
>  
>  
> @@ -710,7 +714,8 @@ extern __ssize_t getline (char **__restrict __lineptr,
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int fputs (const char *__restrict __s, FILE *__restrict __stream);
> +extern int fputs (const char *__restrict __s, FILE *__restrict __stream)
> +  __nonnull ((2));
>  
>  /* Write a string, followed by a newline, to stdout.
>  
> @@ -723,7 +728,7 @@ extern int puts (const char *__s);
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int ungetc (int __c, FILE *__stream);
> +extern int ungetc (int __c, FILE *__stream) __nonnull ((2));
>  
>  
>  /* Read chunks of generic data from STREAM.
> @@ -731,13 +736,14 @@ extern int ungetc (int __c, FILE *__stream);
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
>  extern size_t fread (void *__restrict __ptr, size_t __size,
> -		     size_t __n, FILE *__restrict __stream) __wur;
> +		     size_t __n, FILE *__restrict __stream) __wur
> +  __nonnull((4));
>  /* Write chunks of generic data to STREAM.
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
>  extern size_t fwrite (const void *__restrict __ptr, size_t __size,
> -		      size_t __n, FILE *__restrict __s);
> +		      size_t __n, FILE *__restrict __s) __nonnull((4));
>  
>  #ifdef __USE_GNU
>  /* This function does the same as `fputs' but does not lock the stream.
> @@ -747,7 +753,7 @@ extern size_t fwrite (const void *__restrict __ptr, size_t __size,
>     or due to the implementation it is a cancellation point and
>     therefore not marked with __THROW.  */
>  extern int fputs_unlocked (const char *__restrict __s,
> -			   FILE *__restrict __stream);
> +			   FILE *__restrict __stream) __nonnull ((2));
>  #endif
>  
>  #ifdef __USE_MISC
> @@ -758,9 +764,11 @@ extern int fputs_unlocked (const char *__restrict __s,
>     or due to the implementation they are cancellation points and
>     therefore not marked with __THROW.  */
>  extern size_t fread_unlocked (void *__restrict __ptr, size_t __size,
> -			      size_t __n, FILE *__restrict __stream) __wur;
> +			      size_t __n, FILE *__restrict __stream) __wur
> +  __nonnull ((4));
>  extern size_t fwrite_unlocked (const void *__restrict __ptr, size_t __size,
> -			       size_t __n, FILE *__restrict __stream);
> +			       size_t __n, FILE *__restrict __stream)
> +  __nonnull ((4));
>  #endif
>  
>  
> @@ -768,17 +776,18 @@ extern size_t fwrite_unlocked (const void *__restrict __ptr, size_t __size,
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int fseek (FILE *__stream, long int __off, int __whence);
> +extern int fseek (FILE *__stream, long int __off, int __whence)
> +  __nonnull ((1));
>  /* Return the current position of STREAM.
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern long int ftell (FILE *__stream) __wur;
> +extern long int ftell (FILE *__stream) __wur __nonnull ((1));
>  /* Rewind to the beginning of STREAM.
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern void rewind (FILE *__stream);
> +extern void rewind (FILE *__stream) __nonnull ((1));
>  
>  /* The Single Unix Specification, Version 2, specifies an alternative,
>     more adequate interface for the two functions above which deal with
> @@ -791,18 +800,20 @@ extern void rewind (FILE *__stream);
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int fseeko (FILE *__stream, __off_t __off, int __whence);
> +extern int fseeko (FILE *__stream, __off_t __off, int __whence)
> +  __nonnull ((1));
>  /* Return the current position of STREAM.
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern __off_t ftello (FILE *__stream) __wur;
> +extern __off_t ftello (FILE *__stream) __wur __nonnull ((1));
>  # else
>  #  ifdef __REDIRECT
>  extern int __REDIRECT (fseeko,
>  		       (FILE *__stream, __off64_t __off, int __whence),
> -		       fseeko64);
> -extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64);
> +		       fseeko64) __nonnull ((1));
> +extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64)
> +  __nonnull ((1));
>  #  else
>  #   define fseeko fseeko64
>  #   define ftello ftello64
> @@ -815,18 +826,21 @@ extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64);
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int fgetpos (FILE *__restrict __stream, fpos_t *__restrict __pos);
> +extern int fgetpos (FILE *__restrict __stream, fpos_t *__restrict __pos)
> +  __nonnull ((1));
>  /* Set STREAM's position.
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int fsetpos (FILE *__stream, const fpos_t *__pos);
> +extern int fsetpos (FILE *__stream, const fpos_t *__pos) __nonnull ((1));
>  #else
>  # ifdef __REDIRECT
>  extern int __REDIRECT (fgetpos, (FILE *__restrict __stream,
> -				 fpos_t *__restrict __pos), fgetpos64);
> +				 fpos_t *__restrict __pos), fgetpos64)
> +  __nonnull ((1));
>  extern int __REDIRECT (fsetpos,
> -		       (FILE *__stream, const fpos_t *__pos), fsetpos64);
> +		       (FILE *__stream, const fpos_t *__pos), fsetpos64)
> +  __nonnull ((1));
>  # else
>  #  define fgetpos fgetpos64
>  #  define fsetpos fsetpos64
> @@ -834,24 +848,26 @@ extern int __REDIRECT (fsetpos,
>  #endif
>  
>  #ifdef __USE_LARGEFILE64
> -extern int fseeko64 (FILE *__stream, __off64_t __off, int __whence);
> -extern __off64_t ftello64 (FILE *__stream) __wur;
> -extern int fgetpos64 (FILE *__restrict __stream, fpos64_t *__restrict __pos);
> -extern int fsetpos64 (FILE *__stream, const fpos64_t *__pos);
> +extern int fseeko64 (FILE *__stream, __off64_t __off, int __whence)
> +  __nonnull ((1));
> +extern __off64_t ftello64 (FILE *__stream) __wur __nonnull ((1));
> +extern int fgetpos64 (FILE *__restrict __stream, fpos64_t *__restrict __pos)
> +  __nonnull ((1));
> +extern int fsetpos64 (FILE *__stream, const fpos64_t *__pos) __nonnull ((1));
>  #endif
>  
>  /* Clear the error and EOF indicators for STREAM.  */
> -extern void clearerr (FILE *__stream) __THROW;
> +extern void clearerr (FILE *__stream) __THROW __nonnull ((1));
>  /* Return the EOF indicator for STREAM.  */
> -extern int feof (FILE *__stream) __THROW __wur;
> +extern int feof (FILE *__stream) __THROW __wur __nonnull ((1));
>  /* Return the error indicator for STREAM.  */
> -extern int ferror (FILE *__stream) __THROW __wur;
> +extern int ferror (FILE *__stream) __THROW __wur __nonnull ((1));
>  
>  #ifdef __USE_MISC
>  /* Faster versions when locking is not required.  */
> -extern void clearerr_unlocked (FILE *__stream) __THROW;
> -extern int feof_unlocked (FILE *__stream) __THROW __wur;
> -extern int ferror_unlocked (FILE *__stream) __THROW __wur;
> +extern void clearerr_unlocked (FILE *__stream) __THROW __nonnull ((1));
> +extern int feof_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
> +extern int ferror_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
>  #endif
>  
>  
> @@ -864,12 +880,12 @@ extern void perror (const char *__s) __COLD;
>  
>  #ifdef	__USE_POSIX
>  /* Return the system file descriptor for STREAM.  */
> -extern int fileno (FILE *__stream) __THROW __wur;
> +extern int fileno (FILE *__stream) __THROW __wur __nonnull ((1));
>  #endif /* Use POSIX.  */
>  
>  #ifdef __USE_MISC
>  /* Faster version when locking is not required.  */
> -extern int fileno_unlocked (FILE *__stream) __THROW __wur;
> +extern int fileno_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
>  #endif
>  
>  
> @@ -878,7 +894,7 @@ extern int fileno_unlocked (FILE *__stream) __THROW __wur;
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int pclose (FILE *__stream);
> +extern int pclose (FILE *__stream) __nonnull ((1));
>  
>  /* Create a new stream connected to a pipe running the given command.
>  
> @@ -922,14 +938,14 @@ extern int obstack_vprintf (struct obstack *__restrict __obstack,
>  /* These are defined in POSIX.1:1996.  */
>  
>  /* Acquire ownership of STREAM.  */
> -extern void flockfile (FILE *__stream) __THROW;
> +extern void flockfile (FILE *__stream) __THROW __nonnull ((1));
>  
>  /* Try to acquire ownership of STREAM but do not block if it is not
>     possible.  */
> -extern int ftrylockfile (FILE *__stream) __THROW __wur;
> +extern int ftrylockfile (FILE *__stream) __THROW __wur __nonnull ((1));
>  
>  /* Relinquish the ownership granted for STREAM.  */
> -extern void funlockfile (FILE *__stream) __THROW;
> +extern void funlockfile (FILE *__stream) __THROW __nonnull ((1));
>  #endif /* POSIX */
>  
>  #if defined __USE_XOPEN && !defined __USE_XOPEN2K && !defined __USE_GNU
> -- 
> 2.42.0
>
Zack Weinberg Sept. 25, 2023, 2:10 p.m. UTC | #2 
On Mon, Sep 25, 2023, at 8:00 AM, Alejandro Colomar wrote:
> On Mon, Sep 25, 2023 at 07:53:26PM +0800, Xi Ruoyao wrote:
>>
>> IIRC our conclusion in 2.38 dev cycle was:
>>
>> 1. If this may be acceptable, we'll apply it early in 2.39 dev cycle
>>    and see if things goes well.
>> 2. If this is not acceptable at all, we'll revert commit
>>    71d9e0fe766a3c22a730995b9d024960970670af.
>>
>> Now it's the end of the second month in 2.39 dev cycle, so let's make
>> a decision.
>
> +1

I still think this is a dangerous idea and, if we do it at all, we
should do it *only* for compilers that have a *documented guarantee*
that control flow paths that provably pass a NULL pointer to a __nonnull
argument will *not* be treated as unreachable.  If I understood the
previous discussion correctly, GCC currently does not do that, but there
is no documented guarantee that it will *never* do that, and nobody
chimed in from LLVM's side.

(It's probably fine if compilers treat calls that pass a NULL pointer to
a __nonnull argument as noreturn, or if they replace them with a trap
instruction or a call to abort().  The danger I foresee is from any
circumstance where NULL checks and side effects that happen *before* the
bad call can get deleted.)

(I think this policy should be applied to *all* instances of __nonnull
in glibc's headers, not just new ones.)

zw
Xi Ruoyao Sept. 25, 2023, 2:29 p.m. UTC | #3 
On Mon, 2023-09-25 at 10:10 -0400, Zack Weinberg wrote:

> (It's probably fine if compilers treat calls that pass a NULL pointer to
> a __nonnull argument as noreturn, or if they replace them with a trap
> instruction or a call to abort().  The danger I foresee is from any
> circumstance where NULL checks and side effects that happen *before* the
> bad call can get deleted.)

Regarding this, gcc man page says:

       -fisolate-erroneous-paths-attribute
           Detect paths that trigger erroneous or undefined behavior due to  a
           null  value being used in a way forbidden by a "returns_nonnull" or
           "nonnull" attribute.  Isolate those paths  from  the  main  control
           flow  and  turn  the statement with erroneous or undefined behavior
           into a trap.  This is not currently enabled, but may be enabled  by
           -O2 in the future.

So I think maybe it's now the time to make this enabled by -O2 now (for
GCC 14 and later).  And maybe GCC can define a macro
__isolate_erroneous_paths_attribute so we can check on it.

How do you think Jeff?
Zack Weinberg Sept. 25, 2023, 2:30 p.m. UTC | #4 
On Mon, Sep 25, 2023, at 10:29 AM, Xi Ruoyao wrote:
> On Mon, 2023-09-25 at 10:10 -0400, Zack Weinberg wrote:
>
>> (It's probably fine if compilers treat calls that pass a NULL pointer to
>> a __nonnull argument as noreturn, or if they replace them with a trap
>> instruction or a call to abort().  The danger I foresee is from any
>> circumstance where NULL checks and side effects that happen *before* the
>> bad call can get deleted.)
>
> Regarding this, gcc man page says:
>
>        -fisolate-erroneous-paths-attribute
>            Detect paths that trigger erroneous or undefined behavior due to  a
>            null  value being used in a way forbidden by a "returns_nonnull" or
>            "nonnull" attribute.  Isolate those paths  from  the  main  control
>            flow  and  turn  the statement with erroneous or undefined behavior
>            into a trap.  This is not currently enabled, but may be enabled  by
>            -O2 in the future.

This is not a clear statement that the path will not be deleted as unreachable,
nor is it a promise never to do so in the future.

zw
Xi Ruoyao Sept. 25, 2023, 2:47 p.m. UTC | #5 
On Mon, 2023-09-25 at 10:30 -0400, Zack Weinberg wrote:
> On Mon, Sep 25, 2023, at 10:29 AM, Xi Ruoyao wrote:
> > On Mon, 2023-09-25 at 10:10 -0400, Zack Weinberg wrote:
> > 
> > > (It's probably fine if compilers treat calls that pass a NULL pointer to
> > > a __nonnull argument as noreturn, or if they replace them with a trap
> > > instruction or a call to abort().  The danger I foresee is from any
> > > circumstance where NULL checks and side effects that happen *before* the
> > > bad call can get deleted.)
> > 
> > Regarding this, gcc man page says:
> > 
> >        -fisolate-erroneous-paths-attribute
> >            Detect paths that trigger erroneous or undefined behavior due to  a
> >            null  value being used in a way forbidden by a "returns_nonnull" or
> >            "nonnull" attribute.  Isolate those paths  from  the  main  control
> >            flow  and  turn  the statement with erroneous or undefined behavior
> >            into a trap.  This is not currently enabled, but may be enabled  by
> >            -O2 in the future.
> 
> This is not a clear statement that the path will not be deleted as unreachable,
> nor is it a promise never to do so in the future.

Then let's revert 71d9e0fe766a3c22a730995b9d024960970670af and backport
the revert into 2.38 branch.

The problem is this commit causes some GCC analyzer test failures.  If
we'll use nonnull I can fix the test cases at GCC side, but if we'll not
use nonnull I don't want to spend my time adding some special cases into
test files.  And I don't want everyone seeing the failures to continue
blaming on me.
Gabriel Ravier Sept. 25, 2023, 3:03 p.m. UTC | #6 
On 9/25/23 15:30, Zack Weinberg wrote:
> On Mon, Sep 25, 2023, at 10:29 AM, Xi Ruoyao wrote:
>> On Mon, 2023-09-25 at 10:10 -0400, Zack Weinberg wrote:
>>
>>> (It's probably fine if compilers treat calls that pass a NULL pointer to
>>> a __nonnull argument as noreturn, or if they replace them with a trap
>>> instruction or a call to abort().  The danger I foresee is from any
>>> circumstance where NULL checks and side effects that happen *before* the
>>> bad call can get deleted.)
>> Regarding this, gcc man page says:
>>
>>         -fisolate-erroneous-paths-attribute
>>             Detect paths that trigger erroneous or undefined behavior due to  a
>>             null  value being used in a way forbidden by a "returns_nonnull" or
>>             "nonnull" attribute.  Isolate those paths  from  the  main  control
>>             flow  and  turn  the statement with erroneous or undefined behavior
>>             into a trap.  This is not currently enabled, but may be enabled  by
>>             -O2 in the future.
> This is not a clear statement that the path will not be deleted as unreachable,
> nor is it a promise never to do so in the future.

It seems to me like such a promise as "we will never do this in the 
future" can not possibly be made as it no one can be certain it will be 
upheld forever. Whoever makes the promise (the "GCC developer team" ? 
The FSF ? The GNU Project ?) cannot be forced to uphold that promise, 
and in fact they could easily, over a long enough period of time, be 
replaced (or have all the people constituting them replaced) by people 
who do not want to uphold the promise. By the standard you're trying to 
enforce, glibc should never add any information or annotations of any 
sort to its headers which could indicate to any compiler that any way of 
calling any of its methods may result in undefined behavior.

I will also add that, if you are assuming GCC has a high chance of 
taking the kind of action you want some kind of guarantee against, then 
glibc refusing to add the annotation seems like it would just result in 
GCC eventually making it so that fprintf and other such functions are 
considered as builtins for the purposes of making the nonnull attribute 
is hard-coded and non-removeable, which would take things entirely out 
of the C library's control - I assume you do not see this as a desirable 
outcome either.

It seems to me like a better idea from the POV of the glibc project 
(since this would remain entirely within glibc's control) to make sure 
in the future that any compilers which take adverse action in the face 
of a nonnull attribute do not see such an attribute (i.e. by detecting 
such compilers and disabling nonnull for them).

>
> zw
Zack Weinberg Sept. 25, 2023, 3:08 p.m. UTC | #7 
On Mon, Sep 25, 2023, at 11:03 AM, Gabriel Ravier wrote:
> On 9/25/23 15:30, Zack Weinberg wrote:
>> On Mon, Sep 25, 2023, at 10:29 AM, Xi Ruoyao wrote:
>>> On Mon, 2023-09-25 at 10:10 -0400, Zack Weinberg wrote:
>>>
>>>> (It's probably fine if compilers treat calls that pass a NULL pointer to
>>>> a __nonnull argument as noreturn, or if they replace them with a trap
>>>> instruction or a call to abort().  The danger I foresee is from any
>>>> circumstance where NULL checks and side effects that happen *before* the
>>>> bad call can get deleted.)
>>> Regarding this, gcc man page says:
>>>
>>>         -fisolate-erroneous-paths-attribute
>>>             Detect paths that trigger erroneous or undefined behavior due to  a
>>>             null  value being used in a way forbidden by a "returns_nonnull" or
>>>             "nonnull" attribute.  Isolate those paths  from  the  main  control
>>>             flow  and  turn  the statement with erroneous or undefined behavior
>>>             into a trap.  This is not currently enabled, but may be enabled  by
>>>             -O2 in the future.
>> This is not a clear statement that the path will not be deleted as unreachable,
>> nor is it a promise never to do so in the future.
>
> It seems to me like such a promise as "we will never do this in the 
> future" can not possibly be made as it no one can be certain it will be 
> upheld forever.

Not at all.  GCC (the project) already makes exactly the type of promise I am looking
for with its guarantee that -g will not cause any changes to code generation, which
has been upheld ever since the 1980s.

zw
Xi Ruoyao Sept. 25, 2023, 3:14 p.m. UTC | #8 
On Mon, 2023-09-25 at 16:03 +0100, Gabriel Ravier wrote:
> On 9/25/23 15:30, Zack Weinberg wrote:
> > On Mon, Sep 25, 2023, at 10:29 AM, Xi Ruoyao wrote:
> > > On Mon, 2023-09-25 at 10:10 -0400, Zack Weinberg wrote:
> > > 
> > > > (It's probably fine if compilers treat calls that pass a NULL pointer to
> > > > a __nonnull argument as noreturn, or if they replace them with a trap
> > > > instruction or a call to abort().  The danger I foresee is from any
> > > > circumstance where NULL checks and side effects that happen *before* the
> > > > bad call can get deleted.)
> > > Regarding this, gcc man page says:
> > > 
> > >         -fisolate-erroneous-paths-attribute
> > >             Detect paths that trigger erroneous or undefined behavior due to  a
> > >             null  value being used in a way forbidden by a "returns_nonnull" or
> > >             "nonnull" attribute.  Isolate those paths  from  the  main  control
> > >             flow  and  turn  the statement with erroneous or undefined behavior
> > >             into a trap.  This is not currently enabled, but may be enabled  by
> > >             -O2 in the future.
> > This is not a clear statement that the path will not be deleted as unreachable,
> > nor is it a promise never to do so in the future.
> 
> It seems to me like such a promise as "we will never do this in the 
> future" can not possibly be made as it no one can be certain it will be 
> upheld forever. Whoever makes the promise (the "GCC developer team" ? 
> The FSF ? The GNU Project ?) cannot be forced to uphold that promise, 
> and in fact they could easily, over a long enough period of time, be 
> replaced (or have all the people constituting them replaced) by people
> who do not want to uphold the promise. By the standard you're trying to 
> enforce, glibc should never add any information or annotations of any 
> sort to its headers which could indicate to any compiler that any way of 
> calling any of its methods may result in undefined behavior.

The problem is simple, the GCC maintainers think people shouldn't invoke
undefined behavior, if someone does (s)he is just stupid and in no
position to demand anything.  But the Glibc maintainers don't agree.

Personally I agree with the GCC opinion that "if you invoke undefined
behavior then anything can happen" because I think valid code should not
pay any cost for satisfying invalid code.  But it's my personal feeling
and I don't really expect anyone to agree with me.

Anyway now I feel like "a mouse caught in a wind box", i.e. both sides
now consider me attempting to do something strange.  Now I just want to
get out of the wind box instead of helping anyone with anything.  So
please revert my previous commit
(71d9e0fe766a3c22a730995b9d024960970670af) and backport it to 2.38
release branch.  Then "defining __nonnull to nothing or not" is your own
business.  I just want to get out.
Paul Eggert Sept. 25, 2023, 5:42 p.m. UTC | #9 
On 2023-09-25 08:14, Xi Ruoyao wrote:
> Anyway now I feel like "a mouse caught in a wind box", i.e. both sides
> now consider me attempting to do something strange.

That's a colorful analogy, though not one I'm familiar with. (Googling 
for it, is this a wind box virus from the Mega Man Battle Network 
series, or some other kind of wind box? :-)

Please try to be patient. glibc is a big project with many competing 
concerns. We're often not hasty about changing, and let's also not be 
hasty about giving up.


> glibc refusing to add the annotation seems like it would just result in GCC eventually making it so that fprintf and other such functions are considered as builtins for the purposes of making the nonnull attribute is hard-coded and non-removeable, which would take things entirely out of the C library's control

Good point, and one I hadn't seen Zack address.
Zack Weinberg Sept. 25, 2023, 7:17 p.m. UTC | #10 
On Mon, Sep 25, 2023, at 1:42 PM, Paul Eggert wrote:
> On 2023-09-25 08:14, Xi Ruoyao wrote:
>> glibc refusing to add the annotation seems like it would just result
>> in GCC eventually making it so that fprintf and other such functions
>> are considered as builtins for the purposes of making the nonnull
>> attribute is hard-coded and non-removeable, which would take things
>> entirely out of the C library's control
>
> Good point, and one I hadn't seen Zack address.

Well, if that happens, then any problems are unambiguously laid at the
feet of the compiler and not glibc, but that's kind of a cop-out answer.

The *real* answer is that the thing I want here requires the active
cooperation of the compiler, and so, my objection to adding these
annotations should be seen as the opening move in an actual
negotiation with the compiler people.  If Xi Ruoyao does not wish to
participate, that's fine, there are plenty of other GCC devs being
cc:ed on this thread.

*Is* GCC (the project) willing to make a documented guarantee that gcc
(the compiler) does not now, and will not in the future, optimize based
on the presence of __attribute__((nonnull)) in certain ways which are
valid per the letter of the C standard but which have repeatedly been
demonstrated to cause serious problems for real codebases, up to and
including introduction of security holes where none would have existed
in a naive translation of the same source code?  I have stated what I
believe to be the most important example of such optimizations, but
really I'm looking for a commitment to the *principle* that "well,
the program has undefined behavior" does not trump "you broke this
existing codebase" anymore.

zw
enh Sept. 25, 2023, 7:28 p.m. UTC | #11 
On Mon, Sep 25, 2023 at 12:17 PM Zack Weinberg <zack@owlfolio.org> wrote:
>
> On Mon, Sep 25, 2023, at 1:42 PM, Paul Eggert wrote:
> > On 2023-09-25 08:14, Xi Ruoyao wrote:
> >> glibc refusing to add the annotation seems like it would just result
> >> in GCC eventually making it so that fprintf and other such functions
> >> are considered as builtins for the purposes of making the nonnull
> >> attribute is hard-coded and non-removeable, which would take things
> >> entirely out of the C library's control
> >
> > Good point, and one I hadn't seen Zack address.
>
> Well, if that happens, then any problems are unambiguously laid at the
> feet of the compiler and not glibc, but that's kind of a cop-out answer.
>
> The *real* answer is that the thing I want here requires the active
> cooperation of the compiler, and so, my objection to adding these
> annotations should be seen as the opening move in an actual
> negotiation with the compiler people.  If Xi Ruoyao does not wish to
> participate, that's fine, there are plenty of other GCC devs being
> cc:ed on this thread.
>
> *Is* GCC (the project) willing to make a documented guarantee that gcc
> (the compiler) does not now, and will not in the future, optimize based
> on the presence of __attribute__((nonnull)) in certain ways which are
> valid per the letter of the C standard but which have repeatedly been
> demonstrated to cause serious problems for real codebases, up to and
> including introduction of security holes where none would have existed
> in a naive translation of the same source code?

is this where the _new_ nullability attributes came from? a reaction
to that past mistake?

Android used the original nullability attributes until we found out
what the compiler was doing with that information, at which point we
ripped it all out until a way to express the semantics we actually
wanted came along. i don't know about GCC, but clang explicitly states
that the new annotations are what we want: "Note that, unlike the
declaration attribute nonnull, the presence of _Nonnull does not imply
that passing null is undefined behavior"
[https://clang.llvm.org/docs/AttributeReference.html#nonnull]

> I have stated what I
> believe to be the most important example of such optimizations, but
> really I'm looking for a commitment to the *principle* that "well,
> the program has undefined behavior" does not trump "you broke this
> existing codebase" anymore.
>
> zw
Xi Ruoyao Sept. 26, 2023, 7:39 a.m. UTC | #12 
On Mon, 2023-09-25 at 10:42 -0700, Paul Eggert wrote:
> On 2023-09-25 08:14, Xi Ruoyao wrote:
> > Anyway now I feel like "a mouse caught in a wind box", i.e. both sides
> > now consider me attempting to do something strange.
> 
> That's a colorful analogy, though not one I'm familiar with. (Googling
> for it, is this a wind box virus from the Mega Man Battle Network 
> series, or some other kind of wind box? :-)
> 
> Please try to be patient. glibc is a big project with many competing 
> concerns. We're often not hasty about changing, and let's also not be 
> hasty about giving up.

Maybe, but a short-term "fix" is just reverting my commit introduced
__nonnull to two functions.  It's very strange we only use __nonnull in
two functions but not the others in the same header.
Alejandro Colomar Sept. 26, 2023, 11:24 a.m. UTC | #13 
Hi,

On Tue, Sep 26, 2023 at 03:39:45PM +0800, Xi Ruoyao wrote:
> On Mon, 2023-09-25 at 10:42 -0700, Paul Eggert wrote:
> > On 2023-09-25 08:14, Xi Ruoyao wrote:
> > > Anyway now I feel like "a mouse caught in a wind box", i.e. both sides
> > > now consider me attempting to do something strange.
> > 
> > That's a colorful analogy, though not one I'm familiar with. (Googling
> > for it, is this a wind box virus from the Mega Man Battle Network 
> > series, or some other kind of wind box? :-)
> > 
> > Please try to be patient. glibc is a big project with many competing 
> > concerns. We're often not hasty about changing, and let's also not be 
> > hasty about giving up.
> 
> Maybe, but a short-term "fix" is just reverting my commit introduced
> __nonnull to two functions.  It's very strange we only use __nonnull in
> two functions but not the others in the same header.

This is the thing.  glibc extensively uses nonnull already.  This
discussion about UB is a spin-off that makes sense on its own, but
shouldn't be stopping this change, IMO; I think it should be addressed
separately, as a global change to either glibc or gcc (or both).

Adding nonnull (attribute) to a few functions that should have had it,
and don't have it (probably) because someone forgot, shouldn't be a
problem.  glibc already invokes UB in many libc functions if passed with
NULL.  A couple more shouldn't hurt.

Then, if we want to define the nonnull macro to empty under some
conditions, or want to change the nonnull attribute to some nullability
qualifier (e.g., Clang's _Nullable, or a hypothetical _Optional that
would go on the pointee), that's a separate change.

I think Xi is right in complaining that the discussion is conflating the
change of adding the missing attributes for consistency with the change
of maybe removing the attributes from glibc (under some conditions) or
maybe changing to a qualifier.

I don't think it makes sense to stop this change, and would like to see
it applied.

On the other hand, I think the discussion about UB is great, and I'm no
lover of the nonnull attribute (you may have noticed the _Nullable
qualifier in the manual pages recently added).  I'd like to discuss more
in that regard.  But do we need to block this patch for that?  We know
that correct code shouldn't break; only UB code might be broken by such
a change.  And how many functions do you really expect to do things like
fprintf(NULL, ...)?  I think it's close to none.

Cheers,
Alex

> 
> -- 
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University
Siddhesh Poyarekar Sept. 26, 2023, 11:24 a.m. UTC | #14 
On 2023-09-25 15:10, Zack Weinberg wrote:
> I still think this is a dangerous idea and, if we do it at all, we
> should do it *only* for compilers that have a *documented guarantee*
> that control flow paths that provably pass a NULL pointer to a __nonnull
> argument will *not* be treated as unreachable.  If I understood the

You're essentially asking the compiler to *define* a behaviour when 
faced with undefined behaviour, which doesn't seem like a reasonable 
request to me.  Likewise for glibc; we don't often go out of our way to 
specify an implementation defined behaviour when the standard specifies 
that behaviour as undefined.

> previous discussion correctly, GCC currently does not do that, but there
> is no documented guarantee that it will *never* do that, and nobody
> chimed in from LLVM's side.
> 
> (It's probably fine if compilers treat calls that pass a NULL pointer to
> a __nonnull argument as noreturn, or if they replace them with a trap
> instruction or a call to abort().  The danger I foresee is from any
> circumstance where NULL checks and side effects that happen *before* the
> bad call can get deleted.)

This (traps on undefined behaviour) was in fact discussed at the GNU 
Tools Cauldron last week as one of the things that the compiler could do 
for undefined behaviour in general, essentially subsuming sanitizers 
into the compiler proper.  One must realize though that (1) this would 
be 'best effort' on behalf of the compiler and (2) it's going to have 
overheads, albeit modern, speculating processors may subsume that 
overhead, which however is precisely where Spectre-like flaws operate...

Trying to make undefined behaviour safe in the compiler is not a bad 
idea at all, but guaranteeing fixes to application bugs in the compiler 
is a terrible idea IMO.

> (I think this policy should be applied to *all* instances of __nonnull
> in glibc's headers, not just new ones.)

You're asking for a change in position in glibc, which shouldn't block 
this current patch IMO, which aligns with the current position in glibc.

Thanks,
Sid
Alejandro Colomar Sept. 26, 2023, 12:05 p.m. UTC | #15 
On Tue, Sep 26, 2023 at 07:24:37AM -0400, Siddhesh Poyarekar wrote:
> On 2023-09-25 15:10, Zack Weinberg wrote:
> > I still think this is a dangerous idea and, if we do it at all, we
> > should do it *only* for compilers that have a *documented guarantee*
> > that control flow paths that provably pass a NULL pointer to a __nonnull
> > argument will *not* be treated as unreachable.  If I understood the
> 
> You're essentially asking the compiler to *define* a behaviour when faced
> with undefined behaviour, which doesn't seem like a reasonable request to
> me.  Likewise for glibc; we don't often go out of our way to specify an
> implementation defined behaviour when the standard specifies that behaviour
> as undefined.
> 
> > previous discussion correctly, GCC currently does not do that, but there
> > is no documented guarantee that it will *never* do that, and nobody
> > chimed in from LLVM's side.
> > 
> > (It's probably fine if compilers treat calls that pass a NULL pointer to
> > a __nonnull argument as noreturn, or if they replace them with a trap
> > instruction or a call to abort().  The danger I foresee is from any
> > circumstance where NULL checks and side effects that happen *before* the
> > bad call can get deleted.)
> 
> This (traps on undefined behaviour) was in fact discussed at the GNU Tools
> Cauldron last week as one of the things that the compiler could do for
> undefined behaviour in general, essentially subsuming sanitizers into the
> compiler proper.  One must realize though that (1) this would be 'best
> effort' on behalf of the compiler and (2) it's going to have overheads,
> albeit modern, speculating processors may subsume that overhead, which
> however is precisely where Spectre-like flaws operate...
> 
> Trying to make undefined behaviour safe in the compiler is not a bad idea at
> all, but guaranteeing fixes to application bugs in the compiler is a
> terrible idea IMO.
> 
> > (I think this policy should be applied to *all* instances of __nonnull
> > in glibc's headers, not just new ones.)
> 
> You're asking for a change in position in glibc, which shouldn't block this
> current patch IMO, which aligns with the current position in glibc.

I tend to agree with the GCC side of things.  UB is UB.  Making it safe
at all costs is not necessarily a good thing.  What I think could be
done is using a qualifier, similar to const / restrict, to be able to 
statically analize the code for UB.  Still, I don't think this patch
should be blocked by such a discussion, which would probably take a long
time.

Cheers,
Alex

> 
> Thanks,
> Sid
Zack Weinberg Sept. 26, 2023, 9:01 p.m. UTC | #16 
On Tue, Sep 26, 2023, at 7:24 AM, Siddhesh Poyarekar wrote:
> On 2023-09-25 15:10, Zack Weinberg wrote:
>> I still think this is a dangerous idea and, if we do it at all, we
>> should do it *only* for compilers that have a *documented guarantee*
>> that control flow paths that provably pass a NULL pointer to a __nonnull
>> argument will *not* be treated as unreachable.  If I understood the
>
> You're essentially asking the compiler to *define* a behaviour when 
> faced with undefined behaviour, which doesn't seem like a reasonable 
> request to me.  Likewise for glibc; we don't often go out of our way to 
> specify an implementation defined behaviour when the standard specifies 
> that behaviour as undefined.

I don't think I'm asking for something nearly that strong.  I'm not trying
to argue that a program that passes a null pointer to a nonnull-annotated
library function argument isn't an incorrect program, nor am I saying that
the compiler shouldn't be allowed to make *any* optimizations that penalize
such programs.

I said at the beginning of this thread that what I want is a guarantee that,
if the compiler determines that a particular control flow path will
definitely pass a null pointer to a nonnull-annotated library function
argument, it will not treat that control flow path as unreachable.  This
places *limits* on what the compiler can do with a program containing
this particular type of undefined behavior, but it doesn't pin things down
nearly enough to call it _defining_ the behavior.  I consider it more like
the limits CPU designers tend to document on what the hardware will do in
the face of misuse of the ISA (for instance ARM calls this "boundedly
unpredictable" behavior and is quite specific about what can or cannot
happen).

And I have concrete reasons to be concerned.  Deletion of control flow
paths leading into NULL dereferences has been known to convert crash bugs
into exploitable security vulnerabilities -- for a concrete example see
https://bugs.chromium.org/p/chromium/issues/detail?id=1133635 and
https://chromium-review.googlesource.com/c/chromium/src/+/2463857
(it's a little hard to see what's going on, but I believe the source
code was effectively

  func_ptr = allocation_still_live() ? heap_address : NULL;
  func_ptr();

and the compiler deleted the check for whether the allocation was still
live, turning a null dereference into an exploitable use-after-free.)

>> (I think this policy should be applied to *all* instances of __nonnull
>> in glibc's headers, not just new ones.)
>
> You're asking for a change in position in glibc, which shouldn't block 
> this current patch IMO, which aligns with the current position in glibc.

That's fair.  I don't *like* it, because every *new* introduction of __nonnull
creates *new* opportunities to transform programs dangerously as seen above,
but I can live with it early in a development cycle, particularly if we
keep discussing the larger principle and hopefully come to a consensus on
when these annotations should be active versus just documentation.

zw
Siddhesh Poyarekar Sept. 26, 2023, 10:28 p.m. UTC | #17 
On 2023-09-26 22:01, Zack Weinberg wrote:
> On Tue, Sep 26, 2023, at 7:24 AM, Siddhesh Poyarekar wrote:
>> On 2023-09-25 15:10, Zack Weinberg wrote:
>>> I still think this is a dangerous idea and, if we do it at all, we
>>> should do it *only* for compilers that have a *documented guarantee*
>>> that control flow paths that provably pass a NULL pointer to a __nonnull
>>> argument will *not* be treated as unreachable.  If I understood the
>>
>> You're essentially asking the compiler to *define* a behaviour when
>> faced with undefined behaviour, which doesn't seem like a reasonable
>> request to me.  Likewise for glibc; we don't often go out of our way to
>> specify an implementation defined behaviour when the standard specifies
>> that behaviour as undefined.
> 
> I don't think I'm asking for something nearly that strong.  I'm not trying
> to argue that a program that passes a null pointer to a nonnull-annotated
> library function argument isn't an incorrect program, nor am I saying that
> the compiler shouldn't be allowed to make *any* optimizations that penalize
> such programs.
> 
> I said at the beginning of this thread that what I want is a guarantee that,
> if the compiler determines that a particular control flow path will
> definitely pass a null pointer to a nonnull-annotated library function
> argument, it will not treat that control flow path as unreachable.  This
> places *limits* on what the compiler can do with a program containing
> this particular type of undefined behavior, but it doesn't pin things down
> nearly enough to call it _defining_ the behavior.  I consider it more like
> the limits CPU designers tend to document on what the hardware will do in
> the face of misuse of the ISA (for instance ARM calls this "boundedly
> unpredictable" behavior and is quite specific about what can or cannot
> happen).
> 
> And I have concrete reasons to be concerned.  Deletion of control flow
> paths leading into NULL dereferences has been known to convert crash bugs
> into exploitable security vulnerabilities -- for a concrete example see
> https://bugs.chromium.org/p/chromium/issues/detail?id=1133635 and
> https://chromium-review.googlesource.com/c/chromium/src/+/2463857
> (it's a little hard to see what's going on, but I believe the source
> code was effectively
> 
>    func_ptr = allocation_still_live() ? heap_address : NULL;
>    func_ptr();
> 
> and the compiler deleted the check for whether the allocation was still
> live, turning a null dereference into an exploitable use-after-free.)

Ack, this is something I haven't been able to reproduce on recent 
versions of gcc, but it probably needs closer inspection.  I looked at 
the previous thread again and realized that I may have read your current 
objection too strongly; you're only talking about backward propagation 
of the __nonnull attribute being aggressive.  Is that correct?

I guess my core concern is the *guarantee* aspect of it; it feels like a 
shot in the foot for the compiler to guarantee (and hence specify) 
anything when faced with undefined behaviour.  It would be reasonable 
(and very desirable) to try and ensure that gcc always avoids backward 
propagation (and have tests to ensure it and even accept bug reports for 
any failure to do so) but making it a guarantee amounts to making any 
such failure a vulnerability in the compiler because it escalates what 
could potentially have been a DoS into a UAF.

>>> (I think this policy should be applied to *all* instances of __nonnull
>>> in glibc's headers, not just new ones.)
>>
>> You're asking for a change in position in glibc, which shouldn't block
>> this current patch IMO, which aligns with the current position in glibc.
> 
> That's fair.  I don't *like* it, because every *new* introduction of __nonnull
> creates *new* opportunities to transform programs dangerously as seen above,
> but I can live with it early in a development cycle, particularly if we
> keep discussing the larger principle and hopefully come to a consensus on
> when these annotations should be active versus just documentation.

Thanks, I'll review the patch then.  I wonder how we should continue 
this specific thread of discussion though.  glibc bugzilla seems like 
the wrong place for it, maybe gcc is a better place.  I wasn't able to 
make a simple enough test case for it the last time I had tried but if 
someone has such a test case then filing a gcc bug would be the best 
course of action.

Thanks,
Sid
Siddhesh Poyarekar Sept. 26, 2023, 11 p.m. UTC | #18 
On 2023-09-25 12:53, Xi Ruoyao wrote:
> During the review of a GCC analyzer test case, we found most stdio
> functions accepting a FILE * argument expect it to be nonnull and just
> segfault when the argument is NULL.  Add nonnull attribute for them.
> 
> fflush and fflush_unlocked are well defined when __stream is NULL so
> they are not touched.
> 
> For fputs, fgets, fread, fwrite, fprintf, vfprintf, and their unlocked
> version, if __stream is empty but there is nothing to read or write,
> they did not segfault.  But the standard disallow __stream to be empty
> here, so nonnull attribute is also added for them.  Note that this may
> blow up some old code already subtly broken.
> 
> Also add __nonnull for _chk variants and __fortify_function versions for
> them.
> 
> Signed-off-by: Xi Ruoyao <xry111@xry111.site>

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

> ---
> 
> No change since v6, resubmit for today's patch review.
> 
> IIRC our conclusion in 2.38 dev cycle was:
> 
> 1. If this may be acceptable, we'll apply it early in 2.39 dev cycle
>     and see if things goes well.
> 2. If this is not acceptable at all, we'll revert commit
>     71d9e0fe766a3c22a730995b9d024960970670af.
> 
> Now it's the end of the second month in 2.39 dev cycle, so let's make a
> decision.
> 
>   libio/bits/stdio2-decl.h |  15 ++--
>   libio/bits/stdio2.h      |  14 ++--
>   libio/stdio.h            | 148 ++++++++++++++++++++++-----------------
>   3 files changed, 99 insertions(+), 78 deletions(-)
> 
> diff --git a/libio/bits/stdio2-decl.h b/libio/bits/stdio2-decl.h
> index d7ef7283d6..3b60c72931 100644
> --- a/libio/bits/stdio2-decl.h
> +++ b/libio/bits/stdio2-decl.h
> @@ -47,10 +47,12 @@ extern int __vsnprintf_chk (char *__restrict __s, size_t __n, int __flag,
>   #if __USE_FORTIFY_LEVEL > 1
>   
>   extern int __fprintf_chk (FILE *__restrict __stream, int __flag,
> -			  const char *__restrict __format, ...);
> +			  const char *__restrict __format, ...)
> +    __nonnull ((1));
>   extern int __printf_chk (int __flag, const char *__restrict __format, ...);
>   extern int __vfprintf_chk (FILE *__restrict __stream, int __flag,
> -			   const char *__restrict __format, __gnuc_va_list __ap);
> +			   const char *__restrict __format,
> +			   __gnuc_va_list __ap) __nonnull ((1));
>   extern int __vprintf_chk (int __flag, const char *__restrict __format,
>   			  __gnuc_va_list __ap);
>   
> @@ -103,7 +105,7 @@ extern char *__REDIRECT (__fgets_chk_warn,
>   
>   extern char *__fgets_chk (char *__restrict __s, size_t __size, int __n,
>   			  FILE *__restrict __stream)
> -    __wur __attr_access ((__write_only__, 1, 3));
> +    __wur __attr_access ((__write_only__, 1, 3)) __nonnull ((4));
>   
>   extern size_t __REDIRECT (__fread_alias,
>   			  (void *__restrict __ptr, size_t __size,
> @@ -119,7 +121,7 @@ extern size_t __REDIRECT (__fread_chk_warn,
>   
>   extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen,
>   			   size_t __size, size_t __n,
> -			   FILE *__restrict __stream) __wur;
> +			   FILE *__restrict __stream) __wur __nonnull ((5));
>   
>   #ifdef __USE_GNU
>   extern char *__REDIRECT_FORTIFY (__fgets_unlocked_alias,
> @@ -135,7 +137,7 @@ extern char *__REDIRECT (__fgets_unlocked_chk_warn,
>   
>   extern char *__fgets_unlocked_chk (char *__restrict __s, size_t __size,
>   				   int __n, FILE *__restrict __stream)
> -    __wur __attr_access ((__write_only__, 1, 3));
> +    __wur __attr_access ((__write_only__, 1, 3)) __nonnull ((4));
>   #endif
>   
>   #ifdef __USE_MISC
> @@ -154,7 +156,8 @@ extern size_t __REDIRECT (__fread_unlocked_chk_warn,
>   
>   extern size_t __fread_unlocked_chk (void *__restrict __ptr, size_t __ptrlen,
>   				    size_t __size, size_t __n,
> -				    FILE *__restrict __stream) __wur;
> +				    FILE *__restrict __stream)
> +    __wur __nonnull ((5));
>   #endif
>   
>   #endif /* bits/stdio2-decl.h.  */
> diff --git a/libio/bits/stdio2.h b/libio/bits/stdio2.h
> index 71226408ab..266dccdd1e 100644
> --- a/libio/bits/stdio2.h
> +++ b/libio/bits/stdio2.h
> @@ -73,7 +73,7 @@ __NTH (vsnprintf (char *__restrict __s, size_t __n,
>   
>   #if __USE_FORTIFY_LEVEL > 1
>   # ifdef __va_arg_pack
> -__fortify_function int
> +__fortify_function __nonnull ((1)) int
>   fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
>   {
>     return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
> @@ -102,7 +102,7 @@ vprintf (const char *__restrict __fmt, __gnuc_va_list __ap)
>   #endif
>   }
>   
> -__fortify_function int
> +__fortify_function __nonnull ((1)) int
>   vfprintf (FILE *__restrict __stream,
>   	  const char *__restrict __fmt, __gnuc_va_list __ap)
>   {
> @@ -191,7 +191,8 @@ gets (char *__str)
>   }
>   #endif
>   
> -__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2) char *
> +__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2)
> +__nonnull ((3)) char *
>   fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
>   {
>     size_t sz = __glibc_objsize (__s);
> @@ -202,7 +203,7 @@ fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
>     return __fgets_chk (__s, sz, __n, __stream);
>   }
>   
> -__fortify_function __wur size_t
> +__fortify_function __wur __nonnull ((4)) size_t
>   fread (void *__restrict __ptr, size_t __size, size_t __n,
>          FILE *__restrict __stream)
>   {
> @@ -215,7 +216,8 @@ fread (void *__restrict __ptr, size_t __size, size_t __n,
>   }
>   
>   #ifdef __USE_GNU
> -__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2) char *
> +__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2)
> +__nonnull ((3)) char *
>   fgets_unlocked (char *__restrict __s, int __n, FILE *__restrict __stream)
>   {
>     size_t sz = __glibc_objsize (__s);
> @@ -229,7 +231,7 @@ fgets_unlocked (char *__restrict __s, int __n, FILE *__restrict __stream)
>   
>   #ifdef __USE_MISC
>   # undef fread_unlocked
> -__fortify_function __wur size_t
> +__fortify_function __wur __nonnull ((4)) size_t
>   fread_unlocked (void *__restrict __ptr, size_t __size, size_t __n,
>   		FILE *__restrict __stream)
>   {
> diff --git a/libio/stdio.h b/libio/stdio.h
> index 4cf9f1c012..a640e9beb5 100644
> --- a/libio/stdio.h
> +++ b/libio/stdio.h
> @@ -1,5 +1,6 @@
>   /* Define ISO C stdio on top of C++ iostreams.
>      Copyright (C) 1991-2023 Free Software Foundation, Inc.
> +   Copyright The GNU Toolchain Authors.
>      This file is part of the GNU C Library.
>   
>      The GNU C Library is free software; you can redistribute it and/or
> @@ -278,7 +279,7 @@ extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
>   extern FILE *__REDIRECT (freopen, (const char *__restrict __filename,
>   				   const char *__restrict __modes,
>   				   FILE *__restrict __stream), freopen64)
> -  __wur;
> +  __wur __nonnull ((3));
>   # else
>   #  define fopen fopen64
>   #  define freopen freopen64
> @@ -330,21 +331,22 @@ extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t *__sizeloc) __THROW
>   
>   /* If BUF is NULL, make STREAM unbuffered.
>      Else make it use buffer BUF, of size BUFSIZ.  */
> -extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW;
> +extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW
> +  __nonnull ((1));
>   /* Make STREAM use buffering mode MODE.
>      If BUF is not NULL, use N bytes of it for buffering;
>      else allocate an internal buffer N bytes long.  */
>   extern int setvbuf (FILE *__restrict __stream, char *__restrict __buf,
> -		    int __modes, size_t __n) __THROW;
> +		    int __modes, size_t __n) __THROW __nonnull ((1));
>   
>   #ifdef	__USE_MISC
>   /* If BUF is NULL, make STREAM unbuffered.
>      Else make it use SIZE bytes of BUF for buffering.  */
>   extern void setbuffer (FILE *__restrict __stream, char *__restrict __buf,
> -		       size_t __size) __THROW;
> +		       size_t __size) __THROW __nonnull ((1));
>   
>   /* Make STREAM line-buffered.  */
> -extern void setlinebuf (FILE *__stream) __THROW;
> +extern void setlinebuf (FILE *__stream) __THROW __nonnull ((1));
>   #endif
>   
>   
> @@ -353,7 +355,7 @@ extern void setlinebuf (FILE *__stream) __THROW;
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
>   extern int fprintf (FILE *__restrict __stream,
> -		    const char *__restrict __format, ...);
> +		    const char *__restrict __format, ...) __nonnull ((1));
>   /* Write formatted output to stdout.
>   
>      This function is a possible cancellation point and therefore not
> @@ -368,7 +370,7 @@ extern int sprintf (char *__restrict __s,
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
>   extern int vfprintf (FILE *__restrict __s, const char *__restrict __format,
> -		     __gnuc_va_list __arg);
> +		     __gnuc_va_list __arg) __nonnull ((1));
>   /* Write formatted output to stdout from argument list ARG.
>   
>      This function is a possible cancellation point and therefore not
> @@ -418,7 +420,7 @@ extern int dprintf (int __fd, const char *__restrict __fmt, ...)
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
>   extern int fscanf (FILE *__restrict __stream,
> -		   const char *__restrict __format, ...) __wur;
> +		   const char *__restrict __format, ...) __wur __nonnull ((1));
>   /* Read formatted input from stdin.
>   
>      This function is a possible cancellation point and therefore not
> @@ -439,7 +441,7 @@ extern int sscanf (const char *__restrict __s,
>   #  ifdef __REDIRECT
>   extern int __REDIRECT (fscanf, (FILE *__restrict __stream,
>   				const char *__restrict __format, ...),
> -		       __isoc23_fscanf) __wur;
> +		       __isoc23_fscanf) __wur __nonnull ((1));
>   extern int __REDIRECT (scanf, (const char *__restrict __format, ...),
>   		       __isoc23_scanf) __wur;
>   extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
> @@ -447,7 +449,8 @@ extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
>   			   __isoc23_sscanf);
>   #  else
>   extern int __isoc23_fscanf (FILE *__restrict __stream,
> -			    const char *__restrict __format, ...) __wur;
> +			    const char *__restrict __format, ...) __wur
> +  __nonnull ((1));
>   extern int __isoc23_scanf (const char *__restrict __format, ...) __wur;
>   extern int __isoc23_sscanf (const char *__restrict __s,
>   			    const char *__restrict __format, ...) __THROW;
> @@ -459,7 +462,7 @@ extern int __isoc23_sscanf (const char *__restrict __s,
>   #  ifdef __REDIRECT
>   extern int __REDIRECT (fscanf, (FILE *__restrict __stream,
>   				const char *__restrict __format, ...),
> -		       __isoc99_fscanf) __wur;
> +		       __isoc99_fscanf) __wur __nonnull ((1));
>   extern int __REDIRECT (scanf, (const char *__restrict __format, ...),
>   		       __isoc99_scanf) __wur;
>   extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
> @@ -467,7 +470,8 @@ extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
>   			   __isoc99_sscanf);
>   #  else
>   extern int __isoc99_fscanf (FILE *__restrict __stream,
> -			    const char *__restrict __format, ...) __wur;
> +			    const char *__restrict __format, ...) __wur
> +  __nonnull ((1));
>   extern int __isoc99_scanf (const char *__restrict __format, ...) __wur;
>   extern int __isoc99_sscanf (const char *__restrict __s,
>   			    const char *__restrict __format, ...) __THROW;
> @@ -485,7 +489,7 @@ extern int __isoc99_sscanf (const char *__restrict __s,
>      marked with __THROW.  */
>   extern int vfscanf (FILE *__restrict __s, const char *__restrict __format,
>   		    __gnuc_va_list __arg)
> -     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
> +     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
>   
>   /* Read formatted input from stdin into argument list ARG.
>   
> @@ -508,7 +512,7 @@ extern int __REDIRECT (vfscanf,
>   		       (FILE *__restrict __s,
>   			const char *__restrict __format, __gnuc_va_list __arg),
>   		       __isoc23_vfscanf)
> -     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
> +     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
>   extern int __REDIRECT (vscanf, (const char *__restrict __format,
>   				__gnuc_va_list __arg), __isoc23_vscanf)
>        __attribute__ ((__format__ (__scanf__, 1, 0))) __wur;
> @@ -520,7 +524,7 @@ extern int __REDIRECT_NTH (vsscanf,
>   #   elif !defined __REDIRECT
>   extern int __isoc23_vfscanf (FILE *__restrict __s,
>   			     const char *__restrict __format,
> -			     __gnuc_va_list __arg) __wur;
> +			     __gnuc_va_list __arg) __wur __nonnull ((1));
>   extern int __isoc23_vscanf (const char *__restrict __format,
>   			    __gnuc_va_list __arg) __wur;
>   extern int __isoc23_vsscanf (const char *__restrict __s,
> @@ -537,7 +541,7 @@ extern int __REDIRECT (vfscanf,
>   		       (FILE *__restrict __s,
>   			const char *__restrict __format, __gnuc_va_list __arg),
>   		       __isoc99_vfscanf)
> -     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
> +     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
>   extern int __REDIRECT (vscanf, (const char *__restrict __format,
>   				__gnuc_va_list __arg), __isoc99_vscanf)
>        __attribute__ ((__format__ (__scanf__, 1, 0))) __wur;
> @@ -549,7 +553,7 @@ extern int __REDIRECT_NTH (vsscanf,
>   #   elif !defined __REDIRECT
>   extern int __isoc99_vfscanf (FILE *__restrict __s,
>   			     const char *__restrict __format,
> -			     __gnuc_va_list __arg) __wur;
> +			     __gnuc_va_list __arg) __wur __nonnull ((1));
>   extern int __isoc99_vscanf (const char *__restrict __format,
>   			    __gnuc_va_list __arg) __wur;
>   extern int __isoc99_vsscanf (const char *__restrict __s,
> @@ -568,8 +572,8 @@ extern int __isoc99_vsscanf (const char *__restrict __s,
>   
>      These functions are possible cancellation points and therefore not
>      marked with __THROW.  */
> -extern int fgetc (FILE *__stream);
> -extern int getc (FILE *__stream);
> +extern int fgetc (FILE *__stream) __nonnull ((1));
> +extern int getc (FILE *__stream) __nonnull ((1));
>   
>   /* Read a character from stdin.
>   
> @@ -582,7 +586,7 @@ extern int getchar (void);
>   
>      These functions are possible cancellation points and therefore not
>      marked with __THROW.  */
> -extern int getc_unlocked (FILE *__stream);
> +extern int getc_unlocked (FILE *__stream) __nonnull ((1));
>   extern int getchar_unlocked (void);
>   #endif /* Use POSIX.  */
>   
> @@ -593,7 +597,7 @@ extern int getchar_unlocked (void);
>      cancellation point.  But due to similarity with an POSIX interface
>      or due to the implementation it is a cancellation point and
>      therefore not marked with __THROW.  */
> -extern int fgetc_unlocked (FILE *__stream);
> +extern int fgetc_unlocked (FILE *__stream) __nonnull ((1));
>   #endif /* Use MISC.  */
>   
>   
> @@ -604,8 +608,8 @@ extern int fgetc_unlocked (FILE *__stream);
>   
>      These functions is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int fputc (int __c, FILE *__stream);
> -extern int putc (int __c, FILE *__stream);
> +extern int fputc (int __c, FILE *__stream) __nonnull ((2));
> +extern int putc (int __c, FILE *__stream) __nonnull ((2));
>   
>   /* Write a character to stdout.
>   
> @@ -620,7 +624,7 @@ extern int putchar (int __c);
>      cancellation point.  But due to similarity with an POSIX interface
>      or due to the implementation it is a cancellation point and
>      therefore not marked with __THROW.  */
> -extern int fputc_unlocked (int __c, FILE *__stream);
> +extern int fputc_unlocked (int __c, FILE *__stream) __nonnull ((2));
>   #endif /* Use MISC.  */
>   
>   #ifdef __USE_POSIX199506
> @@ -628,7 +632,7 @@ extern int fputc_unlocked (int __c, FILE *__stream);
>   
>      These functions are possible cancellation points and therefore not
>      marked with __THROW.  */
> -extern int putc_unlocked (int __c, FILE *__stream);
> +extern int putc_unlocked (int __c, FILE *__stream) __nonnull ((2));
>   extern int putchar_unlocked (int __c);
>   #endif /* Use POSIX.  */
>   
> @@ -636,10 +640,10 @@ extern int putchar_unlocked (int __c);
>   #if defined __USE_MISC \
>       || (defined __USE_XOPEN && !defined __USE_XOPEN2K)
>   /* Get a word (int) from STREAM.  */
> -extern int getw (FILE *__stream);
> +extern int getw (FILE *__stream) __nonnull ((1));
>   
>   /* Write a word (int) to STREAM.  */
> -extern int putw (int __w, FILE *__stream);
> +extern int putw (int __w, FILE *__stream) __nonnull ((2));
>   #endif
>   
>   
> @@ -648,7 +652,7 @@ extern int putw (int __w, FILE *__stream);
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
>   extern char *fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
> -     __wur __fortified_attr_access (__write_only__, 1, 2);
> +     __wur __fortified_attr_access (__write_only__, 1, 2) __nonnull ((3));
>   
>   #if __GLIBC_USE (DEPRECATED_GETS)
>   /* Get a newline-terminated string from stdin, removing the newline.
> @@ -672,7 +676,7 @@ extern char *gets (char *__s) __wur __attribute_deprecated__;
>      therefore not marked with __THROW.  */
>   extern char *fgets_unlocked (char *__restrict __s, int __n,
>   			     FILE *__restrict __stream) __wur
> -    __fortified_attr_access (__write_only__, 1, 2);
> +    __fortified_attr_access (__write_only__, 1, 2) __nonnull ((3));
>   #endif
>   
>   
> @@ -689,10 +693,10 @@ extern char *fgets_unlocked (char *__restrict __s, int __n,
>      therefore not marked with __THROW.  */
>   extern __ssize_t __getdelim (char **__restrict __lineptr,
>                                size_t *__restrict __n, int __delimiter,
> -                             FILE *__restrict __stream) __wur;
> +                             FILE *__restrict __stream) __wur __nonnull ((4));
>   extern __ssize_t getdelim (char **__restrict __lineptr,
>                              size_t *__restrict __n, int __delimiter,
> -                           FILE *__restrict __stream) __wur;
> +                           FILE *__restrict __stream) __wur __nonnull ((4));
>   
>   /* Like `getdelim', but reads up to a newline.
>   
> @@ -702,7 +706,7 @@ extern __ssize_t getdelim (char **__restrict __lineptr,
>      therefore not marked with __THROW.  */
>   extern __ssize_t getline (char **__restrict __lineptr,
>                             size_t *__restrict __n,
> -                          FILE *__restrict __stream) __wur;
> +                          FILE *__restrict __stream) __wur __nonnull ((3));
>   #endif
>   
>   
> @@ -710,7 +714,8 @@ extern __ssize_t getline (char **__restrict __lineptr,
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int fputs (const char *__restrict __s, FILE *__restrict __stream);
> +extern int fputs (const char *__restrict __s, FILE *__restrict __stream)
> +  __nonnull ((2));
>   
>   /* Write a string, followed by a newline, to stdout.
>   
> @@ -723,7 +728,7 @@ extern int puts (const char *__s);
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int ungetc (int __c, FILE *__stream);
> +extern int ungetc (int __c, FILE *__stream) __nonnull ((2));
>   
>   
>   /* Read chunks of generic data from STREAM.
> @@ -731,13 +736,14 @@ extern int ungetc (int __c, FILE *__stream);
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
>   extern size_t fread (void *__restrict __ptr, size_t __size,
> -		     size_t __n, FILE *__restrict __stream) __wur;
> +		     size_t __n, FILE *__restrict __stream) __wur
> +  __nonnull((4));
>   /* Write chunks of generic data to STREAM.
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
>   extern size_t fwrite (const void *__restrict __ptr, size_t __size,
> -		      size_t __n, FILE *__restrict __s);
> +		      size_t __n, FILE *__restrict __s) __nonnull((4));
>   
>   #ifdef __USE_GNU
>   /* This function does the same as `fputs' but does not lock the stream.
> @@ -747,7 +753,7 @@ extern size_t fwrite (const void *__restrict __ptr, size_t __size,
>      or due to the implementation it is a cancellation point and
>      therefore not marked with __THROW.  */
>   extern int fputs_unlocked (const char *__restrict __s,
> -			   FILE *__restrict __stream);
> +			   FILE *__restrict __stream) __nonnull ((2));
>   #endif
>   
>   #ifdef __USE_MISC
> @@ -758,9 +764,11 @@ extern int fputs_unlocked (const char *__restrict __s,
>      or due to the implementation they are cancellation points and
>      therefore not marked with __THROW.  */
>   extern size_t fread_unlocked (void *__restrict __ptr, size_t __size,
> -			      size_t __n, FILE *__restrict __stream) __wur;
> +			      size_t __n, FILE *__restrict __stream) __wur
> +  __nonnull ((4));
>   extern size_t fwrite_unlocked (const void *__restrict __ptr, size_t __size,
> -			       size_t __n, FILE *__restrict __stream);
> +			       size_t __n, FILE *__restrict __stream)
> +  __nonnull ((4));
>   #endif
>   
>   
> @@ -768,17 +776,18 @@ extern size_t fwrite_unlocked (const void *__restrict __ptr, size_t __size,
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int fseek (FILE *__stream, long int __off, int __whence);
> +extern int fseek (FILE *__stream, long int __off, int __whence)
> +  __nonnull ((1));
>   /* Return the current position of STREAM.
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern long int ftell (FILE *__stream) __wur;
> +extern long int ftell (FILE *__stream) __wur __nonnull ((1));
>   /* Rewind to the beginning of STREAM.
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern void rewind (FILE *__stream);
> +extern void rewind (FILE *__stream) __nonnull ((1));
>   
>   /* The Single Unix Specification, Version 2, specifies an alternative,
>      more adequate interface for the two functions above which deal with
> @@ -791,18 +800,20 @@ extern void rewind (FILE *__stream);
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int fseeko (FILE *__stream, __off_t __off, int __whence);
> +extern int fseeko (FILE *__stream, __off_t __off, int __whence)
> +  __nonnull ((1));
>   /* Return the current position of STREAM.
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern __off_t ftello (FILE *__stream) __wur;
> +extern __off_t ftello (FILE *__stream) __wur __nonnull ((1));
>   # else
>   #  ifdef __REDIRECT
>   extern int __REDIRECT (fseeko,
>   		       (FILE *__stream, __off64_t __off, int __whence),
> -		       fseeko64);
> -extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64);
> +		       fseeko64) __nonnull ((1));
> +extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64)
> +  __nonnull ((1));
>   #  else
>   #   define fseeko fseeko64
>   #   define ftello ftello64
> @@ -815,18 +826,21 @@ extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64);
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int fgetpos (FILE *__restrict __stream, fpos_t *__restrict __pos);
> +extern int fgetpos (FILE *__restrict __stream, fpos_t *__restrict __pos)
> +  __nonnull ((1));
>   /* Set STREAM's position.
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int fsetpos (FILE *__stream, const fpos_t *__pos);
> +extern int fsetpos (FILE *__stream, const fpos_t *__pos) __nonnull ((1));
>   #else
>   # ifdef __REDIRECT
>   extern int __REDIRECT (fgetpos, (FILE *__restrict __stream,
> -				 fpos_t *__restrict __pos), fgetpos64);
> +				 fpos_t *__restrict __pos), fgetpos64)
> +  __nonnull ((1));
>   extern int __REDIRECT (fsetpos,
> -		       (FILE *__stream, const fpos_t *__pos), fsetpos64);
> +		       (FILE *__stream, const fpos_t *__pos), fsetpos64)
> +  __nonnull ((1));
>   # else
>   #  define fgetpos fgetpos64
>   #  define fsetpos fsetpos64
> @@ -834,24 +848,26 @@ extern int __REDIRECT (fsetpos,
>   #endif
>   
>   #ifdef __USE_LARGEFILE64
> -extern int fseeko64 (FILE *__stream, __off64_t __off, int __whence);
> -extern __off64_t ftello64 (FILE *__stream) __wur;
> -extern int fgetpos64 (FILE *__restrict __stream, fpos64_t *__restrict __pos);
> -extern int fsetpos64 (FILE *__stream, const fpos64_t *__pos);
> +extern int fseeko64 (FILE *__stream, __off64_t __off, int __whence)
> +  __nonnull ((1));
> +extern __off64_t ftello64 (FILE *__stream) __wur __nonnull ((1));
> +extern int fgetpos64 (FILE *__restrict __stream, fpos64_t *__restrict __pos)
> +  __nonnull ((1));
> +extern int fsetpos64 (FILE *__stream, const fpos64_t *__pos) __nonnull ((1));
>   #endif
>   
>   /* Clear the error and EOF indicators for STREAM.  */
> -extern void clearerr (FILE *__stream) __THROW;
> +extern void clearerr (FILE *__stream) __THROW __nonnull ((1));
>   /* Return the EOF indicator for STREAM.  */
> -extern int feof (FILE *__stream) __THROW __wur;
> +extern int feof (FILE *__stream) __THROW __wur __nonnull ((1));
>   /* Return the error indicator for STREAM.  */
> -extern int ferror (FILE *__stream) __THROW __wur;
> +extern int ferror (FILE *__stream) __THROW __wur __nonnull ((1));
>   
>   #ifdef __USE_MISC
>   /* Faster versions when locking is not required.  */
> -extern void clearerr_unlocked (FILE *__stream) __THROW;
> -extern int feof_unlocked (FILE *__stream) __THROW __wur;
> -extern int ferror_unlocked (FILE *__stream) __THROW __wur;
> +extern void clearerr_unlocked (FILE *__stream) __THROW __nonnull ((1));
> +extern int feof_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
> +extern int ferror_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
>   #endif
>   
>   
> @@ -864,12 +880,12 @@ extern void perror (const char *__s) __COLD;
>   
>   #ifdef	__USE_POSIX
>   /* Return the system file descriptor for STREAM.  */
> -extern int fileno (FILE *__stream) __THROW __wur;
> +extern int fileno (FILE *__stream) __THROW __wur __nonnull ((1));
>   #endif /* Use POSIX.  */
>   
>   #ifdef __USE_MISC
>   /* Faster version when locking is not required.  */
> -extern int fileno_unlocked (FILE *__stream) __THROW __wur;
> +extern int fileno_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
>   #endif
>   
>   
> @@ -878,7 +894,7 @@ extern int fileno_unlocked (FILE *__stream) __THROW __wur;
>   
>      This function is a possible cancellation point and therefore not
>      marked with __THROW.  */
> -extern int pclose (FILE *__stream);
> +extern int pclose (FILE *__stream) __nonnull ((1));
>   
>   /* Create a new stream connected to a pipe running the given command.
>   
> @@ -922,14 +938,14 @@ extern int obstack_vprintf (struct obstack *__restrict __obstack,
>   /* These are defined in POSIX.1:1996.  */
>   
>   /* Acquire ownership of STREAM.  */
> -extern void flockfile (FILE *__stream) __THROW;
> +extern void flockfile (FILE *__stream) __THROW __nonnull ((1));
>   
>   /* Try to acquire ownership of STREAM but do not block if it is not
>      possible.  */
> -extern int ftrylockfile (FILE *__stream) __THROW __wur;
> +extern int ftrylockfile (FILE *__stream) __THROW __wur __nonnull ((1));
>   
>   /* Relinquish the ownership granted for STREAM.  */
> -extern void funlockfile (FILE *__stream) __THROW;
> +extern void funlockfile (FILE *__stream) __THROW __nonnull ((1));
>   #endif /* POSIX */
>   
>   #if defined __USE_XOPEN && !defined __USE_XOPEN2K && !defined __USE_GNU
Siddhesh Poyarekar Sept. 26, 2023, 11:02 p.m. UTC | #19 
On 2023-09-25 13:00, Alejandro Colomar wrote:
> On Mon, Sep 25, 2023 at 07:53:26PM +0800, Xi Ruoyao wrote:
>> During the review of a GCC analyzer test case, we found most stdio
>> functions accepting a FILE * argument expect it to be nonnull and just
>> segfault when the argument is NULL.  Add nonnull attribute for them.
>>
>> fflush and fflush_unlocked are well defined when __stream is NULL so
>> they are not touched.
>>
>> For fputs, fgets, fread, fwrite, fprintf, vfprintf, and their unlocked
>> version, if __stream is empty but there is nothing to read or write,
>> they did not segfault.  But the standard disallow __stream to be empty
>> here, so nonnull attribute is also added for them.  Note that this may
>> blow up some old code already subtly broken.
>>
>> Also add __nonnull for _chk variants and __fortify_function versions for
>> them.
>>
>> Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> 
> Acked-by: Alejandro Colomar <alx@kernel.org>
> 

Alejandro, we don't use that tag in glibc, we use Reviewed-by to 
indicate that we have reviewed the patch.  Would you like to add your 
Reviewed-by?

Thanks,
Sid
Alejandro Colomar Sept. 26, 2023, 11:56 p.m. UTC | #20 
On Tue, Sep 26, 2023 at 07:02:08PM -0400, Siddhesh Poyarekar wrote:
> On 2023-09-25 13:00, Alejandro Colomar wrote:
> > On Mon, Sep 25, 2023 at 07:53:26PM +0800, Xi Ruoyao wrote:
> > > During the review of a GCC analyzer test case, we found most stdio
> > > functions accepting a FILE * argument expect it to be nonnull and just
> > > segfault when the argument is NULL.  Add nonnull attribute for them.
> > > 
> > > fflush and fflush_unlocked are well defined when __stream is NULL so
> > > they are not touched.
> > > 
> > > For fputs, fgets, fread, fwrite, fprintf, vfprintf, and their unlocked
> > > version, if __stream is empty but there is nothing to read or write,
> > > they did not segfault.  But the standard disallow __stream to be empty
> > > here, so nonnull attribute is also added for them.  Note that this may
> > > blow up some old code already subtly broken.
> > > 
> > > Also add __nonnull for _chk variants and __fortify_function versions for
> > > them.
> > > 
> > > Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> > 
> > Acked-by: Alejandro Colomar <alx@kernel.org>
> > 
> 
> Alejandro, we don't use that tag in glibc, we use Reviewed-by to indicate
> that we have reviewed the patch.  Would you like to add your Reviewed-by?

Sure, go ahead.

Thanks,
Alex

> 
> Thanks,
> Sid
Alejandro Colomar Sept. 26, 2023, 11:58 p.m. UTC | #21 
On Wed, Sep 27, 2023 at 01:56:58AM +0200, Alejandro Colomar wrote:
> On Tue, Sep 26, 2023 at 07:02:08PM -0400, Siddhesh Poyarekar wrote:
> > On 2023-09-25 13:00, Alejandro Colomar wrote:
> > > On Mon, Sep 25, 2023 at 07:53:26PM +0800, Xi Ruoyao wrote:
> > > > During the review of a GCC analyzer test case, we found most stdio
> > > > functions accepting a FILE * argument expect it to be nonnull and just
> > > > segfault when the argument is NULL.  Add nonnull attribute for them.
> > > > 
> > > > fflush and fflush_unlocked are well defined when __stream is NULL so
> > > > they are not touched.
> > > > 
> > > > For fputs, fgets, fread, fwrite, fprintf, vfprintf, and their unlocked
> > > > version, if __stream is empty but there is nothing to read or write,
> > > > they did not segfault.  But the standard disallow __stream to be empty
> > > > here, so nonnull attribute is also added for them.  Note that this may
> > > > blow up some old code already subtly broken.
> > > > 
> > > > Also add __nonnull for _chk variants and __fortify_function versions for
> > > > them.
> > > > 
> > > > Signed-off-by: Xi Ruoyao <xry111@xry111.site>
> > > 
> > > Acked-by: Alejandro Colomar <alx@kernel.org>
> > > 
> > 
> > Alejandro, we don't use that tag in glibc, we use Reviewed-by to indicate
> > that we have reviewed the patch.  Would you like to add your Reviewed-by?
> 
> Sure, go ahead.

To be more explicit:

Reviewed-by: Alejandro Colomar <alx@kernel.org>

> 
> Thanks,
> Alex
> 
> > 
> > Thanks,
> > Sid
diff mbox series

Patch

diff --git a/libio/bits/stdio2-decl.h b/libio/bits/stdio2-decl.h
index d7ef7283d6..3b60c72931 100644
--- a/libio/bits/stdio2-decl.h
+++ b/libio/bits/stdio2-decl.h
@@ -47,10 +47,12 @@  extern int __vsnprintf_chk (char *__restrict __s, size_t __n, int __flag,
 #if __USE_FORTIFY_LEVEL > 1
 
 extern int __fprintf_chk (FILE *__restrict __stream, int __flag,
-			  const char *__restrict __format, ...);
+			  const char *__restrict __format, ...)
+    __nonnull ((1));
 extern int __printf_chk (int __flag, const char *__restrict __format, ...);
 extern int __vfprintf_chk (FILE *__restrict __stream, int __flag,
-			   const char *__restrict __format, __gnuc_va_list __ap);
+			   const char *__restrict __format,
+			   __gnuc_va_list __ap) __nonnull ((1));
 extern int __vprintf_chk (int __flag, const char *__restrict __format,
 			  __gnuc_va_list __ap);
 
@@ -103,7 +105,7 @@  extern char *__REDIRECT (__fgets_chk_warn,
 
 extern char *__fgets_chk (char *__restrict __s, size_t __size, int __n,
 			  FILE *__restrict __stream)
-    __wur __attr_access ((__write_only__, 1, 3));
+    __wur __attr_access ((__write_only__, 1, 3)) __nonnull ((4));
 
 extern size_t __REDIRECT (__fread_alias,
 			  (void *__restrict __ptr, size_t __size,
@@ -119,7 +121,7 @@  extern size_t __REDIRECT (__fread_chk_warn,
 
 extern size_t __fread_chk (void *__restrict __ptr, size_t __ptrlen,
 			   size_t __size, size_t __n,
-			   FILE *__restrict __stream) __wur;
+			   FILE *__restrict __stream) __wur __nonnull ((5));
 
 #ifdef __USE_GNU
 extern char *__REDIRECT_FORTIFY (__fgets_unlocked_alias,
@@ -135,7 +137,7 @@  extern char *__REDIRECT (__fgets_unlocked_chk_warn,
 
 extern char *__fgets_unlocked_chk (char *__restrict __s, size_t __size,
 				   int __n, FILE *__restrict __stream)
-    __wur __attr_access ((__write_only__, 1, 3));
+    __wur __attr_access ((__write_only__, 1, 3)) __nonnull ((4));
 #endif
 
 #ifdef __USE_MISC
@@ -154,7 +156,8 @@  extern size_t __REDIRECT (__fread_unlocked_chk_warn,
 
 extern size_t __fread_unlocked_chk (void *__restrict __ptr, size_t __ptrlen,
 				    size_t __size, size_t __n,
-				    FILE *__restrict __stream) __wur;
+				    FILE *__restrict __stream)
+    __wur __nonnull ((5));
 #endif
 
 #endif /* bits/stdio2-decl.h.  */
diff --git a/libio/bits/stdio2.h b/libio/bits/stdio2.h
index 71226408ab..266dccdd1e 100644
--- a/libio/bits/stdio2.h
+++ b/libio/bits/stdio2.h
@@ -73,7 +73,7 @@  __NTH (vsnprintf (char *__restrict __s, size_t __n,
 
 #if __USE_FORTIFY_LEVEL > 1
 # ifdef __va_arg_pack
-__fortify_function int
+__fortify_function __nonnull ((1)) int
 fprintf (FILE *__restrict __stream, const char *__restrict __fmt, ...)
 {
   return __fprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1, __fmt,
@@ -102,7 +102,7 @@  vprintf (const char *__restrict __fmt, __gnuc_va_list __ap)
 #endif
 }
 
-__fortify_function int
+__fortify_function __nonnull ((1)) int
 vfprintf (FILE *__restrict __stream,
 	  const char *__restrict __fmt, __gnuc_va_list __ap)
 {
@@ -191,7 +191,8 @@  gets (char *__str)
 }
 #endif
 
-__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2) char *
+__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2)
+__nonnull ((3)) char *
 fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
 {
   size_t sz = __glibc_objsize (__s);
@@ -202,7 +203,7 @@  fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
   return __fgets_chk (__s, sz, __n, __stream);
 }
 
-__fortify_function __wur size_t
+__fortify_function __wur __nonnull ((4)) size_t
 fread (void *__restrict __ptr, size_t __size, size_t __n,
        FILE *__restrict __stream)
 {
@@ -215,7 +216,8 @@  fread (void *__restrict __ptr, size_t __size, size_t __n,
 }
 
 #ifdef __USE_GNU
-__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2) char *
+__fortify_function __wur __fortified_attr_access (__write_only__, 1, 2)
+__nonnull ((3)) char *
 fgets_unlocked (char *__restrict __s, int __n, FILE *__restrict __stream)
 {
   size_t sz = __glibc_objsize (__s);
@@ -229,7 +231,7 @@  fgets_unlocked (char *__restrict __s, int __n, FILE *__restrict __stream)
 
 #ifdef __USE_MISC
 # undef fread_unlocked
-__fortify_function __wur size_t
+__fortify_function __wur __nonnull ((4)) size_t
 fread_unlocked (void *__restrict __ptr, size_t __size, size_t __n,
 		FILE *__restrict __stream)
 {
diff --git a/libio/stdio.h b/libio/stdio.h
index 4cf9f1c012..a640e9beb5 100644
--- a/libio/stdio.h
+++ b/libio/stdio.h
@@ -1,5 +1,6 @@ 
 /* Define ISO C stdio on top of C++ iostreams.
    Copyright (C) 1991-2023 Free Software Foundation, Inc.
+   Copyright The GNU Toolchain Authors.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -278,7 +279,7 @@  extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
 extern FILE *__REDIRECT (freopen, (const char *__restrict __filename,
 				   const char *__restrict __modes,
 				   FILE *__restrict __stream), freopen64)
-  __wur;
+  __wur __nonnull ((3));
 # else
 #  define fopen fopen64
 #  define freopen freopen64
@@ -330,21 +331,22 @@  extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t *__sizeloc) __THROW
 
 /* If BUF is NULL, make STREAM unbuffered.
    Else make it use buffer BUF, of size BUFSIZ.  */
-extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW;
+extern void setbuf (FILE *__restrict __stream, char *__restrict __buf) __THROW
+  __nonnull ((1));
 /* Make STREAM use buffering mode MODE.
    If BUF is not NULL, use N bytes of it for buffering;
    else allocate an internal buffer N bytes long.  */
 extern int setvbuf (FILE *__restrict __stream, char *__restrict __buf,
-		    int __modes, size_t __n) __THROW;
+		    int __modes, size_t __n) __THROW __nonnull ((1));
 
 #ifdef	__USE_MISC
 /* If BUF is NULL, make STREAM unbuffered.
    Else make it use SIZE bytes of BUF for buffering.  */
 extern void setbuffer (FILE *__restrict __stream, char *__restrict __buf,
-		       size_t __size) __THROW;
+		       size_t __size) __THROW __nonnull ((1));
 
 /* Make STREAM line-buffered.  */
-extern void setlinebuf (FILE *__stream) __THROW;
+extern void setlinebuf (FILE *__stream) __THROW __nonnull ((1));
 #endif
 
 
@@ -353,7 +355,7 @@  extern void setlinebuf (FILE *__stream) __THROW;
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
 extern int fprintf (FILE *__restrict __stream,
-		    const char *__restrict __format, ...);
+		    const char *__restrict __format, ...) __nonnull ((1));
 /* Write formatted output to stdout.
 
    This function is a possible cancellation point and therefore not
@@ -368,7 +370,7 @@  extern int sprintf (char *__restrict __s,
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
 extern int vfprintf (FILE *__restrict __s, const char *__restrict __format,
-		     __gnuc_va_list __arg);
+		     __gnuc_va_list __arg) __nonnull ((1));
 /* Write formatted output to stdout from argument list ARG.
 
    This function is a possible cancellation point and therefore not
@@ -418,7 +420,7 @@  extern int dprintf (int __fd, const char *__restrict __fmt, ...)
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
 extern int fscanf (FILE *__restrict __stream,
-		   const char *__restrict __format, ...) __wur;
+		   const char *__restrict __format, ...) __wur __nonnull ((1));
 /* Read formatted input from stdin.
 
    This function is a possible cancellation point and therefore not
@@ -439,7 +441,7 @@  extern int sscanf (const char *__restrict __s,
 #  ifdef __REDIRECT
 extern int __REDIRECT (fscanf, (FILE *__restrict __stream,
 				const char *__restrict __format, ...),
-		       __isoc23_fscanf) __wur;
+		       __isoc23_fscanf) __wur __nonnull ((1));
 extern int __REDIRECT (scanf, (const char *__restrict __format, ...),
 		       __isoc23_scanf) __wur;
 extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
@@ -447,7 +449,8 @@  extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
 			   __isoc23_sscanf);
 #  else
 extern int __isoc23_fscanf (FILE *__restrict __stream,
-			    const char *__restrict __format, ...) __wur;
+			    const char *__restrict __format, ...) __wur
+  __nonnull ((1));
 extern int __isoc23_scanf (const char *__restrict __format, ...) __wur;
 extern int __isoc23_sscanf (const char *__restrict __s,
 			    const char *__restrict __format, ...) __THROW;
@@ -459,7 +462,7 @@  extern int __isoc23_sscanf (const char *__restrict __s,
 #  ifdef __REDIRECT
 extern int __REDIRECT (fscanf, (FILE *__restrict __stream,
 				const char *__restrict __format, ...),
-		       __isoc99_fscanf) __wur;
+		       __isoc99_fscanf) __wur __nonnull ((1));
 extern int __REDIRECT (scanf, (const char *__restrict __format, ...),
 		       __isoc99_scanf) __wur;
 extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
@@ -467,7 +470,8 @@  extern int __REDIRECT_NTH (sscanf, (const char *__restrict __s,
 			   __isoc99_sscanf);
 #  else
 extern int __isoc99_fscanf (FILE *__restrict __stream,
-			    const char *__restrict __format, ...) __wur;
+			    const char *__restrict __format, ...) __wur
+  __nonnull ((1));
 extern int __isoc99_scanf (const char *__restrict __format, ...) __wur;
 extern int __isoc99_sscanf (const char *__restrict __s,
 			    const char *__restrict __format, ...) __THROW;
@@ -485,7 +489,7 @@  extern int __isoc99_sscanf (const char *__restrict __s,
    marked with __THROW.  */
 extern int vfscanf (FILE *__restrict __s, const char *__restrict __format,
 		    __gnuc_va_list __arg)
-     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
+     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
 
 /* Read formatted input from stdin into argument list ARG.
 
@@ -508,7 +512,7 @@  extern int __REDIRECT (vfscanf,
 		       (FILE *__restrict __s,
 			const char *__restrict __format, __gnuc_va_list __arg),
 		       __isoc23_vfscanf)
-     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
+     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
 extern int __REDIRECT (vscanf, (const char *__restrict __format,
 				__gnuc_va_list __arg), __isoc23_vscanf)
      __attribute__ ((__format__ (__scanf__, 1, 0))) __wur;
@@ -520,7 +524,7 @@  extern int __REDIRECT_NTH (vsscanf,
 #   elif !defined __REDIRECT
 extern int __isoc23_vfscanf (FILE *__restrict __s,
 			     const char *__restrict __format,
-			     __gnuc_va_list __arg) __wur;
+			     __gnuc_va_list __arg) __wur __nonnull ((1));
 extern int __isoc23_vscanf (const char *__restrict __format,
 			    __gnuc_va_list __arg) __wur;
 extern int __isoc23_vsscanf (const char *__restrict __s,
@@ -537,7 +541,7 @@  extern int __REDIRECT (vfscanf,
 		       (FILE *__restrict __s,
 			const char *__restrict __format, __gnuc_va_list __arg),
 		       __isoc99_vfscanf)
-     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur;
+     __attribute__ ((__format__ (__scanf__, 2, 0))) __wur __nonnull ((1));
 extern int __REDIRECT (vscanf, (const char *__restrict __format,
 				__gnuc_va_list __arg), __isoc99_vscanf)
      __attribute__ ((__format__ (__scanf__, 1, 0))) __wur;
@@ -549,7 +553,7 @@  extern int __REDIRECT_NTH (vsscanf,
 #   elif !defined __REDIRECT
 extern int __isoc99_vfscanf (FILE *__restrict __s,
 			     const char *__restrict __format,
-			     __gnuc_va_list __arg) __wur;
+			     __gnuc_va_list __arg) __wur __nonnull ((1));
 extern int __isoc99_vscanf (const char *__restrict __format,
 			    __gnuc_va_list __arg) __wur;
 extern int __isoc99_vsscanf (const char *__restrict __s,
@@ -568,8 +572,8 @@  extern int __isoc99_vsscanf (const char *__restrict __s,
 
    These functions are possible cancellation points and therefore not
    marked with __THROW.  */
-extern int fgetc (FILE *__stream);
-extern int getc (FILE *__stream);
+extern int fgetc (FILE *__stream) __nonnull ((1));
+extern int getc (FILE *__stream) __nonnull ((1));
 
 /* Read a character from stdin.
 
@@ -582,7 +586,7 @@  extern int getchar (void);
 
    These functions are possible cancellation points and therefore not
    marked with __THROW.  */
-extern int getc_unlocked (FILE *__stream);
+extern int getc_unlocked (FILE *__stream) __nonnull ((1));
 extern int getchar_unlocked (void);
 #endif /* Use POSIX.  */
 
@@ -593,7 +597,7 @@  extern int getchar_unlocked (void);
    cancellation point.  But due to similarity with an POSIX interface
    or due to the implementation it is a cancellation point and
    therefore not marked with __THROW.  */
-extern int fgetc_unlocked (FILE *__stream);
+extern int fgetc_unlocked (FILE *__stream) __nonnull ((1));
 #endif /* Use MISC.  */
 
 
@@ -604,8 +608,8 @@  extern int fgetc_unlocked (FILE *__stream);
 
    These functions is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int fputc (int __c, FILE *__stream);
-extern int putc (int __c, FILE *__stream);
+extern int fputc (int __c, FILE *__stream) __nonnull ((2));
+extern int putc (int __c, FILE *__stream) __nonnull ((2));
 
 /* Write a character to stdout.
 
@@ -620,7 +624,7 @@  extern int putchar (int __c);
    cancellation point.  But due to similarity with an POSIX interface
    or due to the implementation it is a cancellation point and
    therefore not marked with __THROW.  */
-extern int fputc_unlocked (int __c, FILE *__stream);
+extern int fputc_unlocked (int __c, FILE *__stream) __nonnull ((2));
 #endif /* Use MISC.  */
 
 #ifdef __USE_POSIX199506
@@ -628,7 +632,7 @@  extern int fputc_unlocked (int __c, FILE *__stream);
 
    These functions are possible cancellation points and therefore not
    marked with __THROW.  */
-extern int putc_unlocked (int __c, FILE *__stream);
+extern int putc_unlocked (int __c, FILE *__stream) __nonnull ((2));
 extern int putchar_unlocked (int __c);
 #endif /* Use POSIX.  */
 
@@ -636,10 +640,10 @@  extern int putchar_unlocked (int __c);
 #if defined __USE_MISC \
     || (defined __USE_XOPEN && !defined __USE_XOPEN2K)
 /* Get a word (int) from STREAM.  */
-extern int getw (FILE *__stream);
+extern int getw (FILE *__stream) __nonnull ((1));
 
 /* Write a word (int) to STREAM.  */
-extern int putw (int __w, FILE *__stream);
+extern int putw (int __w, FILE *__stream) __nonnull ((2));
 #endif
 
 
@@ -648,7 +652,7 @@  extern int putw (int __w, FILE *__stream);
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
 extern char *fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
-     __wur __fortified_attr_access (__write_only__, 1, 2);
+     __wur __fortified_attr_access (__write_only__, 1, 2) __nonnull ((3));
 
 #if __GLIBC_USE (DEPRECATED_GETS)
 /* Get a newline-terminated string from stdin, removing the newline.
@@ -672,7 +676,7 @@  extern char *gets (char *__s) __wur __attribute_deprecated__;
    therefore not marked with __THROW.  */
 extern char *fgets_unlocked (char *__restrict __s, int __n,
 			     FILE *__restrict __stream) __wur
-    __fortified_attr_access (__write_only__, 1, 2);
+    __fortified_attr_access (__write_only__, 1, 2) __nonnull ((3));
 #endif
 
 
@@ -689,10 +693,10 @@  extern char *fgets_unlocked (char *__restrict __s, int __n,
    therefore not marked with __THROW.  */
 extern __ssize_t __getdelim (char **__restrict __lineptr,
                              size_t *__restrict __n, int __delimiter,
-                             FILE *__restrict __stream) __wur;
+                             FILE *__restrict __stream) __wur __nonnull ((4));
 extern __ssize_t getdelim (char **__restrict __lineptr,
                            size_t *__restrict __n, int __delimiter,
-                           FILE *__restrict __stream) __wur;
+                           FILE *__restrict __stream) __wur __nonnull ((4));
 
 /* Like `getdelim', but reads up to a newline.
 
@@ -702,7 +706,7 @@  extern __ssize_t getdelim (char **__restrict __lineptr,
    therefore not marked with __THROW.  */
 extern __ssize_t getline (char **__restrict __lineptr,
                           size_t *__restrict __n,
-                          FILE *__restrict __stream) __wur;
+                          FILE *__restrict __stream) __wur __nonnull ((3));
 #endif
 
 
@@ -710,7 +714,8 @@  extern __ssize_t getline (char **__restrict __lineptr,
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int fputs (const char *__restrict __s, FILE *__restrict __stream);
+extern int fputs (const char *__restrict __s, FILE *__restrict __stream)
+  __nonnull ((2));
 
 /* Write a string, followed by a newline, to stdout.
 
@@ -723,7 +728,7 @@  extern int puts (const char *__s);
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int ungetc (int __c, FILE *__stream);
+extern int ungetc (int __c, FILE *__stream) __nonnull ((2));
 
 
 /* Read chunks of generic data from STREAM.
@@ -731,13 +736,14 @@  extern int ungetc (int __c, FILE *__stream);
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
 extern size_t fread (void *__restrict __ptr, size_t __size,
-		     size_t __n, FILE *__restrict __stream) __wur;
+		     size_t __n, FILE *__restrict __stream) __wur
+  __nonnull((4));
 /* Write chunks of generic data to STREAM.
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
 extern size_t fwrite (const void *__restrict __ptr, size_t __size,
-		      size_t __n, FILE *__restrict __s);
+		      size_t __n, FILE *__restrict __s) __nonnull((4));
 
 #ifdef __USE_GNU
 /* This function does the same as `fputs' but does not lock the stream.
@@ -747,7 +753,7 @@  extern size_t fwrite (const void *__restrict __ptr, size_t __size,
    or due to the implementation it is a cancellation point and
    therefore not marked with __THROW.  */
 extern int fputs_unlocked (const char *__restrict __s,
-			   FILE *__restrict __stream);
+			   FILE *__restrict __stream) __nonnull ((2));
 #endif
 
 #ifdef __USE_MISC
@@ -758,9 +764,11 @@  extern int fputs_unlocked (const char *__restrict __s,
    or due to the implementation they are cancellation points and
    therefore not marked with __THROW.  */
 extern size_t fread_unlocked (void *__restrict __ptr, size_t __size,
-			      size_t __n, FILE *__restrict __stream) __wur;
+			      size_t __n, FILE *__restrict __stream) __wur
+  __nonnull ((4));
 extern size_t fwrite_unlocked (const void *__restrict __ptr, size_t __size,
-			       size_t __n, FILE *__restrict __stream);
+			       size_t __n, FILE *__restrict __stream)
+  __nonnull ((4));
 #endif
 
 
@@ -768,17 +776,18 @@  extern size_t fwrite_unlocked (const void *__restrict __ptr, size_t __size,
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int fseek (FILE *__stream, long int __off, int __whence);
+extern int fseek (FILE *__stream, long int __off, int __whence)
+  __nonnull ((1));
 /* Return the current position of STREAM.
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern long int ftell (FILE *__stream) __wur;
+extern long int ftell (FILE *__stream) __wur __nonnull ((1));
 /* Rewind to the beginning of STREAM.
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern void rewind (FILE *__stream);
+extern void rewind (FILE *__stream) __nonnull ((1));
 
 /* The Single Unix Specification, Version 2, specifies an alternative,
    more adequate interface for the two functions above which deal with
@@ -791,18 +800,20 @@  extern void rewind (FILE *__stream);
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int fseeko (FILE *__stream, __off_t __off, int __whence);
+extern int fseeko (FILE *__stream, __off_t __off, int __whence)
+  __nonnull ((1));
 /* Return the current position of STREAM.
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern __off_t ftello (FILE *__stream) __wur;
+extern __off_t ftello (FILE *__stream) __wur __nonnull ((1));
 # else
 #  ifdef __REDIRECT
 extern int __REDIRECT (fseeko,
 		       (FILE *__stream, __off64_t __off, int __whence),
-		       fseeko64);
-extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64);
+		       fseeko64) __nonnull ((1));
+extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64)
+  __nonnull ((1));
 #  else
 #   define fseeko fseeko64
 #   define ftello ftello64
@@ -815,18 +826,21 @@  extern __off64_t __REDIRECT (ftello, (FILE *__stream), ftello64);
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int fgetpos (FILE *__restrict __stream, fpos_t *__restrict __pos);
+extern int fgetpos (FILE *__restrict __stream, fpos_t *__restrict __pos)
+  __nonnull ((1));
 /* Set STREAM's position.
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int fsetpos (FILE *__stream, const fpos_t *__pos);
+extern int fsetpos (FILE *__stream, const fpos_t *__pos) __nonnull ((1));
 #else
 # ifdef __REDIRECT
 extern int __REDIRECT (fgetpos, (FILE *__restrict __stream,
-				 fpos_t *__restrict __pos), fgetpos64);
+				 fpos_t *__restrict __pos), fgetpos64)
+  __nonnull ((1));
 extern int __REDIRECT (fsetpos,
-		       (FILE *__stream, const fpos_t *__pos), fsetpos64);
+		       (FILE *__stream, const fpos_t *__pos), fsetpos64)
+  __nonnull ((1));
 # else
 #  define fgetpos fgetpos64
 #  define fsetpos fsetpos64
@@ -834,24 +848,26 @@  extern int __REDIRECT (fsetpos,
 #endif
 
 #ifdef __USE_LARGEFILE64
-extern int fseeko64 (FILE *__stream, __off64_t __off, int __whence);
-extern __off64_t ftello64 (FILE *__stream) __wur;
-extern int fgetpos64 (FILE *__restrict __stream, fpos64_t *__restrict __pos);
-extern int fsetpos64 (FILE *__stream, const fpos64_t *__pos);
+extern int fseeko64 (FILE *__stream, __off64_t __off, int __whence)
+  __nonnull ((1));
+extern __off64_t ftello64 (FILE *__stream) __wur __nonnull ((1));
+extern int fgetpos64 (FILE *__restrict __stream, fpos64_t *__restrict __pos)
+  __nonnull ((1));
+extern int fsetpos64 (FILE *__stream, const fpos64_t *__pos) __nonnull ((1));
 #endif
 
 /* Clear the error and EOF indicators for STREAM.  */
-extern void clearerr (FILE *__stream) __THROW;
+extern void clearerr (FILE *__stream) __THROW __nonnull ((1));
 /* Return the EOF indicator for STREAM.  */
-extern int feof (FILE *__stream) __THROW __wur;
+extern int feof (FILE *__stream) __THROW __wur __nonnull ((1));
 /* Return the error indicator for STREAM.  */
-extern int ferror (FILE *__stream) __THROW __wur;
+extern int ferror (FILE *__stream) __THROW __wur __nonnull ((1));
 
 #ifdef __USE_MISC
 /* Faster versions when locking is not required.  */
-extern void clearerr_unlocked (FILE *__stream) __THROW;
-extern int feof_unlocked (FILE *__stream) __THROW __wur;
-extern int ferror_unlocked (FILE *__stream) __THROW __wur;
+extern void clearerr_unlocked (FILE *__stream) __THROW __nonnull ((1));
+extern int feof_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
+extern int ferror_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
 #endif
 
 
@@ -864,12 +880,12 @@  extern void perror (const char *__s) __COLD;
 
 #ifdef	__USE_POSIX
 /* Return the system file descriptor for STREAM.  */
-extern int fileno (FILE *__stream) __THROW __wur;
+extern int fileno (FILE *__stream) __THROW __wur __nonnull ((1));
 #endif /* Use POSIX.  */
 
 #ifdef __USE_MISC
 /* Faster version when locking is not required.  */
-extern int fileno_unlocked (FILE *__stream) __THROW __wur;
+extern int fileno_unlocked (FILE *__stream) __THROW __wur __nonnull ((1));
 #endif
 
 
@@ -878,7 +894,7 @@  extern int fileno_unlocked (FILE *__stream) __THROW __wur;
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int pclose (FILE *__stream);
+extern int pclose (FILE *__stream) __nonnull ((1));
 
 /* Create a new stream connected to a pipe running the given command.
 
@@ -922,14 +938,14 @@  extern int obstack_vprintf (struct obstack *__restrict __obstack,
 /* These are defined in POSIX.1:1996.  */
 
 /* Acquire ownership of STREAM.  */
-extern void flockfile (FILE *__stream) __THROW;
+extern void flockfile (FILE *__stream) __THROW __nonnull ((1));
 
 /* Try to acquire ownership of STREAM but do not block if it is not
    possible.  */
-extern int ftrylockfile (FILE *__stream) __THROW __wur;
+extern int ftrylockfile (FILE *__stream) __THROW __wur __nonnull ((1));
 
 /* Relinquish the ownership granted for STREAM.  */
-extern void funlockfile (FILE *__stream) __THROW;
+extern void funlockfile (FILE *__stream) __THROW __nonnull ((1));
 #endif /* POSIX */
 
 #if defined __USE_XOPEN && !defined __USE_XOPEN2K && !defined __USE_GNU