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

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

[Jonny Grant]

Hi

There are two small changes I suggest if someone has a moment?

misc/sys/cdefs.h  nonull  - typo in comment
Should be "nonnull"

Also

/posix/glob.c has an unused macro
# define _GL_ARG_NONNULL(params)

Could that be removed?

Seems _GL_ARG_NONNULL is only used in misc/error.c which again just compiles it out. So maybe it can be removed overall in both files?

Kind regards
Jonny

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

[Adhemerval Zanella Netto]

On 11/04/23 06:09, Jonny Grant wrote:
> Hi
> 
> There are two small changes I suggest if someone has a moment?
> 
> misc/sys/cdefs.h  nonull  - typo in comment
> Should be "nonnull"

This is already being fixed by c8ba52ab3350c334d (2.34).

> 
> Also
> 
> /posix/glob.c has an unused macro
> # define _GL_ARG_NONNULL(params)
> 
> Could that be removed?

This definition is used by gnulib code, since it defined for !_LIBC,
Different than glibc, gnulib defines the function as:

lib/glob.in.h

103 #if @GNULIB_GLOB@
104 # if @REPLACE_GLOB@
105 _GL_FUNCDECL_RPL (glob, int, (const char *_Restrict_ __pattern, int __flags,
106                               _gl_glob_errfunc_fn __errfunc,
107                               glob_t *_Restrict_ __pglob)
108                               _GL_ARG_NONNULL ((1)));
109 _GL_CXXALIAS_RPL (glob, int, (const char *_Restrict_ __pattern, int __flags,
110                               _gl_glob_errfunc_fn __errfunc,
111                               glob_t *_Restrict_ __pglob));
112 # else
113 #  if !@HAVE_GLOB@
114 _GL_FUNCDECL_SYS (glob, int, (const char *_Restrict_ __pattern, int __flags,
115                               _gl_glob_errfunc_fn __errfunc,
116                               glob_t *_Restrict_ __pglob)
117                               _GL_ARG_NONNULL ((1)));
118 #  endif
119 _GL_CXXALIAS_SYS (glob, int, (const char *_Restrict_ __pattern, int __flags,
120                               _gl_glob_errfunc_fn __errfunc,
121                               glob_t *_Restrict_ __pglob));
122 # endif

Which then at the implementation is also check for pattern == NULL:

lib/glob.c:

 316   if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0)
 317     {
 318       __set_errno (EINVAL);
 319       return -1;
 320     }

So the comment is right that compiler might indeed remove the test. 
Different than gnulib, glibc prototype does not add the 
__attribute__ ((nonnul)).

> 
> Seems _GL_ARG_NONNULL is only used in misc/error.c which again just compiles it out. So maybe it can be removed overall in both files?

This was added as a sync with gnulib code by 888c679ba40, because it is
used in error_tail definition and glibc does not define it.  So we can not
remove without also adjusting error_tail, and since the code is originally
from gnulib maybe it would be better to use __nonnull macro.

> 
> Kind regards
> Jonny

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

[Jonny Grant]

Hi Adhemerval

On 11/04/2023 14:39, Adhemerval Zanella Netto wrote:
> 
> 
> On 11/04/23 06:09, Jonny Grant wrote:
>> Hi
>>
>> There are two small changes I suggest if someone has a moment?
>>
>> misc/sys/cdefs.h  nonull  - typo in comment
>> Should be "nonnull"
> 
> This is already being fixed by c8ba52ab3350c334d (2.34).

That's great news!

I was interested to have a look through the history, and see the changes.

Sorry I am bit new to sourceware git repo browser. 
May I ask, is there an easy way to look up that hex? It looks shorter than usual commit hex strings.

The way I found the change was to use https://sourceware.org/git/?p=glibc.git  "grep" search box with "c8ba52ab3350c334d" which found the ChangeLog with c8ba52ab3350c334d6e34b1439a4c0c1431351f3

Then I found another commit, and added to the URL
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=

https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=c8ba52ab3350c334d6e34b1439a4c0c1431351f3


