[PATCH] Don't pass NULL pointer to error [BZ #24556]

[PATCH] Don't pass NULL pointer to error [BZ #24556]

[H.J. Lu]

GCC 9 with -O3 will issue a warning for passing NULL pointer to ‘%s':

In file included from ../include/bits/error.h:1,
                 from ../misc/error.h:57,
                 from ../include/error.h:2,
                 from bench-string.h:60,
                 from bench-strstr.c:22:
In function ‘error’,
    inlined from ‘do_one_test’ at bench-strstr.c:149:7,
    inlined from ‘do_test’ at bench-strstr.c:201:5,
    inlined from ‘test_main’ at bench-strstr.c:220:2:
../include/bits/../../misc/bits/error.h:42:5: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
   42 |     __error_alias (__status, __errnum, __format, __va_arg_pack ());
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘error’,
    inlined from ‘do_one_test’ at bench-strstr.c:149:7,
    inlined from ‘do_test’ at bench-strstr.c:201:5,
    inlined from ‘test_main’ at bench-strstr.c:227:2:
../include/bits/../../misc/bits/error.h:42:5: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
   42 |     __error_alias (__status, __errnum, __format, __va_arg_pack ());
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

We can either disable -Werror=format-overflow= or don't pass NULL pointer
to error.  This patch does the latter.

	[BZ #24556]
	* benchtests/bench-strstr.c (do_one_test): Don't pass NULL
	pointer to error.
---
 benchtests/bench-strstr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/benchtests/bench-strstr.c b/benchtests/bench-strstr.c
index b4cd141083..104dd979f0 100644
--- a/benchtests/bench-strstr.c
+++ b/benchtests/bench-strstr.c
@@ -147,7 +147,7 @@ do_one_test (impl_t *impl, const char *s1, const char *s2, char *exp_result)
   if (res != exp_result)
     {
       error (0, 0, "Wrong result in function %s %s %s", impl->name,
-	     res, exp_result);
+	     res, exp_result ? exp_result : "(null)");
       ret = 1;
     }
 }
-- 
2.20.1

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[Florian Weimer]

* H. J. Lu:

> In function ‘error’,
>     inlined from ‘do_one_test’ at bench-strstr.c:149:7,
>     inlined from ‘do_test’ at bench-strstr.c:201:5,
>     inlined from ‘test_main’ at bench-strstr.c:220:2:
> ../include/bits/../../misc/bits/error.h:42:5: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
>    42 |     __error_alias (__status, __errnum, __format, __va_arg_pack ());
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Isn't this warning wrong for glibc in general (but not for
_dl_debug_printf)?

I think printing "(null)" for null pointers is a widely-used GNU
extension.

Martin, I assume this warning is yours.  Has this matter come up during
its implementation?

Thanks,
Florian

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[H.J. Lu]

On Thu, May 23, 2019 at 12:02 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > In function ‘error’,
> >     inlined from ‘do_one_test’ at bench-strstr.c:149:7,
> >     inlined from ‘do_test’ at bench-strstr.c:201:5,
> >     inlined from ‘test_main’ at bench-strstr.c:220:2:
> > ../include/bits/../../misc/bits/error.h:42:5: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
> >    42 |     __error_alias (__status, __errnum, __format, __va_arg_pack ());
> >       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Isn't this warning wrong for glibc in general (but not for
> _dl_debug_printf)?
>
> I think printing "(null)" for null pointers is a widely-used GNU
> extension.

Only for limited cases:

[hjl@gnu-cfl-1 tmp]$ cat x.c
#include <stdio.h>

char *p;

int
main ()
{
  printf("null string:%s\n", p);
  printf ("%s\n", p);
  return 0;
}
[hjl@gnu-cfl-1 tmp]$ gcc x.c
[hjl@gnu-cfl-1 tmp]$ ./a.out
null string:(null)
Segmentation fault
[hjl@gnu-cfl-1 tmp]$

> Martin, I assume this warning is yours.  Has this matter come up during
> its implementation?
>
> Thanks,
> Florian



-- 
H.J.

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[Florian Weimer]

* H. J. Lu:

