mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] undefined behavior in getdelim.c
@ 2021-08-29 22:13 Tamir Duberstein
  2021-08-30 12:17 ` Rich Felker
  2021-09-02 19:36 ` Jeffrey Walton
  0 siblings, 2 replies; 6+ messages in thread
From: Tamir Duberstein @ 2021-08-29 22:13 UTC (permalink / raw)
  To: musl; +Cc: Petr Hosek

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.

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

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.

Thank you.
Tamir

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] undefined behavior in getdelim.c
  2021-08-29 22:13 [musl] undefined behavior in getdelim.c Tamir Duberstein
@ 2021-08-30 12:17 ` Rich Felker
  2021-08-30 14:37   ` Tamir Duberstein
  2021-09-02 19:36 ` Jeffrey Walton
  1 sibling, 1 reply; 6+ messages in thread
From: Rich Felker @ 2021-08-30 12:17 UTC (permalink / raw)
  To: Tamir Duberstein; +Cc: musl, Petr Hosek

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?

> 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) { ... }"

Rich

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] undefined behavior in getdelim.c
  2021-08-30 12:17 ` Rich Felker
@ 2021-08-30 14:37   ` Tamir Duberstein
  2021-09-02 16:39     ` Tamir Duberstein
  0 siblings, 1 reply; 6+ messages in thread
From: Tamir Duberstein @ 2021-08-30 14:37 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl, Petr Hosek

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] undefined behavior in getdelim.c
  2021-08-30 14:37   ` Tamir Duberstein
@ 2021-09-02 16:39     ` Tamir Duberstein
  0 siblings, 0 replies; 6+ messages in thread
From: Tamir Duberstein @ 2021-09-02 16:39 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl, Petr Hosek

[-- Attachment #1: Type: text/plain, Size: 4756 bytes --]

Ping.

On Mon, Aug 30, 2021 at 10:37 AM Tamir Duberstein <tamird@google.com> wrote:

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

[-- Attachment #2: Type: text/html, Size: 6486 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] undefined behavior in getdelim.c
  2021-08-29 22:13 [musl] undefined behavior in getdelim.c Tamir Duberstein
  2021-08-30 12:17 ` Rich Felker
@ 2021-09-02 19:36 ` Jeffrey Walton
  2021-09-02 20:19   ` Rich Felker
  1 sibling, 1 reply; 6+ messages in thread
From: Jeffrey Walton @ 2021-09-02 19:36 UTC (permalink / raw)
  To: musl

Hi Tamir,

On Mon, Aug 30, 2021 at 7:18 AM Tamir Duberstein <tamird@google.com> 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.

Off-topic, but your emails are landing in Gmail's spam folder.

It looks like DKIM and DMARC are not setup properly for your domain at
1e100.net. Also see https://support.google.com/a/answer/9948472.

Jeff

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [musl] undefined behavior in getdelim.c
  2021-09-02 19:36 ` Jeffrey Walton
@ 2021-09-02 20:19   ` Rich Felker
  0 siblings, 0 replies; 6+ messages in thread
From: Rich Felker @ 2021-09-02 20:19 UTC (permalink / raw)
  To: Jeffrey Walton; +Cc: musl

On Thu, Sep 02, 2021 at 03:36:01PM -0400, Jeffrey Walton wrote:
> Hi Tamir,
> 
> On Mon, Aug 30, 2021 at 7:18 AM Tamir Duberstein <tamird@google.com> 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.
> 
> Off-topic, but your emails are landing in Gmail's spam folder.
> 
> It looks like DKIM and DMARC are not setup properly for your domain at
> 1e100.net. Also see https://support.google.com/a/answer/9948472.

Are you sure this isn't just DMARC being fundamentally
mailing-list-hostile? If so, you just need to add "not spam" rules for
mailing lists if you have a receiving mail system that's enforcing it
according to the (flawed) spec.

Rich

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-02 20:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-29 22:13 [musl] undefined behavior in getdelim.c Tamir Duberstein
2021-08-30 12:17 ` Rich Felker
2021-08-30 14:37   ` Tamir Duberstein
2021-09-02 16:39     ` Tamir Duberstein
2021-09-02 19:36 ` Jeffrey Walton
2021-09-02 20:19   ` Rich Felker

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