>> Also
>>
>> /posix/glob.c has an unused macro
>> # define _GL_ARG_NONNULL(params)
>>
>> Could that be removed?
> 
> This definition is used by gnulib code, since it defined for !_LIBC,
> Different than glibc, gnulib defines the function as:
> 
> lib/glob.in.h
> 
> 103 #if @GNULIB_GLOB@
> 104 # if @REPLACE_GLOB@
> 105 _GL_FUNCDECL_RPL (glob, int, (const char *_Restrict_ __pattern, int __flags,
> 106                               _gl_glob_errfunc_fn __errfunc,
> 107                               glob_t *_Restrict_ __pglob)
> 108                               _GL_ARG_NONNULL ((1)));
> 109 _GL_CXXALIAS_RPL (glob, int, (const char *_Restrict_ __pattern, int __flags,
> 110                               _gl_glob_errfunc_fn __errfunc,
> 111                               glob_t *_Restrict_ __pglob));
> 112 # else
> 113 #  if !@HAVE_GLOB@
> 114 _GL_FUNCDECL_SYS (glob, int, (const char *_Restrict_ __pattern, int __flags,
> 115                               _gl_glob_errfunc_fn __errfunc,
> 116                               glob_t *_Restrict_ __pglob)
> 117                               _GL_ARG_NONNULL ((1)));
> 118 #  endif
> 119 _GL_CXXALIAS_SYS (glob, int, (const char *_Restrict_ __pattern, int __flags,
> 120                               _gl_glob_errfunc_fn __errfunc,
> 121                               glob_t *_Restrict_ __pglob));
> 122 # endif
> 
> Which then at the implementation is also check for pattern == NULL:
> 
> lib/glob.c:
> 
>  316   if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0)
>  317     {
>  318       __set_errno (EINVAL);
>  319       return -1;
>  320     }
> 
> So the comment is right that compiler might indeed remove the test. 
> Different than gnulib, glibc prototype does not add the 
> __attribute__ ((nonnul)).
> 
>>
>> Seems _GL_ARG_NONNULL is only used in misc/error.c which again just compiles it out. So maybe it can be removed overall in both files?
> 
> This was added as a sync with gnulib code by 888c679ba40, because it is
> used in error_tail definition and glibc does not define it.  So we can not
> remove without also adjusting error_tail, and since the code is originally
> from gnulib maybe it would be better to use __nonnull macro.

Thank you for your reply with information. I'll read up a bit more next time before asking!

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

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.
   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.  */


Kind regards, Jonny

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

[Xi Ruoyao]

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.

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".

> 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.

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;
}

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".

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

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

[Xi Ruoyao]

On Thu, 2023-04-13 at 00:26 +0800, Xi Ruoyao via Libc-alpha wrote:
> size_t
> f (const char *s)
> {
>   return s ? strlen (r) : 0;

Sorry, obviously I mean "strlen (s)" here.

> }

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

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

[Jonny Grant]

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

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

[Paul Eggert]

On 2023-10-28 16:50, Jonny Grant wrote:
> Could you give an example of a POSIX API text you refer to that 
> specifies many arguments should not be NULL?

"If an argument to a function has an invalid value (such as a value 
outside the domain of the function, or a pointer outside the address 
space of the program, or a null pointer), the behavior is undefined."

https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/V2_chap02.html#tag_15_01_01

This wording is copied from the C Standard.


The April 2023 working draft of C23 has adjusted the wording to be the 
following, and I expect POSIX to follow suit eventually. Notice the new 
restrictions:

"If an argument to a function has an invalid value (such as a value 
outside the domain of the function, or a pointer outside the address 
space of the program, or a null pointer, or a pointer to non-modifiable 
storage when the corresponding parameter is not const-qualified) or a 
type (after default argument promotion) not expected by a function with 
a variable number of arguments, the behavior is undefined.

"If a function argument is described as being an array, the pointer 
actually passed to the function shall have a value such that all address 
computations and accesses to objects (that would be valid if the pointer 
did point to the first element of such an array) are in fact valid.[210]

"[210] This includes, for example, passing a valid pointer that points 
one-past-the-end of an array along with a size of 0, or using any valid 
pointer with a size of 0."

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

[Jonny Grant]

On 29/10/2023 05:24, Paul Eggert wrote:
> On 2023-10-28 16:50, Jonny Grant wrote:
>> Could you give an example of a POSIX API text you refer to that specifies many arguments should not be NULL?
> 
> "If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer), the behavior is undefined."
> 
> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/V2_chap02.html#tag_15_01_01

Thank you for sharing the link. Yes, I've seen that everything not detailed on a particular function description would be UB.

glibc does go beyond POSIX and set errno to EFAULT if a null pointer constant is passed.
https://man7.org/linux/man-pages/man2/olduname.2.html

