From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-9.2 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HTML_MESSAGE,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,URIBL_BLACK, USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 8675 invoked from network); 2 Sep 2021 16:39:51 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 2 Sep 2021 16:39:51 -0000 Received: (qmail 5162 invoked by uid 550); 2 Sep 2021 16:39:49 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 5144 invoked from network); 2 Sep 2021 16:39:48 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Lws8xMuehKwEBXsAC9Uy3rztbk70UPHRJCbxci13KYI=; b=A7a1VDim1sAffN4rc+lJPzQtHUk1cM6355fu/TAmdWAoQG21m5IS8lzNJ+7zjE4tXP aPw0BPsUGBrDXdmGAI/6DjIbtNvLwhRZUCUmTaGrS5yCKf0RxHoBV7c3nTN9gqn7GFqa GtzF8GBxbSvMjgKuLIeW1FbeliDW+VFsttOfGy4BeMGltPfsC+67KhNTKm7t7TTyLmxk sVtIe79649DceIcieF05u9kFTBFh57XEXldNUJob89sqPv6GCzfKd/QEN2U9ARNqE5nK ISIRPI+KVi7FX9o3QwGDvUnN6tEaugxRGCKqflDBpFYtLHDnpcCxmN4zCkFwruyeIUhE FwNw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Lws8xMuehKwEBXsAC9Uy3rztbk70UPHRJCbxci13KYI=; b=JG4BYVK/wtEhBHoi1RB5j9rd5DOLhuiSyD+dJLPByY1S+H4fvZIn1USSt9XGwfNHJO 9S88Nc3HI2Ls1kIDQMxD5yHdyNQKTHOuFzYCtv40XFVnM9njBLDRWpAUK2i+bzppy0WS I0Zb7PcKKeZ8QO6HujqrPb7ftTqoE/tHKQDbvUTHC/MAdJuYv31SpM1LGlsV3DqZ7QA/ w7CckjOQH2pGF4VomXSAs/vrjSqtHMtaz7plOeUgk3TxXoRvokVU7UPAUAGiX/y+Zfll qDE6SVbD6aM6ZMVsQks7aWLxOxQY5Kxx4FEDxpPckOrKvI8J9egKLGYNH2WCSztUHDK6 jtLQ== X-Gm-Message-State: AOAM532zYCDYVwfa1ax4i3QeBh/LYdT4gVaYH8QOBAMsUYIbxkHFbvrA Z+zOsL6svPdgGXTTZAtxR0IJzVsev12+KkbYkdNMK5VWW/j+ X-Google-Smtp-Source: ABdhPJynRZAdy4e6QYg14QCO2OxxZ9MMv/0J5NvTnpOSBTpOuyw2ZfanWvT8lqBt12rssq6HgnL8UkiNpLT7ohc0OnA= X-Received: by 2002:a17:906:fc7:: with SMTP id c7mr4753966ejk.333.1630600777060; Thu, 02 Sep 2021 09:39:37 -0700 (PDT) MIME-Version: 1.0 References: <20210830121736.GB13220@brightrain.aerifal.cx> In-Reply-To: From: Tamir Duberstein Date: Thu, 2 Sep 2021 12:39:25 -0400 Message-ID: To: Rich Felker Cc: musl@lists.openwall.com, Petr Hosek Content-Type: multipart/alternative; boundary="000000000000172ad405cb05d4d0" Subject: Re: [musl] undefined behavior in getdelim.c --000000000000172ad405cb05d4d0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Ping. On Mon, Aug 30, 2021 at 10:37 AM Tamir Duberstein wrote= : > On Mon, Aug 30, 2021 at 8:17 AM Rich Felker wrote: > > > > On Sun, Aug 29, 2021 at 06:13:44PM -0400, Tamir Duberstein wrote: > > > Fuchsia's libc is derived from musl. We make extensive use of clang > > > sanitizers in Fuchsia, and UBSAN has found "applying zero offset to > > > null pointer" in getdelim.c. > > > > Thank you for the detailed report! > > > > > Any call to `fopen` followed by a call to `getdelim` will trigger thi= s > > > behavior. The UB happens at > > > https://git.musl-libc.org/cgit/musl/tree/src/stdio/getdelim.c#n59. > > > Immediately after `fopen` `f->rpos` is `NULL`; `rpos` won't be > > > initialized until a few lines down in `getcunlocked`. > > > > So, a code path where p+=3Dk where p is a null pointer and k is 0, > > right? > > Yup. > > > > Here's the stack trace from UBSAN in Fuchsia: > > > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48:13: runti= me > > > error: applying zero offset to null pointer > > > #0 0x0000432ff5bf0613 in getdelim(char** restrict, size_t* > > > restrict, int, FILE* restrict) > > > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48 > > > +0x165613 > > > #1.2 0x00002380af30fe37 in ubsan_GetStackTrace() > > > compiler-rt/lib/ubsan/ubsan_diag.cpp:55 +0x3be37 > > > #1.1 0x00002380af30fe37 in MaybePrintStackTrace() > > > compiler-rt/lib/ubsan/ubsan_diag.cpp:53 +0x3be37 > > > #1 0x00002380af30fe37 in ~ScopedReport() > > > compiler-rt/lib/ubsan/ubsan_diag.cpp:389 +0x3be3= 7 > > > #2 0x00002380af3141fb in handlePointerOverflowImpl() > > > compiler-rt/lib/ubsan/ubsan_handlers.cpp:809 > > > +0x401fb > > > #3 0x00002380af313d6d in > > > compiler-rt/lib/ubsan/ubsan_handlers.cpp:815 > > > +0x3fd6d > > > #4 0x0000432ff5bf0613 in getdelim(char** restrict, size_t* > > > restrict, int, FILE* restrict) > > > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48 > > > +0x165613 > > > > > > Note that Fuchsia is a years behind, but I've confirmed this UB > > > happens even with the latest musl sources. > > > > > > Fixing this should be quite straightforward. I'm happy to send a patc= h > > > if you agree. > > > > > > Please CC me on response as I am not a subscriber to this mailing lis= t > > > per the guidance on https://musl.libc.org/support.html. > > > > What fix do you have in mind? I believe like 58 is also UB, by virtue > > of passing a null pointer to memcpy, albeit with zero length. My first > > leaning would be to get rid of the else body at line 32-33 and instead > > goto line with getc_unlocked, but the reason that's not already being > > done is that the buffer growth code at least originally needed to be > > hit before the getc_unlocked. I don't recall immediately if that's > > still the case. A very safe approach would be just putting lines 58-60 > > inside "if (k) { ... }" > > Good point, 58 is UB according to ISO/IEC 9899:1999 > (http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf) =C2=A77.21.1: > > Where an argument declared as size_t n specifies the length of the array > for a > function, n can have the value zero on a call to that function. Unless > explicitly stated > otherwise in the description of a particular function in this > subclause, pointer arguments > on such a call shall still have valid values, as described in 7.1.4. > On such a call, a > function that locates a character finds no occurrence, a function that > compares two > character sequences returns zero, and a function that copies > characters copies zero > characters. > > where =C2=A77.1.4.1 states: > > 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 promotion) not > expected by a function > with variable number of arguments, the behavior is undefined. > > I think the safe choice of just wrapping it in an `if (k) { ... }` is > the way to go. It would be nice to add a test case to libc-test for > this trivial case. Is there precedent for file operations in > libc-test? As far as I can tell the only stdio tests in libc-test are > signature tests > (http://nsz.repo.hu/git/?p=3Dlibc-test;a=3Dblob;f=3Dsrc/api/stdio.c;hb=3D= HEAD). > > By the way, where is the infrastructure that runs libc-test against > musl? Is UBSAN enabled there? > > Tamir > --000000000000172ad405cb05d4d0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Ping.

On Mon, Aug 30, 2021 at 10:37 AM Tamir Duberstein <= ;tamird@google.com> wrote:
<= /div>
On Mon, Aug 30, 2021= at 8:17 AM Rich Felker <dalias@libc.org> wrote:
>
> On Sun, Aug 29, 2021 at 06:13:44PM -0400, Tamir Duberstein wrote:
> > Fuchsia's libc is derived from musl. We make extensive use of= clang
> > sanitizers in Fuchsia, and UBSAN has found "applying zero of= fset to
> > null pointer" in getdelim.c.
>
> Thank you for the detailed report!
>
> > Any call to `fopen` followed by a call to `getdelim` will trigger= this
> > behavior. The UB happens at
> > https://git.musl-libc.org= /cgit/musl/tree/src/stdio/getdelim.c#n59.
> > Immediately after `fopen` `f->rpos` is `NULL`; `rpos` won'= t be
> > initialized until a few lines down in `getcunlocked`.
>
> So, a code path where p+=3Dk where p is a null pointer and k is 0,
> right?

Yup.

> > Here's the stack trace from UBSAN in Fuchsia:
> > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48:13: r= untime
> > error: applying zero offset to null pointer
> >=C2=A0 =C2=A0 #0=C2=A0 =C2=A0 0x0000432ff5bf0613 in getdelim(char*= * restrict, size_t*
> > restrict, int, FILE* restrict)
> > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48
> > <libc.so>+0x165613
> >=C2=A0 =C2=A0 #1.2=C2=A0 0x00002380af30fe37 in ubsan_GetStackTrace= ()
> > compiler-rt/lib/ubsan/ubsan_diag.cpp:55 <libclang_rt.asan.so<= /a>>+0x3be37
> >=C2=A0 =C2=A0 #1.1=C2=A0 0x00002380af30fe37 in MaybePrintStackTrac= e()
> > compiler-rt/lib/ubsan/ubsan_diag.cpp:53 <libclang_rt.asan.so<= /a>>+0x3be37
> >=C2=A0 =C2=A0 #1=C2=A0 =C2=A0 0x00002380af30fe37 in ~ScopedReport(= )
> > compiler-rt/lib/ubsan/ubsan_diag.cpp:389 <libclang_rt.asan.so= >+0x3be37
> >=C2=A0 =C2=A0 #2=C2=A0 =C2=A0 0x00002380af3141fb in handlePointerO= verflowImpl()
> > compiler-rt/lib/ubsan/ubsan_handlers.cpp:809
> > <libclang_rt.asan.so>+0x401fb
> >=C2=A0 =C2=A0 #3=C2=A0 =C2=A0 0x00002380af313d6d in
> > compiler-rt/lib/ubsan/ubsan_handlers.cpp:815
> > <libclang_rt.asan.so>+0x3fd6d
> >=C2=A0 =C2=A0 #4=C2=A0 =C2=A0 0x0000432ff5bf0613 in getdelim(char*= * restrict, size_t*
> > restrict, int, FILE* restrict)
> > .../../zircon/third_party/ulib/musl/src/stdio/getdelim.c:48
> > <libc.so>+0x165613
> >
> > Note that Fuchsia is a years behind, but I've confirmed this = UB
> > happens even with the latest musl sources.
> >
> > Fixing this should be quite straightforward. I'm happy to sen= d a patch
> > if you agree.
> >
> > Please CC me on response as I am not a subscriber to this mailing= list
> > per the guidance on https://musl.libc.org/support.html.
>
> What fix do you have in mind? I believe like 58 is also UB, by virtue<= br> > of passing a null pointer to memcpy, albeit with zero length. My first=
> leaning would be to get rid of the else body at line 32-33 and instead=
> goto line with getc_unlocked, but the reason that's not already be= ing
> done is that the buffer growth code at least originally needed to be > hit before the getc_unlocked. I don't recall immediately if that&#= 39;s
> still the case. A very safe approach would be just putting lines 58-60=
> inside "if (k) { ... }"