> On Thu, May 23, 2019 at 12:02 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > In function ‘error’,
>> >     inlined from ‘do_one_test’ at bench-strstr.c:149:7,
>> >     inlined from ‘do_test’ at bench-strstr.c:201:5,
>> >     inlined from ‘test_main’ at bench-strstr.c:220:2:
>> > ../include/bits/../../misc/bits/error.h:42:5: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
>> >    42 |     __error_alias (__status, __errnum, __format, __va_arg_pack ());
>> >       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Isn't this warning wrong for glibc in general (but not for
>> _dl_debug_printf)?
>>
>> I think printing "(null)" for null pointers is a widely-used GNU
>> extension.
>
> Only for limited cases:
>
> [hjl@gnu-cfl-1 tmp]$ cat x.c
> #include <stdio.h>
>
> char *p;
>
> int
> main ()
> {
>   printf("null string:%s\n", p);
>   printf ("%s\n", p);
>   return 0;
> }
> [hjl@gnu-cfl-1 tmp]$ gcc x.c
> [hjl@gnu-cfl-1 tmp]$ ./a.out
> null string:(null)
> Segmentation fault
> [hjl@gnu-cfl-1 tmp]$

Ah, because GCC transforms printf with "%s\n" to puts?  Hmm.

Thanks,
Florian

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[H.J. Lu]

On Thu, May 23, 2019 at 8:20 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Thu, May 23, 2019 at 12:02 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> > In function ‘error’,
> >> >     inlined from ‘do_one_test’ at bench-strstr.c:149:7,
> >> >     inlined from ‘do_test’ at bench-strstr.c:201:5,
> >> >     inlined from ‘test_main’ at bench-strstr.c:220:2:
> >> > ../include/bits/../../misc/bits/error.h:42:5: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
> >> >    42 |     __error_alias (__status, __errnum, __format, __va_arg_pack ());
> >> >       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> Isn't this warning wrong for glibc in general (but not for
> >> _dl_debug_printf)?
> >>
> >> I think printing "(null)" for null pointers is a widely-used GNU
> >> extension.
> >
> > Only for limited cases:
> >
> > [hjl@gnu-cfl-1 tmp]$ cat x.c
> > #include <stdio.h>
> >
> > char *p;
> >
> > int
> > main ()
> > {
> >   printf("null string:%s\n", p);
> >   printf ("%s\n", p);
> >   return 0;
> > }
> > [hjl@gnu-cfl-1 tmp]$ gcc x.c
> > [hjl@gnu-cfl-1 tmp]$ ./a.out
> > null string:(null)
> > Segmentation fault
> > [hjl@gnu-cfl-1 tmp]$
>
> Ah, because GCC transforms printf with "%s\n" to puts?  Hmm.
>

Yes.

-- 
H.J.

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[Florian Weimer]

* H. J. Lu:

> On Thu, May 23, 2019 at 8:20 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > On Thu, May 23, 2019 at 12:02 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >>
>> >> * H. J. Lu:
>> >>
>> >> > In function ‘error’,
>> >> >     inlined from ‘do_one_test’ at bench-strstr.c:149:7,
>> >> >     inlined from ‘do_test’ at bench-strstr.c:201:5,
>> >> >     inlined from ‘test_main’ at bench-strstr.c:220:2:
>> >> > ../include/bits/../../misc/bits/error.h:42:5: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
>> >> >    42 |     __error_alias (__status, __errnum, __format, __va_arg_pack ());
>> >> >       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >>
>> >> Isn't this warning wrong for glibc in general (but not for
>> >> _dl_debug_printf)?
>> >>
>> >> I think printing "(null)" for null pointers is a widely-used GNU
>> >> extension.
>> >
>> > Only for limited cases:
>> >
>> > [hjl@gnu-cfl-1 tmp]$ cat x.c
>> > #include <stdio.h>
>> >
>> > char *p;
>> >
>> > int
>> > main ()
>> > {
>> >   printf("null string:%s\n", p);
>> >   printf ("%s\n", p);
>> >   return 0;
>> > }
>> > [hjl@gnu-cfl-1 tmp]$ gcc x.c
>> > [hjl@gnu-cfl-1 tmp]$ ./a.out
>> > null string:(null)
>> > Segmentation fault
>> > [hjl@gnu-cfl-1 tmp]$
>>
>> Ah, because GCC transforms printf with "%s\n" to puts?  Hmm.
>>
>
> Yes.

We document the printf behavior:

|    If you accidentally pass a null pointer as the argument for a ‘%s’
| conversion, the GNU C Library prints it as ‘(null)’.  We think this is
| more useful than crashing.  But it’s not good practice to pass a null
| argument intentionally.

So we should perhaps fix puts to behave in the same way.  (puts isn't
even annotated with __nonnull today.)

Thanks,
Florian

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[Martin Sebor]