Although I looked at glibc/posix/uname.c and it has EINVAL there, couldn't spot where the EFAULT comes from, probably there is another file.

The POSIX pages don't specify any error checking for uname().
https://man7.org/linux/man-pages/man3/uname.3p.html
https://pubs.opengroup.org/onlinepubs/009604599/functions/uname.html

It might be too difficult to get behaviors described for the null pointer constant in the POSIX standard for something like uname().

Other functions do check parameters, like the way write() checks fd, and setting errno EBADF if it's not a valid file descriptor.

> 
> This wording is copied from the C Standard.
> 
> 
> The April 2023 working draft of C23 has adjusted the wording to be the following, and I expect POSIX to follow suit eventually. Notice the new restrictions:
> 
> "If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer, or a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) or a type (after default argument promotion) not expected by a function with a variable number of arguments, the behavior is undefined.
> 
> "If a function argument is described as being an array, the pointer actually passed to the function shall have a value such that all address computations and accesses to objects (that would be valid if the pointer did point to the first element of such an array) are in fact valid.[210]
> 
> "[210] This includes, for example, passing a valid pointer that points one-past-the-end of an array along with a size of 0, or using any valid pointer with a size of 0."

It is good it is being clarified further. 

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

[Andreas Schwab]

On Okt 29 2023, Jonny Grant wrote:

> glibc does go beyond POSIX and set errno to EFAULT if a null pointer constant is passed.
> https://man7.org/linux/man-pages/man2/olduname.2.html
>
> Although I looked at glibc/posix/uname.c and it has EINVAL there, couldn't spot where the EFAULT comes from, probably there is another file.

glibc never generates EFAULT on its own, it always comes from the
kernel.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Xi Ruoyao]

On Mon, 2023-10-30 at 10:04 +0100, Andreas Schwab wrote:
> On Okt 29 2023, Jonny Grant wrote:
> 
> > glibc does go beyond POSIX and set errno to EFAULT if a null pointer
> > constant is passed.
> > https://man7.org/linux/man-pages/man2/olduname.2.html
> > 
> > Although I looked at glibc/posix/uname.c and it has EINVAL there,
> > couldn't spot where the EFAULT comes from, probably there is another
> > file.
> 
> glibc never generates EFAULT on its own, it always comes from the
> kernel.

And POSIX is clear that people should not rely on EFAULT, at all:

[EFAULT]
Bad address. The system detected an invalid address in attempting to use
an argument of a call. The reliable detection of this error cannot be
guaranteed, and when not detected may result in the generation of a
signal, indicating an address violation, which is sent to the process.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

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

[Jonny Grant]

On 30/10/2023 10:10, Xi Ruoyao wrote:
> On Mon, 2023-10-30 at 10:04 +0100, Andreas Schwab wrote:
>> On Okt 29 2023, Jonny Grant wrote:
>>
>>> glibc does go beyond POSIX and set errno to EFAULT if a null pointer
>>> constant is passed.
>>> https://man7.org/linux/man-pages/man2/olduname.2.html
>>>
>>> Although I looked at glibc/posix/uname.c and it has EINVAL there,
>>> couldn't spot where the EFAULT comes from, probably there is another
>>> file.
>>
>> glibc never generates EFAULT on its own, it always comes from the
>> kernel.
> 
> And POSIX is clear that people should not rely on EFAULT, at all:
> 
> [EFAULT]
> Bad address. The system detected an invalid address in attempting to use
> an argument of a call. The reliable detection of this error cannot be
> guaranteed, and when not detected may result in the generation of a
> signal, indicating an address violation, which is sent to the process.
> 

Fair enough, as you and Andreas have said, the EFAULT is returned due to the kernel and POSIX doesn't state many functions that actually specify supporting a NULL pointer argument [a few eg. free(), realloc(), time()].

I usually just wrap calls to add extra checks when I'm not in control of how safe the rest of the user space software is, it is more tricky if 3rd party libraries are using symbols from libc directly.

Kind regards, Jonny

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

[Florian Weimer]

* Paul Eggert:

