Re: [PATCH] Increase judgment on buf.

[Xi Ruoyao <xry111@xry111.site>]

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