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