> The April 2023 working draft of C23 has adjusted the wording to be the 
> following, and I expect POSIX to follow suit eventually. Notice the new 
> restrictions:
>
> "If an argument to a function has an invalid value (such as a value 
> outside the domain of the function, or a pointer outside the address 
> space of the program, or a null pointer, or a pointer to non-modifiable 
> storage when the corresponding parameter is not const-qualified) or a 
> type (after default argument promotion) not expected by a function with 
> a variable number of arguments, the behavior is undefined.
>
> "If a function argument is described as being an array, the pointer 
> actually passed to the function shall have a value such that all address 
> computations and accesses to objects (that would be valid if the pointer 
> did point to the first element of such an array) are in fact valid.[210]
>
> "[210] This includes, for example, passing a valid pointer that points 
> one-past-the-end of an array along with a size of 0, or using any valid 
> pointer with a size of 0."

I'm not sure if these are new restrictions.  Doesn't this make
previously undefined behavior when calling strncmp with shorter
strings defined?

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

[Paul Eggert]

On 2023-11-01 00:38, Florian Weimer wrote:
> * Paul Eggert:
> 
>> The April 2023 working draft of C23 has adjusted the wording to be the
>> following, and I expect POSIX to follow suit eventually. Notice the new
>> restrictions:
>>
>> "If an argument to a function has an invalid value (such as a value
>> outside the domain of the function, or a pointer outside the address
>> space of the program, or a null pointer, or a pointer to non-modifiable
>> storage when the corresponding parameter is not const-qualified) or a
>> type (after default argument promotion) not expected by a function with
>> a variable number of arguments, the behavior is undefined.
>>
>> "If a function argument is described as being an array, the pointer
>> actually passed to the function shall have a value such that all address
>> computations and accesses to objects (that would be valid if the pointer
>> did point to the first element of such an array) are in fact valid....
> 
> I'm not sure if these are new restrictions.  Doesn't this make
> previously undefined behavior when calling strncmp with shorter
> strings defined?

I don't see why. The old wording (quoted below) gives examples of 
invalid values that can be summarized as "A or B or C". The first 
paragraph of the new wording (quoted above) gives examples that can be 
summarized as "A or B or C or D or E".

Strictly speaking the new wording is just a clarification of the old, 
and the "such as" in the wording means that the standard continues to be 
deliberately vague about exactly which function arguments are "invalid". 
Still, the intent is that the standardizers wanted to state clearly in 
the new standard that some things are undefined, even if the previous 
standard didn't clearly say that.

Here's the old wording:

"If an argument to a function has an invalid value (such as a value 
outside the domain of the function, or a pointer outside the address 
space of the program, or a null pointer), the behavior is undefined."

The new wording adds the following examples of invalid arguments that 
cause undefined behavior:

* a pointer to non-modifiable storage when the corresponding parameter 
is not const-qualified

* a type (after default argument promotion) not expected by a function 
with a variable number of arguments

The first new example means, for example, memset ("", 0, 0) has 
undefined behavior. The second one means, for example, printf ("%ld", 0) 
has undefined behavior.

As far as I can see, the changes to the standard are orthogonal to the 
funny business with strncmp, because strncmp's arguments are not 
described as being arrays of N bytes. The standard uses different 
wording for strncmp vs memcmp and although the wording is remarkably 
unclear, surely the intent is that memcmp's arguments are arrays of N 
bytes whereas strncmp's arguments are arrays that are at most N bytes 
and are null-terminated if they are shorter than N bytes.

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

[Jonny Grant]

On 01/11/2023 19:30, Paul Eggert wrote:
> On 2023-11-01 00:38, Florian Weimer wrote:
>> * Paul Eggert:
>>
>>> The April 2023 working draft of C23 has adjusted the wording to be the
>>> following, and I expect POSIX to follow suit eventually. Notice the new
>>> restrictions:
>>>
>>> "If an argument to a function has an invalid value (such as a value
>>> outside the domain of the function, or a pointer outside the address
>>> space of the program, or a null pointer, or a pointer to non-modifiable
>>> storage when the corresponding parameter is not const-qualified) or a
>>> type (after default argument promotion) not expected by a function with
>>> a variable number of arguments, the behavior is undefined.
>>>
>>> "If a function argument is described as being an array, the pointer
>>> actually passed to the function shall have a value such that all address
>>> computations and accesses to objects (that would be valid if the pointer
>>> did point to the first element of such an array) are in fact valid....
>>
>> I'm not sure if these are new restrictions.  Doesn't this make
>> previously undefined behavior when calling strncmp with shorter
>> strings defined?
> 
> I don't see why. The old wording (quoted below) gives examples of invalid values that can be summarized as "A or B or C". The first paragraph of the new wording (quoted above) gives examples that can be summarized as "A or B or C or D or E".
> 
> Strictly speaking the new wording is just a clarification of the old, and the "such as" in the wording means that the standard continues to be deliberately vague about exactly which function arguments are "invalid". Still, the intent is that the standardizers wanted to state clearly in the new standard that some things are undefined, even if the previous standard didn't clearly say that.
> 
> Here's the old wording:
> 
> "If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer), the behavior is undefined."
> 
> The new wording adds the following examples of invalid arguments that cause undefined behavior:
> 
> * a pointer to non-modifiable storage when the corresponding parameter is not const-qualified
> 
> * a type (after default argument promotion) not expected by a function with a variable number of arguments
> 
> The first new example means, for example, memset ("", 0, 0) has undefined behavior. The second one means, for example, printf ("%ld", 0) has undefined behavior.
> 
> As far as I can see, the changes to the standard are orthogonal to the funny business with strncmp, because strncmp's arguments are not described as being arrays of N bytes. The standard uses different wording for strncmp vs memcmp and although the wording is remarkably unclear, surely the intent is that memcmp's arguments are arrays of N bytes whereas strncmp's arguments are arrays that are at most N bytes and are null-terminated if they are shorter than N bytes.

