From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from xry111.site (xry111.site [89.208.246.23]) by sourceware.org (Postfix) with ESMTPS id 9F7EA3858C52 for ; Sat, 20 May 2023 10:19:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9F7EA3858C52 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=xry111.site Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=xry111.site DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=xry111.site; s=default; t=1684577952; bh=UMbti4tl9TfSXb7hzvJ8MHfeM/mK+Y9wEXHbzSp8qrc=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=MV2+dYsaSZbk8am5TTRvKL2amQtffUGVZGN5FYKBBkjz+W0ICjlLt9j03YcNv+hj0 liw1x9yRXbWecMsJ5HE1YkEw3M4GPDsFUXpjUN5TTDQYtzSA6GmSMRRVfBz8YKa/OB jMhJZJ/s9kz9/focqjar9478/104ptsGsgGm7y1s= Received: from [192.168.124.8] (unknown [113.140.29.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature ECDSA (P-384) server-digest SHA384) (Client did not present a certificate) (Authenticated sender: xry111@xry111.site) by xry111.site (Postfix) with ESMTPSA id 590D665AE7; Sat, 20 May 2023 06:19:11 -0400 (EDT) Message-ID: <0cb83efa85ae32d956f81e6b9d4966c38fd54bcb.camel@xry111.site> Subject: Re: [PATCH] Increase judgment on buf. From: Xi Ruoyao To: lixing , Carlos O'Donell , Adhemerval Zanella Netto , Peng Fan Cc: "libc-alpha@sourceware.org" Date: Sat, 20 May 2023 18:19:09 +0800 In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.1 MIME-Version: 1.0 X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,LIKELY_SPAM_FROM,RCVD_IN_BARRACUDACENTRAL,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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.=C2=A0 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.=20 * If you have no contract to accept NULL and immediately dereference the pointer then the segfault is sufficient to indicate the error.=C2=A0 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.=C2=A0 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.=C2=A0 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.=C2=A0 Should we start to add NULL checks everywhere, say "read", "strlen", "fclose" etc.?=C2=A0 Yes some (stupid) textbook says we should do this but let's burn such a textbook in fire.=C2=A0 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.=C2=A0 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". -------- =E5=8E=9F=E5=A7=8B=E9=82=AE=E4=BB=B6 -------- =E5=8F=91=E4=BB=B6=E4=BA=BA=EF=BC=9A lixing =E6=97=A5=E6=9C=9F=EF=BC=9A 2023=E5=B9=B45=E6=9C=8820=E6=97=A5=E5=91=A8=E5= =85=AD =E6=97=A9=E4=B8=8A8:29 =E6=94=B6=E4=BB=B6=E4=BA=BA=EF=BC=9A Carlos O'Donell , A= dhemerval Zanella Netto , Peng Fan , libc-alpha@sourceware.org =E6=8A=84=E9=80=81=EF=BC=9A Xi Ruoyao =E4=B8=BB =E9=A2=98=EF=BC=9A Re: [PATCH] Increase judgment on buf. >=20 > =E5=9C=A8 2023/5/19 =E4=B8=8B=E5=8D=887:55, Carlos O'Donell via Libc-alph= a =E5=86=99=E9=81=93: > > 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 > >>> Signed-off-by: Peng Fan > >> 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_pr= ogram > > > > 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. >=20 > Yes, LTP fstat03 test pass the buf with NULL. We just want to fail=20 > earlier during the syscall statx with return value EFAULT if the buf > is=20 > NULL, >=20 > but not pass the fault to the struct assignment which trigger SIGSEGV. >=20 > >>> --- > >>>=C2=A0=C2=A0 sysdeps/unix/sysv/linux/fstatat64.c | 8 ++++++-- > >>>=C2=A0=C2=A0 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, > >>>=C2=A0=C2=A0 { > >>>=C2=A0=C2=A0=C2=A0=C2=A0 /* 32-bit kABI with default 64-bit time_t, e.= g. arc, > riscv32.=C2=A0=C2=A0 Also > >>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 64-bit time_t support is do= ne through statx syscall.=C2=A0 */ > >>> -=C2=A0 struct statx tmp; > >>> +=C2=A0 struct statx tmp, *ptr; > >>> +=C2=A0 if (buf) > >>> + ptr =3D &tmp; > >>> +=C2=A0 else > >>> + ptr =3D NULL; > >>>=C2=A0=C2=A0=C2=A0=C2=A0 int r =3D INTERNAL_SYSCALL_CALL (statx, fd, f= ile, > AT_NO_AUTOMOUNT | flag, > >>> - STATX_BASIC_STATS, &tmp); > >>> + STATX_BASIC_STATS, ptr); > >>>=C2=A0=C2=A0=C2=A0=C2=A0 if (r !=3D 0) > >>>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return r; > >>>=C2=A0=C2=A0=20 >=20 --=20 Xi Ruoyao School of Aerospace Science and Technology, Xidian University