* Re: [PATCH] Increase judgment on buf. [not found] <lxnjka-9sevacf455zj-1fthj246gvr4-712jsi8w59t4969pqyxmxkaq-l3n4z6dp0ybpuvpiuudtnfot-dl7onwkii2tq8gtprte6cu4fgip6f-ikuh33-a5p9ixcl44cx2h7mimcgl3xt-13bjom.1684553069255@email.android.com> @ 2023-05-20 10:19 ` Xi Ruoyao 2023-05-21 8:19 ` Paul Eggert 0 siblings, 1 reply; 9+ messages in thread From: Xi Ruoyao @ 2023-05-20 10:19 UTC (permalink / raw) To: lixing, Carlos O'Donell, Adhemerval Zanella Netto, Peng Fan Cc: libc-alpha Resend because HTML seems rejected by the list. Sorry for top posting but my mobile phone is too stupid to format the reply properly. Glibc is not "what somebody wants" and it's not a bug nor something we should change. In the consensus page we say: * If you have no contract to accept NULL and you don't immediately dereference the pointer then use an assert to raise an error when NULL is passed as an invalid argument. * If you have no contract to accept NULL and immediately dereference the pointer then the segfault is sufficient to indicate the error. In this case invoking the syscall is a immediate dereference (well, if it's not clear we may edit the wiki page to say "including a dereference in a syscall" or something), and the segfault indicates the error properly. Nevertheless, even if the syscall does not segfault, according to our policy we should deliberately trigger a segfault for NULL input, not returning EFAULT (unless the standard says we should return EFAULT). If a LTP test relies on a EFAULT here, then LTP has a bug. You should tell LTP to fix it then. And adding NULL check here would be a beginning of a sequence of very very bad moves. Should we start to add NULL checks everywhere, say "read", "strlen", "fclose" etc.? Yes some (stupid) textbook says we should do this but let's burn such a textbook in fire. They were written in 80's or 90's when memory errors were everwhere (and a memory error was considered "not fatal"), but today memory errors are considered security vulnerabilities. Allowing a vulnerable program continuing to run may make the vulnerability more severe (crashing is a Deniel of Service, but running with a broken state may lead to Arbitary Code Execution). Glibc is a library implementing the ISO C langauage library and POSIX functions, for supporting real softwares, not for "allowing people who don't know C or POSIX to program". -------- 原始邮件 -------- 发件人: lixing <lixing@loongson.cn> 日期: 2023年5月20日周六 早上8:29 收件人: Carlos O'Donell <carlos@redhat.com>, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>, Peng Fan <fanpeng@loongson.cn>, libc-alpha@sourceware.org 抄送: Xi Ruoyao <xry111@xry111.site> 主 题: Re: [PATCH] Increase judgment on buf. > > 在 2023/5/19 下午7:55, Carlos O'Donell via Libc-alpha 写道: > > On 5/19/23 07:48, Adhemerval Zanella Netto via Libc-alpha wrote: > >> > >> On 19/05/23 00:57, Peng Fan wrote: > >>> When buf is empty, if it is not checked, the subsequent assignment > >>> operation will trigger a page fault. This is unnecessary. > >>> > >>> Signed-off-by: lixing <lixing@loongson.cn> > >>> Signed-off-by: Peng Fan <fanpeng@loongson.cn> > >> The stat family is explicitly marked with nonnull for the input > struct > >> stat buffer, and calling with a NULL argument is an UB. > > Agreed, and "Style and Conventions" > > https://sourceware.org/glibc/wiki/Style_and_Conventions > > says: > > > https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program > > > > We should fail catastrophically and early in the case of user bugs. > > The segfault generates a core dump at exactly the right point to > debug the UB. > > Yes, LTP fstat03 test pass the buf with NULL. We just want to fail > earlier during the syscall statx with return value EFAULT if the buf > is > NULL, > > but not pass the fault to the struct assignment which trigger SIGSEGV. > > >>> --- > >>> sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++-- > >>> 1 file changed, 6 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c > b/sysdeps/unix/sysv/linux/fstatat64.c > >>> index 3509d3ca6d..b635a8299a 100644 > >>> --- a/sysdeps/unix/sysv/linux/fstatat64.c > >>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c > >>> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char > *file, struct __stat64_t64 *buf, > >>> { > >>> /* 32-bit kABI with default 64-bit time_t, e.g. arc, > riscv32. Also > >>> 64-bit time_t support is done through statx syscall. */ > >>> - struct statx tmp; > >>> + struct statx tmp, *ptr; > >>> + if (buf) > >>> + ptr = &tmp; > >>> + else > >>> + ptr = NULL; > >>> int r = INTERNAL_SYSCALL_CALL (statx, fd, file, > AT_NO_AUTOMOUNT | flag, > >>> - STATX_BASIC_STATS, &tmp); > >>> + STATX_BASIC_STATS, ptr); > >>> if (r != 0) > >>> return r; > >>> > -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Increase judgment on buf. 2023-05-20 10:19 ` [PATCH] Increase judgment on buf Xi Ruoyao @ 2023-05-21 8:19 ` Paul Eggert 2023-05-21 8:27 ` Andreas Schwab 0 siblings, 1 reply; 9+ messages in thread From: Paul Eggert @ 2023-05-21 8:19 UTC (permalink / raw) To: Xi Ruoyao, lixing, Carlos O'Donell, Adhemerval Zanella Netto, Peng Fan Cc: libc-alpha, ltp [-- Attachment #1: Type: text/plain, Size: 513 bytes --] On 2023-05-20 03:19, Xi Ruoyao via Libc-alpha wrote: > If a LTP test relies on a EFAULT here, then LTP has a bug. You should > tell LTP to fix it then. LTP fstat03 allows either EFAULT or SIGSEGV, but no other behavior, when 'stat' is passed a null pointer. Therefore that test is too strict for POSIX, as the POSIX behavior is undefined. It's also too strict for GNU/Linux, because it insists on SIGSEGV whereas any terminating signal will do (e.g., SIGABRT). Proposed LTP patch attached. [-- Attachment #2: 0001-fstat03-wrongly-required-SIGSEGV.patch --] [-- Type: text/x-patch, Size: 1490 bytes --] From 8a2b3f75d2b61e5e4ec0a2c93f3db62584daf1cd Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Sun, 21 May 2023 01:15:40 -0700 Subject: [PATCH] fstat03 wrongly required SIGSEGV Change fstat03 to allow the syscall to signal SIGABRT (or any other signal) instead of SIGSEGV when a null pointer is passed, as POSIX says behavior is undefined and the glibc behavior (which is a bit better defined) could be any signal, not necessarily SIGSEGV. Signed-off-by: Paul Eggert <eggert@cs.ucla.edu> --- testcases/kernel/syscalls/fstat/fstat03.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/testcases/kernel/syscalls/fstat/fstat03.c b/testcases/kernel/syscalls/fstat/fstat03.c index 4ff37e882..a400692e9 100644 --- a/testcases/kernel/syscalls/fstat/fstat03.c +++ b/testcases/kernel/syscalls/fstat/fstat03.c @@ -10,7 +10,7 @@ * 1) Calls fstat() with closed file descriptor * -> EBADF * 2) Calls fstat() with an invalid address for stat structure - * -> EFAULT (or receive signal SIGSEGV) + * -> EFAULT (or receive signal) */ #include <errno.h> @@ -70,8 +70,8 @@ static void run(unsigned int tc_num) } SAFE_WAITPID(pid, &status, 0); - if (tcases[tc_num].exp_err == EFAULT && WTERMSIG(status) == SIGSEGV) { - tst_res(TPASS, "fstat() failed as expected with SIGSEGV"); + if (tcases[tc_num].exp_err == EFAULT && WIFSIGNALED(status)) { + tst_res(TPASS, "fstat() failed as expected with a signal"); return; } -- 2.39.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Increase judgment on buf. 2023-05-21 8:19 ` Paul Eggert @ 2023-05-21 8:27 ` Andreas Schwab 2023-05-21 8:44 ` Paul Eggert 0 siblings, 1 reply; 9+ messages in thread From: Andreas Schwab @ 2023-05-21 8:27 UTC (permalink / raw) To: Paul Eggert Cc: Xi Ruoyao, lixing, Carlos O'Donell, Adhemerval Zanella Netto, Peng Fan, libc-alpha, ltp On Mai 21 2023, Paul Eggert wrote: > + if (tcases[tc_num].exp_err == EFAULT && WIFSIGNALED(status)) { Can this condition ever be true? -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Increase judgment on buf. 2023-05-21 8:27 ` Andreas Schwab @ 2023-05-21 8:44 ` Paul Eggert 2023-05-21 9:05 ` Andreas Schwab 0 siblings, 1 reply; 9+ messages in thread From: Paul Eggert @ 2023-05-21 8:44 UTC (permalink / raw) To: Andreas Schwab Cc: Xi Ruoyao, lixing, Carlos O'Donell, Adhemerval Zanella Netto, Peng Fan, libc-alpha, ltp On 2023-05-21 01:27, Andreas Schwab wrote: >> + if (tcases[tc_num].exp_err == EFAULT && WIFSIGNALED(status)) { > Can this condition ever be true? I don't see why not, if fstat(fd_ok, 0) does a SIGABRT or similar. But perhaps we're interpreting "Can" differently. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Increase judgment on buf. 2023-05-21 8:44 ` Paul Eggert @ 2023-05-21 9:05 ` Andreas Schwab 0 siblings, 0 replies; 9+ messages in thread From: Andreas Schwab @ 2023-05-21 9:05 UTC (permalink / raw) To: Paul Eggert Cc: Xi Ruoyao, lixing, Carlos O'Donell, Adhemerval Zanella Netto, Peng Fan, libc-alpha, ltp I have misinterpreted the first part of the condition. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Increase judgment on buf. @ 2023-05-19 3:57 Peng Fan 2023-05-19 11:48 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 9+ messages in thread From: Peng Fan @ 2023-05-19 3:57 UTC (permalink / raw) To: libc-alpha; +Cc: Xi Ruoyao When buf is empty, if it is not checked, the subsequent assignment operation will trigger a page fault. This is unnecessary. Signed-off-by: lixing <lixing@loongson.cn> Signed-off-by: Peng Fan <fanpeng@loongson.cn> --- sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c index 3509d3ca6d..b635a8299a 100644 --- a/sysdeps/unix/sysv/linux/fstatat64.c +++ b/sysdeps/unix/sysv/linux/fstatat64.c @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, { /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also 64-bit time_t support is done through statx syscall. */ - struct statx tmp; + struct statx tmp, *ptr; + if (buf) + ptr = &tmp; + else + ptr = NULL; int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag, - STATX_BASIC_STATS, &tmp); + STATX_BASIC_STATS, ptr); if (r != 0) return r; -- 2.33.0 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Increase judgment on buf. 2023-05-19 3:57 Peng Fan @ 2023-05-19 11:48 ` Adhemerval Zanella Netto 2023-05-19 11:55 ` Carlos O'Donell 0 siblings, 1 reply; 9+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-19 11:48 UTC (permalink / raw) To: Peng Fan, libc-alpha; +Cc: Xi Ruoyao On 19/05/23 00:57, Peng Fan wrote: > When buf is empty, if it is not checked, the subsequent assignment > operation will trigger a page fault. This is unnecessary. > > Signed-off-by: lixing <lixing@loongson.cn> > Signed-off-by: Peng Fan <fanpeng@loongson.cn> The stat family is explicitly marked with nonnull for the input struct stat buffer, and calling with a NULL argument is an UB. > --- > sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c > index 3509d3ca6d..b635a8299a 100644 > --- a/sysdeps/unix/sysv/linux/fstatat64.c > +++ b/sysdeps/unix/sysv/linux/fstatat64.c > @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, > { > /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also > 64-bit time_t support is done through statx syscall. */ > - struct statx tmp; > + struct statx tmp, *ptr; > + if (buf) > + ptr = &tmp; > + else > + ptr = NULL; > int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag, > - STATX_BASIC_STATS, &tmp); > + STATX_BASIC_STATS, ptr); > if (r != 0) > return r; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Increase judgment on buf. 2023-05-19 11:48 ` Adhemerval Zanella Netto @ 2023-05-19 11:55 ` Carlos O'Donell 2023-05-20 0:29 ` lixing 0 siblings, 1 reply; 9+ messages in thread From: Carlos O'Donell @ 2023-05-19 11:55 UTC (permalink / raw) To: Adhemerval Zanella Netto, Peng Fan, libc-alpha; +Cc: Xi Ruoyao On 5/19/23 07:48, Adhemerval Zanella Netto via Libc-alpha wrote: > > > On 19/05/23 00:57, Peng Fan wrote: >> When buf is empty, if it is not checked, the subsequent assignment >> operation will trigger a page fault. This is unnecessary. >> >> Signed-off-by: lixing <lixing@loongson.cn> >> Signed-off-by: Peng Fan <fanpeng@loongson.cn> > > The stat family is explicitly marked with nonnull for the input struct > stat buffer, and calling with a NULL argument is an UB. Agreed, and "Style and Conventions" https://sourceware.org/glibc/wiki/Style_and_Conventions says: https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program We should fail catastrophically and early in the case of user bugs. The segfault generates a core dump at exactly the right point to debug the UB. >> --- >> sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c >> index 3509d3ca6d..b635a8299a 100644 >> --- a/sysdeps/unix/sysv/linux/fstatat64.c >> +++ b/sysdeps/unix/sysv/linux/fstatat64.c >> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, >> { >> /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also >> 64-bit time_t support is done through statx syscall. */ >> - struct statx tmp; >> + struct statx tmp, *ptr; >> + if (buf) >> + ptr = &tmp; >> + else >> + ptr = NULL; >> int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag, >> - STATX_BASIC_STATS, &tmp); >> + STATX_BASIC_STATS, ptr); >> if (r != 0) >> return r; >> > -- Cheers, Carlos. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Increase judgment on buf. 2023-05-19 11:55 ` Carlos O'Donell @ 2023-05-20 0:29 ` lixing 0 siblings, 0 replies; 9+ messages in thread From: lixing @ 2023-05-20 0:29 UTC (permalink / raw) To: Carlos O'Donell, Adhemerval Zanella Netto, Peng Fan, libc-alpha Cc: Xi Ruoyao 在 2023/5/19 下午7:55, Carlos O'Donell via Libc-alpha 写道: > On 5/19/23 07:48, Adhemerval Zanella Netto via Libc-alpha wrote: >> >> On 19/05/23 00:57, Peng Fan wrote: >>> When buf is empty, if it is not checked, the subsequent assignment >>> operation will trigger a page fault. This is unnecessary. >>> >>> Signed-off-by: lixing <lixing@loongson.cn> >>> Signed-off-by: Peng Fan <fanpeng@loongson.cn> >> The stat family is explicitly marked with nonnull for the input struct >> stat buffer, and calling with a NULL argument is an UB. > Agreed, and "Style and Conventions" > https://sourceware.org/glibc/wiki/Style_and_Conventions > says: > https://sourceware.org/glibc/wiki/Style_and_Conventions#Bugs_in_the_user_program > > We should fail catastrophically and early in the case of user bugs. > The segfault generates a core dump at exactly the right point to debug the UB. Yes, LTP fstat03 test pass the buf with NULL. We just want to fail earlier during the syscall statx with return value EFAULT if the buf is NULL, but not pass the fault to the struct assignment which trigger SIGSEGV. >>> --- >>> sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/sysdeps/unix/sysv/linux/fstatat64.c b/sysdeps/unix/sysv/linux/fstatat64.c >>> index 3509d3ca6d..b635a8299a 100644 >>> --- a/sysdeps/unix/sysv/linux/fstatat64.c >>> +++ b/sysdeps/unix/sysv/linux/fstatat64.c >>> @@ -52,9 +52,13 @@ fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf, >>> { >>> /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also >>> 64-bit time_t support is done through statx syscall. */ >>> - struct statx tmp; >>> + struct statx tmp, *ptr; >>> + if (buf) >>> + ptr = &tmp; >>> + else >>> + ptr = NULL; >>> int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag, >>> - STATX_BASIC_STATS, &tmp); >>> + STATX_BASIC_STATS, ptr); >>> if (r != 0) >>> return r; >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-05-21 9:06 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <lxnjka-9sevacf455zj-1fthj246gvr4-712jsi8w59t4969pqyxmxkaq-l3n4z6dp0ybpuvpiuudtnfot-dl7onwkii2tq8gtprte6cu4fgip6f-ikuh33-a5p9ixcl44cx2h7mimcgl3xt-13bjom.1684553069255@email.android.com> 2023-05-20 10:19 ` [PATCH] Increase judgment on buf Xi Ruoyao 2023-05-21 8:19 ` Paul Eggert 2023-05-21 8:27 ` Andreas Schwab 2023-05-21 8:44 ` Paul Eggert 2023-05-21 9:05 ` Andreas Schwab 2023-05-19 3:57 Peng Fan 2023-05-19 11:48 ` Adhemerval Zanella Netto 2023-05-19 11:55 ` Carlos O'Donell 2023-05-20 0:29 ` lixing
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).