As I read it, the C standard wording doesn't make it 100% clear for readers that strcmp or strncmp will stop comparing when it reads a null terminating byte, in either, or both strings. It is probably implied from other description of C strings in the standard. Perhaps the standard could be clarified.
Jonny

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

[Florian Weimer]

* Jonny Grant:

> As I read it, the C standard wording doesn't make it 100% clear for
> readers that strcmp or strncmp will stop comparing when it reads a
> null terminating byte, in either, or both strings. It is probably
> implied from other description of C strings in the standard. Perhaps
> the standard could be clarified.

But strncmp doesn't operate on strings.  The standard talks about
arrays, which makes sense given the historic usage (fixed-length
strings).

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

[Jonny Grant]

On 01/11/2023 20:05, Florian Weimer wrote:
> * Jonny Grant:
> 
>> As I read it, the C standard wording doesn't make it 100% clear for
>> readers that strcmp or strncmp will stop comparing when it reads a
>> null terminating byte, in either, or both strings. It is probably
>> implied from other description of C strings in the standard. Perhaps
>> the standard could be clarified.
> 
> But strncmp doesn't operate on strings.  The standard talks about
> arrays, which makes sense given the historic usage (fixed-length
> strings).

My apologies.

C23 7.26.4.4 strncmp does indeed state 'array', and it does stop at the null terminator.

C23 7.26.4.2 strcmp states 'string'.

Jonny

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

[Florian Weimer]

* Paul Eggert:

> On 2023-11-01 00:38, Florian Weimer wrote:
>> * Paul Eggert:
>> 
>>> The April 2023 working draft of C23 has adjusted the wording to be the
>>> following, and I expect POSIX to follow suit eventually. Notice the new
>>> restrictions:
>>>
>>> "If an argument to a function has an invalid value (such as a value
>>> outside the domain of the function, or a pointer outside the address
>>> space of the program, or a null pointer, or a pointer to non-modifiable
>>> storage when the corresponding parameter is not const-qualified) or a
>>> type (after default argument promotion) not expected by a function with
>>> a variable number of arguments, the behavior is undefined.
>>>
>>> "If a function argument is described as being an array, the pointer
>>> actually passed to the function shall have a value such that all address
>>> computations and accesses to objects (that would be valid if the pointer
>>> did point to the first element of such an array) are in fact valid....
>> 
>> I'm not sure if these are new restrictions.  Doesn't this make
>> previously undefined behavior when calling strncmp with shorter
>> strings defined?
>
> I don't see why. The old wording (quoted below) gives examples of 
> invalid values that can be summarized as "A or B or C". The first 
> paragraph of the new wording (quoted above) gives examples that can be 
> summarized as "A or B or C or D or E".

Okay, I was confused.  The array part is already in C99, it seems.

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

[Adhemerval Zanella Netto]