On 5/23/19 9:41 AM, Florian Weimer wrote:
> * H. J. Lu:
> 
>> On Thu, May 23, 2019 at 8:20 AM Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>> * H. J. Lu:
>>>
>>>> On Thu, May 23, 2019 at 12:02 AM Florian Weimer <fweimer@redhat.com> wrote:
>>>>>
>>>>> * H. J. Lu:
>>>>>
>>>>>> In function ‘error’,
>>>>>>      inlined from ‘do_one_test’ at bench-strstr.c:149:7,
>>>>>>      inlined from ‘do_test’ at bench-strstr.c:201:5,
>>>>>>      inlined from ‘test_main’ at bench-strstr.c:220:2:
>>>>>> ../include/bits/../../misc/bits/error.h:42:5: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
>>>>>>     42 |     __error_alias (__status, __errnum, __format, __va_arg_pack ());
>>>>>>        |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> Isn't this warning wrong for glibc in general (but not for
>>>>> _dl_debug_printf)?
>>>>>
>>>>> I think printing "(null)" for null pointers is a widely-used GNU
>>>>> extension.
>>>>
>>>> Only for limited cases:
>>>>
>>>> [hjl@gnu-cfl-1 tmp]$ cat x.c
>>>> #include <stdio.h>
>>>>
>>>> char *p;
>>>>
>>>> int
>>>> main ()
>>>> {
>>>>    printf("null string:%s\n", p);
>>>>    printf ("%s\n", p);
>>>>    return 0;
>>>> }
>>>> [hjl@gnu-cfl-1 tmp]$ gcc x.c
>>>> [hjl@gnu-cfl-1 tmp]$ ./a.out
>>>> null string:(null)
>>>> Segmentation fault
>>>> [hjl@gnu-cfl-1 tmp]$
>>>
>>> Ah, because GCC transforms printf with "%s\n" to puts?  Hmm.
>>>
>>
>> Yes.
> 
> We document the printf behavior:
> 
> |    If you accidentally pass a null pointer as the argument for a ‘%s’
> | conversion, the GNU C Library prints it as ‘(null)’.  We think this is
> | more useful than crashing.  But it’s not good practice to pass a null
> | argument intentionally.
> 
> So we should perhaps fix puts to behave in the same way.  (puts isn't
> even annotated with __nonnull today.)

There are two transformations that don't handle null pointers: printf
to puts and sprintf to strcpy (or memcpy).  They have been in GCC since
at least 2005, and in Clang since at least 2011.  I'd rather discourage
relying on the Glibc printf extension than remove the transformations
or suppress the warning.

Martin

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[Paul Eggert]

On 5/23/19 9:21 AM, Martin Sebor wrote:
> I'd rather discourage
> relying on the Glibc printf extension than remove the transformations
> or suppress the warning. 

I agree that this is a good way to go, as we can't keep fighting 
compilers forever. I wrote a patch implementing this suggestion and 
filed a bug report proposing it here:

https://sourceware.org/bugzilla/show_bug.cgi?id=24616

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[H.J. Lu]

On Thu, May 23, 2019 at 12:35 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 5/23/19 9:21 AM, Martin Sebor wrote:
> > I'd rather discourage
> > relying on the Glibc printf extension than remove the transformations
> > or suppress the warning.
>
> I agree that this is a good way to go, as we can't keep fighting
> compilers forever. I wrote a patch implementing this suggestion and
> filed a bug report proposing it here:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=24616
>

GCC 9 can't build bench today.  What should we do?

-- 
H.J.

Re: [PATCH] Don't pass NULL pointer to error [BZ #24556]

[Stefan Liebler]

On 6/17/19 9:32 PM, H.J. Lu wrote:
> On Thu, May 23, 2019 at 12:35 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>>
>> On 5/23/19 9:21 AM, Martin Sebor wrote:
>>> I'd rather discourage
>>> relying on the Glibc printf extension than remove the transformations
>>> or suppress the warning.
>>
>> I agree that this is a good way to go, as we can't keep fighting
>> compilers forever. I wrote a patch implementing this suggestion and
>> filed a bug report proposing it here:
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=24616
>>
> 
> GCC 9 can't build bench today.  What should we do?
> 
Sorry. I've just recognized this thread.
I've already posted a patch which also fixes "make bench" with gcc 9:
"[PATCH] Fix gcc 9 build errors for make xcheck."
https://www.sourceware.org/ml/libc-alpha/2019-06/msg00374.html

Bye
Stefan