Re: strlen

["Alejandro Colomar (man-pages)" <alx.manpages@gmail.com>]

Hello Jonny,

On 7/7/21 1:36 PM, Jonny Grant wrote:
> 
> 
> On 06/07/2021 23:11, Florian Weimer wrote:
>> * Jonny Grant:
>>
>>> The reason it does not crash appears to be because of this warning
>>> which removes the call to strlen due to the return not being
>>> checked?
>>
>> GCC uses the information that the argument to strlen cannot be null on
>> that particular path.
> 
> It's a shame GCC doesn't give a warning
> 
> It may be GCC is using '__builtin_strlen'
> 
> <string.h> marks the param as nonnull. However, I am surprised this does not trigger the GCC warning -Werror=nonnull

[[gnu::nunnull]], aka __attribute__((nonnull)), is only triggered by 
compile time NULL.  That is: passing NULL (either hardcoded or through a 
macro)

If after optimizations the compiler realizes that an argument is NULL at 
compile time, it may (but also may not) trigger the warning.  However, 
if you optimize too much, the expression may vanish, and the warning be 
silenced again.  The -fanalyzer option of GCC may (but also may not) 
realize of more NULL cases.

I managed to trigger the following, but only with '-O0 -fanalyzer':

[[
<source>:17:20: error: use of NULL 's' where non-null expected [CWE-476] 
[-Werror=analyzer-null-argument]
    17 |     int x = strlen (s);
       |             ~~~~~~~^~~
   'void f(const char*)': events 1-2
     |
     |   16 |     const char *s = NULL;
     |      |                 ^
     |      |                 |
     |      |                 (1) 's' is NULL
     |   17 |     int x = strlen(s);
     |      |             ~~~~~~~~~~
     |      |                   |
     |      |                   (2) argument 1 ('s') NULL where non-null 
expected
     |
In file included from <source>:4:
/usr/include/string.h:385:15: note: argument 1 of 'size_t strlen(const 
char*)' must be non-null
   385 | extern size_t strlen (const char *__s)
       |               ^~~~~~
]]


It is undefined behavior to pass NULL to a nonnull parameter, and it is 
a problem that the caller should check before the call.  The compiler 
may warn you of the most obvious ones, but in the end, the 
responsibility is on the programmer.

See 
<https://stackoverflow.com/questions/42035769/how-to-generate-warning-of-non-null-argument-for-my-custom-function>.

> 
> /* Return the length of S.  */
> extern size_t strlen (const char *__s)
>       __THROW __attribute_pure__ __nonnull ((1));
> 
> Perhaps that is just a macro that is not actually used......

It _is_ being used:

/usr/include$ grep -rn 'define __nonnull' -B3 -A1
x86_64-linux-gnu/sys/cdefs.h-290-/* The nonull function attribute allows 
to mark pointer parameters which
x86_64-linux-gnu/sys/cdefs.h-291-   must not be NULL.  */
x86_64-linux-gnu/sys/cdefs.h-292-#if __GNUC_PREREQ (3,3)
x86_64-linux-gnu/sys/cdefs.h:293:# define __nonnull(params) 
__attribute__ ((__nonnull__ params))
x86_64-linux-gnu/sys/cdefs.h-294-#else
x86_64-linux-gnu/sys/cdefs.h:295:# define __nonnull(params)
x86_64-linux-gnu/sys/cdefs.h-296-#endif

> 
> If I add another function -Werror=nonnull does give a warning
> void test(const char * const p) __attribute__((nonnull));
> 
> https://godbolt.org/z/x37sbfWaG
> 
> <source>:15:9: error: argument 1 null where non-null expected [-Werror=nonnull]
>     15 |     test(NULL);
> 
>>
>>> strlen.c:11:3: warning: statement with no effect [-Wunused-value]
>>>     11 |   strlen (str);
>>>        |   ^~~~~~~~~~~~
>>>
>>> https://godbolt.org/z/caoes5nGa
>>
>> That site probably uses different library headers.
>>
>> As posted, with Debian's GCC 8.3, I get this for the main function:
>>
>> main:
>> 	xorl	%eax, %eax
>> 	ret
>>
> 
> 
> In that case maybe https://man7.org/linux/man-pages/man3/strlen.3.html should have a "NOTES" section stating something similar to your wording ?
> 
> NOTES
> 
> The behavior of strlen(NULL) is depends on different things. It may cause a SEGV exception, or the compiler may optimize and remove the code path handling NULL.
> 

I disagree with this.  It is likely that the behavior is that, given the 
current implementation of Linux/GCC/glibc.  But it is undefined 
behavior, and anything can happen.  You should just try harder to avoid 
it, and not rely on any possible outcome of it.  GCC people may decide 
tomorrow to change the behavior to do some more agresive optimizations, 
and the documentation shouldn't preclude such a thing, as long as it's 
legal according to the relevant standards, and sane.


Cheers,

Alex


-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/