Re: glibc misc/sys/cdefs.h nonull - typo in comment

[Jonny Grant <jg@jguk.org>]

On Wed, 12 Apr 2023 at 17:26, Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Wed, 2023-04-12 at 16:56 +0100, Jonny Grant wrote:
>
> > Do you think it is risky to have the nonnull attribute in use in glibc?
> > Some projects have abandoned attribute nonnull due to the GCC optimizer using the nonnull attribute to remove the runtime null pointer checks.
> > https://gitlab.com/gnuwget/wget2/-/issues/200
>
> No because in the C standard & POSIX standard the text is clear that
> many arguments just should not be NULL.  So the implementation does not
> need to check if such an argument is NULL.

Could you give an example of a POSIX API text you refer to that
specifies many arguments should not be NULL? I recall they seem to
omit mentioning, just implying it must be a valid pointer. Take
puts(), it doesn't mention
https://pubs.opengroup.org/onlinepubs/9699919799/functions/puts.html

But I hear you, I get it, these APIs have been like this for since the
beginning requiring a valid pointer. So passing a NULL pointer is UB
and will likely crash, which is what we would try to avoid by wrapping
library function calls.

> If the standard allows an argument to be NULL but there is attribute
> nonnull there, it's a bug.  Please open a ticket in
> https://sourceware.org/bugzilla if you find such a bug.  But when there
> is no bugs, we have no reason to remove avoid nonnull for "comforting
> some people".

If I see any, I will be sure to file a bug report.
We generally would wrap library functions on application side to check
for NULL for functional safety.

>
> > Currently cdefs.h has
> >
> > /* The nonnull function attribute marks pointer parameters that
> >    must not be NULL.  This has the name __nonnull in glibc,
> >    and __attribute_nonnull__ in files shared with Gnulib to avoid
> >    collision with a different __nonnull in DragonFlyBSD 5.9.  */
> >
> > Is it better to clarify this to be something like the following?
> >
> > /* The nonnull function attribute marks pointer parameters that
> >    the compiler's optimizer knows will never be NULL. This means NULL checks can be optimized out.
>
> No because such NULL checks which can be removed by nonnull attribute
> should not exist in Glibc.  If any one exists, it's a bug (CWE-1164
> "Irrelevant Code").  Again open a ticket if you find one.  But I guess
> you won't find any, because such a bug will be rejected by "gcc -Wall -
> Werror" and there are many people building Glibc with this.
>
> And the user should not assume any implementation details in Glibc
> functions.  For example:
>
> size_t
> f (const char *s)
> {
>   size_t r = strlen (s);
>   return s ? r : 0;
> }
>
> will do strange things no matter if nonnull is used for strlen.  Even if
> you remove the nonnull attribute of strlen and expect this thing to
> work, the different implementations (in many Glibc ports, heavily
> optimized assembly code) can still do unexpected things.  The people
> writing the implementation of strlen will assume the argument is not
> null anyway because the standard says so.

Could you share the page of the standard you are referring to please?

POSIX standard doesn't mention on the strlen page:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/strlen.html

C99 7.21.6.3 strlen doesn't mention.

>
> So this is just broken in the nature, regardless of nonnull or not.
>
> OTOH
>
> size_t
> f (const char *s)
> {
>   return s ? strlen (r) : 0;
> }

I get it, the condition is checked before the call to strlen() that
doesn't check for the null pointer constant.

> is well-defined.  The compiler is prohibited from removing the check, no
> matter if nonnull is used for strlen.  If the compiler removed the
> check, it's a bug.  Again if there is a bug, report it; but if there is
> no bugs, you cannot tell people to "fix" it just because "it seems
> risky".
>
> Anyway Glibc (and GCC) are C implementations for real C programs, not
> for "allowing people who don't know C to programming in C".

Popular libraries, even glibc from time to time suffer crash issues
and vulnerabilities.

In this thread I was speaking about nonnull removing null pointer
checks, I get it that it's nearly impossible to change a POSIX API.
For new APIs it would be useful to clarify in the specification that
pointers should be checked against the null pointer constant to meet
functional safety requirements.
Of course we can easily wrap all function calls for libraries we don't
maintain if UB can't be changed (I can't imagine anyone is relying
upon passing NULL to puts() etc?).

getdomainname() and uname()  APIs are well specified to check for the
null pointer constant.

Regards, Jonny