[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK-_uh4rCaVtN+rzqqt+3jpKZnG82VxAaGjcF809qqoKtS6Bsw@mail.gmail.com>
Date: Mon, 30 Aug 2021 10:37:22 -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
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
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.
