mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Tamir Duberstein <tamird@google.com>
To: Rich Felker <dalias@libc.org>
Cc: musl@lists.openwall.com, Petr Hosek <phosek@google.com>
Subject: Re: [musl] undefined behavior in getdelim.c
Date: Mon, 30 Aug 2021 10:37:22 -0400	[thread overview]
Message-ID: <CAK-_uh4rCaVtN+rzqqt+3jpKZnG82VxAaGjcF809qqoKtS6Bsw@mail.gmail.com> (raw)
In-Reply-To: <20210830121736.GB13220@brightrain.aerifal.cx>

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 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

  reply	other threads:[~2021-08-30 14:54 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-29 22:13 Tamir Duberstein
2021-08-30 12:17 ` Rich Felker
2021-08-30 14:37   ` Tamir Duberstein [this message]
2021-09-02 16:39     ` Tamir Duberstein
2021-09-02 19:36 ` Jeffrey Walton
2021-09-02 20:19   ` Rich Felker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAK-_uh4rCaVtN+rzqqt+3jpKZnG82VxAaGjcF809qqoKtS6Bsw@mail.gmail.com \
    --to=tamird@google.com \
    --cc=dalias@libc.org \
    --cc=musl@lists.openwall.com \
    --cc=phosek@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).