Good point, 58 is UB according to ISO/IEC 9899:1999
(http://www.open-std.org/jtc1/sc22/WG14/ww= w/docs/n1256.pdf) =C2=A77.21.1:

Where an argument declared as size_t n specifies the length of the array fo= r a
function, n can have the value zero on a call to that function. Unless
explicitly stated
otherwise in the description of a particular function in this
subclause, pointer arguments
on such a call shall still have valid values, as described in 7.1.4.
On such a call, a
function that locates a character finds no occurrence, a function that
compares two
character sequences returns zero, and a function that copies
characters copies zero
characters.

where =C2=A77.1.4.1 states:

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 correspo= nding
parameter is not const-qualified) or a type (after promotion) not
expected by a function
with variable number of arguments, the behavior is undefined.

I think the safe choice of just wrapping it in an `if (k) { ... }` is
the way to go. It would be nice to add a test case to libc-test for
this trivial case. Is there precedent for file operations in
libc-test? As far as I can tell the only stdio tests in libc-test are
signature tests
(http://nsz.repo.hu/git/= ?p=3Dlibc-test;a=3Dblob;f=3Dsrc/api/stdio.c;hb=3DHEAD).

By the way, where is the infrastructure that runs libc-test against
musl? Is UBSAN enabled there?

Tamir
--000000000000172ad405cb05d4d0--