On 5/18/23 20:06, Alex Colomar wrote: > Hi Xi! > > On 5/18/23 19:25, Xi Ruoyao via Libc-alpha 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. >> >> setbuf is well defined when __stream is NULL so it's not touched. Is this true? I couldn't find anything about it in either setbuf(3), setbuf(3POSIX), or info libc. The buffer can be NULL, but the stream? >> >> For fputs, fgets, fread, fwrite, fprintf, vfprintf, and their unlocked >> version, if __stream is empty but there is nothing to read or write, >> they don't segfault and I'm not sure if the standard allows such a use >> so I left them out. >> --- >>   libio/stdio.h | 119 ++++++++++++++++++++++++++------------------------ >>   1 file changed, 62 insertions(+), 57 deletions(-) >> >> diff --git a/libio/stdio.h b/libio/stdio.h >> index 4cf9f1c012..ae3d7295d4 100644 >> --- a/libio/stdio.h >> +++ b/libio/stdio.h >> @@ -232,7 +232,7 @@ extern char *tempnam (const char *__dir, const >> char *__pfx) >>      This function is a possible cancellation point and therefore not >>      marked with __THROW.  */ >> -extern int fflush (FILE *__stream); >> +extern int fflush (FILE *__stream) __nonnull ((1)); > > flush(NULL) is well defined.  It flushes all streams that can be > flushed.  This reminds me that I should document that in the SYNOPSIS > section of the manual page; an oversight on my side. > >>   #ifdef __USE_MISC >>   /* Faster versions when locking is not required. >> @@ -241,7 +241,7 @@ extern int fflush (FILE *__stream); >>      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 fflush_unlocked (FILE *__stream); >> +extern int fflush_unlocked (FILE *__stream) __nonnull ((1)); > > Without checking, I'll guess that fflush_unlocked(NULL) is also well > defined. > > I didn't see any other similar cases, but I may have missed some; I > didn't revise them all thoroughly; please check. > >>   #endif >>   #ifdef __USE_GNU >> @@ -278,7 +278,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 >> @@ -335,16 +335,16 @@ extern void setbuf (FILE *__restrict __stream, >> char *__restrict __buf) __THROW; >>      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 >> @@ -418,7 +418,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 +439,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 +447,7 @@ 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 +459,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 +467,7 @@ 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 +485,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 +508,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 +520,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 +537,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 +549,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 +568,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 +582,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 +593,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 +604,8 @@ extern int fgetc_unlocked (FILE *__stream); >>      These functions is a possible cancellation point and therefore not > > Aside: here's a typo mixing plural and singular in the sentence (not > part of your commit).  I'll prepare a patch for it. > >>      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 +620,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 +628,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 +636,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 >> @@ -689,10 +689,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 +702,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 >> @@ -723,7 +723,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. >> @@ -768,17 +768,17 @@ 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 +791,19 @@ 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 +816,20 @@ 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 +837,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 +869,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 +883,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)); > > You didn't patch fclose(3).  Any reason?  I guess it's similarly UB to > call fclose(NULL). > > Cheers, > Alex > >>   /* Create a new stream connected to a pipe running the given command. >> @@ -922,14 +927,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 > -- GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5