On 12/04/23 12:56, Jonny Grant wrote:
> Hi Adhemerval
> 
> On 11/04/2023 14:39, Adhemerval Zanella Netto wrote:
>>
>>
>> On 11/04/23 06:09, Jonny Grant wrote:
>>> Hi
>>>
>>> There are two small changes I suggest if someone has a moment?
>>>
>>> misc/sys/cdefs.h  nonull  - typo in comment
>>> Should be "nonnull"
>>
>> This is already being fixed by c8ba52ab3350c334d (2.34).
> 
> That's great news!
> 
> I was interested to have a look through the history, and see the changes.
> 
> Sorry I am bit new to sourceware git repo browser. 
> May I ask, is there an easy way to look up that hex? It looks shorter than usual commit hex strings.
> 
> The way I found the change was to use https://sourceware.org/git/?p=glibc.git  "grep" search box with "c8ba52ab3350c334d" which found the ChangeLog with c8ba52ab3350c334d6e34b1439a4c0c1431351f3
> 
> Then I found another commit, and added to the URL
> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=
> 
> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=c8ba52ab3350c334d6e34b1439a4c0c1431351f3
> 

I would suggest to track https://sourceware.org/git/?p=glibc.git, it is the
official glibc repo and the used for development.  And usually I check for
such changes using the normal git tools (blame, history, etc.), and 
occasionally with some graphical tool to get a more comprehensible history
view (gitk, etc.).

> 
>>> Also
>>>
>>> /posix/glob.c has an unused macro
>>> # define _GL_ARG_NONNULL(params)
>>>
>>> Could that be removed?
>>
>> This definition is used by gnulib code, since it defined for !_LIBC,
>> Different than glibc, gnulib defines the function as:
>>
>> lib/glob.in.h
>>
>> 103 #if @GNULIB_GLOB@
>> 104 # if @REPLACE_GLOB@
>> 105 _GL_FUNCDECL_RPL (glob, int, (const char *_Restrict_ __pattern, int __flags,
>> 106                               _gl_glob_errfunc_fn __errfunc,
>> 107                               glob_t *_Restrict_ __pglob)
>> 108                               _GL_ARG_NONNULL ((1)));
>> 109 _GL_CXXALIAS_RPL (glob, int, (const char *_Restrict_ __pattern, int __flags,
>> 110                               _gl_glob_errfunc_fn __errfunc,
>> 111                               glob_t *_Restrict_ __pglob));
>> 112 # else
>> 113 #  if !@HAVE_GLOB@
>> 114 _GL_FUNCDECL_SYS (glob, int, (const char *_Restrict_ __pattern, int __flags,
>> 115                               _gl_glob_errfunc_fn __errfunc,
>> 116                               glob_t *_Restrict_ __pglob)
>> 117                               _GL_ARG_NONNULL ((1)));
>> 118 #  endif
>> 119 _GL_CXXALIAS_SYS (glob, int, (const char *_Restrict_ __pattern, int __flags,
>> 120                               _gl_glob_errfunc_fn __errfunc,
>> 121                               glob_t *_Restrict_ __pglob));
>> 122 # endif
>>
>> Which then at the implementation is also check for pattern == NULL:
>>
>> lib/glob.c:
>>
>>  316   if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0)
>>  317     {
>>  318       __set_errno (EINVAL);
>>  319       return -1;
>>  320     }
>>
>> So the comment is right that compiler might indeed remove the test. 
>> Different than gnulib, glibc prototype does not add the 
>> __attribute__ ((nonnul)).
>>
>>>
>>> Seems _GL_ARG_NONNULL is only used in misc/error.c which again just compiles it out. So maybe it can be removed overall in both files?
>>
>> This was added as a sync with gnulib code by 888c679ba40, because it is
>> used in error_tail definition and glibc does not define it.  So we can not
>> remove without also adjusting error_tail, and since the code is originally
>> from gnulib maybe it would be better to use __nonnull macro.
> 
> Thank you for your reply with information. I'll read up a bit more next time before asking!
> 
> Do you think it is risky to have the nonnull attribute in use in glibc?

It reasonable to expect a non-null argument for glob, however glibc will
need to carry a similar hack from gnulib to keep the test for compatibility
(so program built against old header does not trigger invalid memory header).

> 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

This does not seem to be a good example to not use the nonnull attribute,
since from the brief discussion that nonnull attribute was added later
after API was initially set.  It means that if you have callers that rely
on the null check, if just add the nonnull attribute and remove the check
you are essentially breaking the API without either suppressing the attribute
on function implementation or providing a compatibility symbol (which is not
even widely supported on ELF world).

And as Xi Ruoyao has added, if your define your API to accept null argument
and use nonnull attribute this is an bug.

> 
> 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.
>    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.  */
> 
> 
> Kind regards, Jonny