From: Alex Colomar <alx.manpages@gmail.com>
To: Xi Ruoyao <xry111@xry111.site>, libc-alpha@sourceware.org
Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>,
Carlos O'Donell <carlos@redhat.com>
Subject: Re: [PATCH] libio: Add nonnull attribute for most FILE * arguments in stdio.h
Date: Thu, 18 May 2023 20:29:57 +0200 [thread overview]
Message-ID: <ee297ca5-ee6a-7215-e786-2d30f1b40f27@gmail.com> (raw)
In-Reply-To: <d96fd93d-4e49-111b-175d-314ff38cc9f1@gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 22028 bytes --]
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
>
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2023-05-18 18:30 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-18 17:25 Xi Ruoyao
2023-05-18 18:06 ` Alex Colomar
2023-05-18 18:29 ` Alex Colomar [this message]
2023-05-19 4:13 ` Xi Ruoyao
2023-05-19 5:21 ` Xi Ruoyao
2023-05-19 12:40 ` Alejandro Colomar
2023-05-19 13:03 ` Xi Ruoyao
2023-05-19 13:07 ` Alejandro Colomar
2023-05-18 18:56 ` Alejandro Colomar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ee297ca5-ee6a-7215-e786-2d30f1b40f27@gmail.com \
--to=alx.manpages@gmail.com \
--cc=adhemerval.zanella@linaro.org \
--cc=carlos@redhat.com \
--cc=libc-alpha@sourceware.org \
--cc=xry111@xry111.site \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).