* glibc misc/sys/cdefs.h nonull - typo in comment @ 2023-04-11 9:09 Jonny Grant 2023-04-11 13:39 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 18+ messages in thread From: Jonny Grant @ 2023-04-11 9:09 UTC (permalink / raw) To: GNU C Library 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-04-11 9:09 glibc misc/sys/cdefs.h nonull - typo in comment Jonny Grant @ 2023-04-11 13:39 ` Adhemerval Zanella Netto 2023-04-12 15:56 ` Jonny Grant 0 siblings, 1 reply; 18+ messages in thread From: Adhemerval Zanella Netto @ 2023-04-11 13:39 UTC (permalink / raw) To: Jonny Grant, GNU C Library, Paul Eggert 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-04-11 13:39 ` Adhemerval Zanella Netto @ 2023-04-12 15:56 ` Jonny Grant 2023-04-12 16:26 ` Xi Ruoyao 2023-04-12 17:28 ` Adhemerval Zanella Netto 0 siblings, 2 replies; 18+ messages in thread From: Jonny Grant @ 2023-04-12 15:56 UTC (permalink / raw) To: Adhemerval Zanella Netto, GNU C Library, Paul Eggert 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-04-12 15:56 ` Jonny Grant @ 2023-04-12 16:26 ` Xi Ruoyao 2023-04-12 16:31 ` Xi Ruoyao 2023-10-28 23:50 ` Jonny Grant 2023-04-12 17:28 ` Adhemerval Zanella Netto 1 sibling, 2 replies; 18+ messages in thread From: Xi Ruoyao @ 2023-04-12 16:26 UTC (permalink / raw) To: Jonny Grant, Adhemerval Zanella Netto, GNU C Library, Paul Eggert 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-04-12 16:26 ` Xi Ruoyao @ 2023-04-12 16:31 ` Xi Ruoyao 2023-10-28 23:50 ` Jonny Grant 1 sibling, 0 replies; 18+ messages in thread From: Xi Ruoyao @ 2023-04-12 16:31 UTC (permalink / raw) To: Jonny Grant, Adhemerval Zanella Netto, GNU C Library, Paul Eggert 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-04-12 16:26 ` Xi Ruoyao 2023-04-12 16:31 ` Xi Ruoyao @ 2023-10-28 23:50 ` Jonny Grant 2023-10-29 5:24 ` Paul Eggert 1 sibling, 1 reply; 18+ messages in thread From: Jonny Grant @ 2023-10-28 23:50 UTC (permalink / raw) To: Xi Ruoyao; +Cc: Adhemerval Zanella Netto, GNU C Library, Paul Eggert 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-10-28 23:50 ` Jonny Grant @ 2023-10-29 5:24 ` Paul Eggert 2023-10-29 22:43 ` Jonny Grant 2023-11-01 7:38 ` Florian Weimer 0 siblings, 2 replies; 18+ messages in thread From: Paul Eggert @ 2023-10-29 5:24 UTC (permalink / raw) To: Jonny Grant; +Cc: Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao 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." ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-10-29 5:24 ` Paul Eggert @ 2023-10-29 22:43 ` Jonny Grant 2023-10-30 9:04 ` Andreas Schwab 2023-11-01 7:38 ` Florian Weimer 1 sibling, 1 reply; 18+ messages in thread From: Jonny Grant @ 2023-10-29 22:43 UTC (permalink / raw) To: Paul Eggert; +Cc: Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-10-29 22:43 ` Jonny Grant @ 2023-10-30 9:04 ` Andreas Schwab 2023-10-30 10:10 ` Xi Ruoyao 0 siblings, 1 reply; 18+ messages in thread From: Andreas Schwab @ 2023-10-30 9:04 UTC (permalink / raw) To: Jonny Grant Cc: Paul Eggert, Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao 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." ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-10-30 9:04 ` Andreas Schwab @ 2023-10-30 10:10 ` Xi Ruoyao 2023-12-03 22:09 ` Jonny Grant 0 siblings, 1 reply; 18+ messages in thread From: Xi Ruoyao @ 2023-10-30 10:10 UTC (permalink / raw) To: Andreas Schwab, Jonny Grant Cc: Paul Eggert, Adhemerval Zanella Netto, GNU C Library 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-10-30 10:10 ` Xi Ruoyao @ 2023-12-03 22:09 ` Jonny Grant 0 siblings, 0 replies; 18+ messages in thread From: Jonny Grant @ 2023-12-03 22:09 UTC (permalink / raw) To: Xi Ruoyao, Andreas Schwab Cc: Paul Eggert, Adhemerval Zanella Netto, GNU C Library 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-10-29 5:24 ` Paul Eggert 2023-10-29 22:43 ` Jonny Grant @ 2023-11-01 7:38 ` Florian Weimer 2023-11-01 19:30 ` Paul Eggert 1 sibling, 1 reply; 18+ messages in thread From: Florian Weimer @ 2023-11-01 7:38 UTC (permalink / raw) To: Paul Eggert Cc: Jonny Grant, Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao * 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? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-11-01 7:38 ` Florian Weimer @ 2023-11-01 19:30 ` Paul Eggert 2023-11-01 19:52 ` Jonny Grant 2023-11-01 20:06 ` Florian Weimer 0 siblings, 2 replies; 18+ messages in thread From: Paul Eggert @ 2023-11-01 19:30 UTC (permalink / raw) To: Florian Weimer Cc: Jonny Grant, Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-11-01 19:30 ` Paul Eggert @ 2023-11-01 19:52 ` Jonny Grant 2023-11-01 20:05 ` Florian Weimer 2023-11-01 20:06 ` Florian Weimer 1 sibling, 1 reply; 18+ messages in thread From: Jonny Grant @ 2023-11-01 19:52 UTC (permalink / raw) To: Paul Eggert, Florian Weimer Cc: Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-11-01 19:52 ` Jonny Grant @ 2023-11-01 20:05 ` Florian Weimer 2023-11-01 20:16 ` Jonny Grant 0 siblings, 1 reply; 18+ messages in thread From: Florian Weimer @ 2023-11-01 20:05 UTC (permalink / raw) To: Jonny Grant Cc: Paul Eggert, Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao * 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). ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-11-01 20:05 ` Florian Weimer @ 2023-11-01 20:16 ` Jonny Grant 0 siblings, 0 replies; 18+ messages in thread From: Jonny Grant @ 2023-11-01 20:16 UTC (permalink / raw) To: Florian Weimer Cc: Paul Eggert, Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-11-01 19:30 ` Paul Eggert 2023-11-01 19:52 ` Jonny Grant @ 2023-11-01 20:06 ` Florian Weimer 1 sibling, 0 replies; 18+ messages in thread From: Florian Weimer @ 2023-11-01 20:06 UTC (permalink / raw) To: Paul Eggert Cc: Jonny Grant, Adhemerval Zanella Netto, GNU C Library, Xi Ruoyao * 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: glibc misc/sys/cdefs.h nonull - typo in comment 2023-04-12 15:56 ` Jonny Grant 2023-04-12 16:26 ` Xi Ruoyao @ 2023-04-12 17:28 ` Adhemerval Zanella Netto 1 sibling, 0 replies; 18+ messages in thread From: Adhemerval Zanella Netto @ 2023-04-12 17:28 UTC (permalink / raw) To: Jonny Grant, GNU C Library, Paul Eggert, Xi Ruoyao 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-12-03 22:09 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-11 9:09 glibc misc/sys/cdefs.h nonull - typo in comment Jonny Grant 2023-04-11 13:39 ` Adhemerval Zanella Netto 2023-04-12 15:56 ` Jonny Grant 2023-04-12 16:26 ` Xi Ruoyao 2023-04-12 16:31 ` Xi Ruoyao 2023-10-28 23:50 ` Jonny Grant 2023-10-29 5:24 ` Paul Eggert 2023-10-29 22:43 ` Jonny Grant 2023-10-30 9:04 ` Andreas Schwab 2023-10-30 10:10 ` Xi Ruoyao 2023-12-03 22:09 ` Jonny Grant 2023-11-01 7:38 ` Florian Weimer 2023-11-01 19:30 ` Paul Eggert 2023-11-01 19:52 ` Jonny Grant 2023-11-01 20:05 ` Florian Weimer 2023-11-01 20:16 ` Jonny Grant 2023-11-01 20:06 ` Florian Weimer 2023-04-12 17:28 ` Adhemerval Zanella Netto
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).