diff mbox series

libio: Add __nonnull for FILE * arguments of fclose and freopen

Message ID 20230421062133.2084910-1-xry111@xry111.site
State Committed
Commit 71d9e0fe766a3c22a730995b9d024960970670af
Headers
Series libio: Add __nonnull for FILE * arguments of fclose and freopen |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Xi Ruoyao April 21, 2023, 6:21 a.m. UTC 
  Calling fclose or freopen with a null FILE * is undefined behavior, and
doing so in practice will cause a SIGSEGV.  So it seems suitable for
__nonnull.

This will help the compiler to warn for some buggy code, like
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109570.
---
 libio/stdio.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Adhemerval Zanella Netto April 21, 2023, 1 p.m. UTC | #1 
On 21/04/23 03:21, Xi Ruoyao wrote:
> Calling fclose or freopen with a null FILE * is undefined behavior, and
> doing so in practice will cause a SIGSEGV.  So it seems suitable for
> __nonnull.
> 
> This will help the compiler to warn for some buggy code, like
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109570.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  libio/stdio.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libio/stdio.h b/libio/stdio.h
> index 45ddafdf20..bfb8415d3b 100644
> --- a/libio/stdio.h
> +++ b/libio/stdio.h
> @@ -180,7 +180,7 @@ extern int renameat2 (int __oldfd, const char *__old, int __newfd,
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int fclose (FILE *__stream);
> +extern int fclose (FILE *__stream) __nonnull ((1));
>  
>  #undef __attr_dealloc_fclose
>  #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
> @@ -269,7 +269,7 @@ extern FILE *fopen (const char *__restrict __filename,
>     marked with __THROW.  */
>  extern FILE *freopen (const char *__restrict __filename,
>  		      const char *__restrict __modes,
> -		      FILE *__restrict __stream) __wur;
> +		      FILE *__restrict __stream) __wur __nonnull ((3));
>  #else
>  # ifdef __REDIRECT
>  extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
> @@ -290,7 +290,7 @@ extern FILE *fopen64 (const char *__restrict __filename,
>    __attribute_malloc__ __attr_dealloc_fclose __wur;
>  extern FILE *freopen64 (const char *__restrict __filename,
>  			const char *__restrict __modes,
> -			FILE *__restrict __stream) __wur;
> +			FILE *__restrict __stream) __wur __nonnull ((3));
>  #endif
>  
>  #ifdef	__USE_POSIX
Xi Ruoyao April 22, 2023, 7:32 a.m. UTC | #2 
On Fri, 2023-04-21 at 10:00 -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 21/04/23 03:21, Xi Ruoyao wrote:
> > Calling fclose or freopen with a null FILE * is undefined behavior,
> > and
> > doing so in practice will cause a SIGSEGV.  So it seems suitable for
> > __nonnull.
> > 
> > This will help the compiler to warn for some buggy code, like
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109570.
> 
> LGTM, thanks.

Thanks, please commit it if there is no objections.  Or may I get a
write after approve access for glibc.git?

> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> > ---
> >  libio/stdio.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libio/stdio.h b/libio/stdio.h
> > index 45ddafdf20..bfb8415d3b 100644
> > --- a/libio/stdio.h
> > +++ b/libio/stdio.h
> > @@ -180,7 +180,7 @@ extern int renameat2 (int __oldfd, const char
> > *__old, int __newfd,
> >  
> >     This function is a possible cancellation point and therefore not
> >     marked with __THROW.  */
> > -extern int fclose (FILE *__stream);
> > +extern int fclose (FILE *__stream) __nonnull ((1));
> >  
> >  #undef __attr_dealloc_fclose
> >  #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
> > @@ -269,7 +269,7 @@ extern FILE *fopen (const char *__restrict
> > __filename,
> >     marked with __THROW.  */
> >  extern FILE *freopen (const char *__restrict __filename,
> >                       const char *__restrict __modes,
> > -                     FILE *__restrict __stream) __wur;
> > +                     FILE *__restrict __stream) __wur __nonnull
> > ((3));
> >  #else
> >  # ifdef __REDIRECT
> >  extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
> > @@ -290,7 +290,7 @@ extern FILE *fopen64 (const char *__restrict
> > __filename,
> >    __attribute_malloc__ __attr_dealloc_fclose __wur;
> >  extern FILE *freopen64 (const char *__restrict __filename,
> >                         const char *__restrict __modes,
> > -                       FILE *__restrict __stream) __wur;
> > +                       FILE *__restrict __stream) __wur __nonnull
> > ((3));
> >  #endif
> >  
> >  #ifdef __USE_POSIX
Xi Ruoyao May 16, 2023, 2:50 a.m. UTC | #3 
Ping, as this seems reviewed as OK but fallen through two review
meetings. 

On Fri, 2023-04-21 at 14:21 +0800, Xi Ruoyao wrote:
> Calling fclose or freopen with a null FILE * is undefined behavior,
> and
> doing so in practice will cause a SIGSEGV.  So it seems suitable for
> __nonnull.
> 
> This will help the compiler to warn for some buggy code, like
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109570.
> ---
>  libio/stdio.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libio/stdio.h b/libio/stdio.h
> index 45ddafdf20..bfb8415d3b 100644
> --- a/libio/stdio.h
> +++ b/libio/stdio.h
> @@ -180,7 +180,7 @@ extern int renameat2 (int __oldfd, const char
> *__old, int __newfd,
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern int fclose (FILE *__stream);
> +extern int fclose (FILE *__stream) __nonnull ((1));
>  
>  #undef __attr_dealloc_fclose
>  #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
> @@ -269,7 +269,7 @@ extern FILE *fopen (const char *__restrict
> __filename,
>     marked with __THROW.  */
>  extern FILE *freopen (const char *__restrict __filename,
>                       const char *__restrict __modes,
> -                     FILE *__restrict __stream) __wur;
> +                     FILE *__restrict __stream) __wur __nonnull
> ((3));
>  #else
>  # ifdef __REDIRECT
>  extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
> @@ -290,7 +290,7 @@ extern FILE *fopen64 (const char *__restrict
> __filename,
>    __attribute_malloc__ __attr_dealloc_fclose __wur;
>  extern FILE *freopen64 (const char *__restrict __filename,
>                         const char *__restrict __modes,
> -                       FILE *__restrict __stream) __wur;
> +                       FILE *__restrict __stream) __wur __nonnull
> ((3));
>  #endif
>  
>  #ifdef __USE_POSIX
Adhemerval Zanella Netto May 16, 2023, 10:04 a.m. UTC | #4 
Done.

On 15/05/23 23:50, Xi Ruoyao wrote:
> Ping, as this seems reviewed as OK but fallen through two review
> meetings. 
> 
> On Fri, 2023-04-21 at 14:21 +0800, Xi Ruoyao wrote:
>> Calling fclose or freopen with a null FILE * is undefined behavior,
>> and
>> doing so in practice will cause a SIGSEGV.  So it seems suitable for
>> __nonnull.
>>
>> This will help the compiler to warn for some buggy code, like
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109570.
>> ---
>>  libio/stdio.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/libio/stdio.h b/libio/stdio.h
>> index 45ddafdf20..bfb8415d3b 100644
>> --- a/libio/stdio.h
>> +++ b/libio/stdio.h
>> @@ -180,7 +180,7 @@ extern int renameat2 (int __oldfd, const char
>> *__old, int __newfd,
>>  
>>     This function is a possible cancellation point and therefore not
>>     marked with __THROW.  */
>> -extern int fclose (FILE *__stream);
>> +extern int fclose (FILE *__stream) __nonnull ((1));
>>  
>>  #undef __attr_dealloc_fclose
>>  #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
>> @@ -269,7 +269,7 @@ extern FILE *fopen (const char *__restrict
>> __filename,
>>     marked with __THROW.  */
>>  extern FILE *freopen (const char *__restrict __filename,
>>                       const char *__restrict __modes,
>> -                     FILE *__restrict __stream) __wur;
>> +                     FILE *__restrict __stream) __wur __nonnull
>> ((3));
>>  #else
>>  # ifdef __REDIRECT
>>  extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
>> @@ -290,7 +290,7 @@ extern FILE *fopen64 (const char *__restrict
>> __filename,
>>    __attribute_malloc__ __attr_dealloc_fclose __wur;
>>  extern FILE *freopen64 (const char *__restrict __filename,
>>                         const char *__restrict __modes,
>> -                       FILE *__restrict __stream) __wur;
>> +                       FILE *__restrict __stream) __wur __nonnull
>> ((3));
>>  #endif
>>  
>>  #ifdef __USE_POSIX
>
Carlos O'Donell May 22, 2023, 12:36 p.m. UTC | #5 
On 5/16/23 06:04, Adhemerval Zanella Netto wrote:
> Done.

Thanks for pushing this last week!

> On 15/05/23 23:50, Xi Ruoyao wrote:
>> Ping, as this seems reviewed as OK but fallen through two review
>> meetings. 
>>
>> On Fri, 2023-04-21 at 14:21 +0800, Xi Ruoyao wrote:
>>> Calling fclose or freopen with a null FILE * is undefined behavior,
>>> and
>>> doing so in practice will cause a SIGSEGV.  So it seems suitable for
>>> __nonnull.
>>>
>>> This will help the compiler to warn for some buggy code, like
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109570.
>>> ---
>>>  libio/stdio.h | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/libio/stdio.h b/libio/stdio.h
>>> index 45ddafdf20..bfb8415d3b 100644
>>> --- a/libio/stdio.h
>>> +++ b/libio/stdio.h
>>> @@ -180,7 +180,7 @@ extern int renameat2 (int __oldfd, const char
>>> *__old, int __newfd,
>>>  
>>>     This function is a possible cancellation point and therefore not
>>>     marked with __THROW.  */
>>> -extern int fclose (FILE *__stream);
>>> +extern int fclose (FILE *__stream) __nonnull ((1));
>>>  
>>>  #undef __attr_dealloc_fclose
>>>  #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
>>> @@ -269,7 +269,7 @@ extern FILE *fopen (const char *__restrict
>>> __filename,
>>>     marked with __THROW.  */
>>>  extern FILE *freopen (const char *__restrict __filename,
>>>                       const char *__restrict __modes,
>>> -                     FILE *__restrict __stream) __wur;
>>> +                     FILE *__restrict __stream) __wur __nonnull
>>> ((3));
>>>  #else
>>>  # ifdef __REDIRECT
>>>  extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
>>> @@ -290,7 +290,7 @@ extern FILE *fopen64 (const char *__restrict
>>> __filename,
>>>    __attribute_malloc__ __attr_dealloc_fclose __wur;
>>>  extern FILE *freopen64 (const char *__restrict __filename,
>>>                         const char *__restrict __modes,
>>> -                       FILE *__restrict __stream) __wur;
>>> +                       FILE *__restrict __stream) __wur __nonnull
>>> ((3));
>>>  #endif
>>>  
>>>  #ifdef __USE_POSIX
>>
>
diff mbox series

Patch

diff --git a/libio/stdio.h b/libio/stdio.h
index 45ddafdf20..bfb8415d3b 100644
--- a/libio/stdio.h
+++ b/libio/stdio.h
@@ -180,7 +180,7 @@  extern int renameat2 (int __oldfd, const char *__old, int __newfd,
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int fclose (FILE *__stream);
+extern int fclose (FILE *__stream) __nonnull ((1));
 
 #undef __attr_dealloc_fclose
 #define __attr_dealloc_fclose __attr_dealloc (fclose, 1)
@@ -269,7 +269,7 @@  extern FILE *fopen (const char *__restrict __filename,
    marked with __THROW.  */
 extern FILE *freopen (const char *__restrict __filename,
 		      const char *__restrict __modes,
-		      FILE *__restrict __stream) __wur;
+		      FILE *__restrict __stream) __wur __nonnull ((3));
 #else
 # ifdef __REDIRECT
 extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
@@ -290,7 +290,7 @@  extern FILE *fopen64 (const char *__restrict __filename,
   __attribute_malloc__ __attr_dealloc_fclose __wur;
 extern FILE *freopen64 (const char *__restrict __filename,
 			const char *__restrict __modes,
-			FILE *__restrict __stream) __wur;
+			FILE *__restrict __stream) __wur __nonnull ((3));
 #endif
 
 #ifdef	__USE_POSIX