[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK-_uh53G1hu8nXM0aH2U+RtZ61Z3fFof8RNsnda0wnq-Roy9w@mail.gmail.com>
Date: Thu, 2 Sep 2021 12:39:25 -0400
From: Tamir Duberstein <tamird@...gle.com>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com, Petr Hosek <phosek@...gle.com>
Subject: Re: undefined behavior in getdelim.c
Ping.
On Mon, Aug 30, 2021 at 10:37 AM Tamir Duberstein <tamird@...gle.com> wrote:
> On Mon, Aug 30, 2021 at 8:17 AM Rich Felker <dalias@...c.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 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 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+=k 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: runtime
> > > 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
> > > <libc.so>+0x165613
> > > #1.2 0x00002380af30fe37 in ubsan_GetStackTrace()
> > > compiler-rt/lib/ubsan/ubsan_diag.cpp:55 <libclang_rt.asan.so>+0x3be37
> > > #1.1 0x00002380af30fe37 in MaybePrintStackTrace()
> > > compiler-rt/lib/ubsan/ubsan_diag.cpp:53 <libclang_rt.asan.so>+0x3be37
> > > #1 0x00002380af30fe37 in ~ScopedReport()
> > > compiler-rt/lib/ubsan/ubsan_diag.cpp:389 <libclang_rt.asan.so>+0x3be37
> > > #2 0x00002380af3141fb in handlePointerOverflowImpl()
> > > compiler-rt/lib/ubsan/ubsan_handlers.cpp:809
> > > <libclang_rt.asan.so>+0x401fb
> > > #3 0x00002380af313d6d in
> > > compiler-rt/lib/ubsan/ubsan_handlers.cpp:815
> > > <libclang_rt.asan.so>+0x3fd6d
> > > #4 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 send 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
> > 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) §7.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 §7.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=libc-test;a=blob;f=src/api/stdio.c;hb=HEAD).
>
> By the way, where is the infrastructure that runs libc-test against
> musl? Is UBSAN enabled there?
>
> Tamir
>
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
