[-- Attachment #1: Type: text/plain, Size: 2038 bytes --] Hello, it's me again! I previously reported undefined behavior in getdelim.c in https://www.openwall.com/lists/musl/2021/08/30/1, and just noticed this week that it has been fixed. Thank you! After pulling in the latest changes, we now trip over UB in fread.c at https://git.musl-libc.org/cgit/musl/tree/src/stdio/fread.c#n21 on a `fread(NULL, 1, 0, ...)` call. `dest` is `NULL`, and incrementing a null pointer (even by zero) is UB. Here's the stack trace: ../../zircon/third_party/ulib/musl/src/stdio/fread.c:22:10: runtime error: applying zero offset to null pointer #0 0x00008037bf602a6c in fread(void* restrict, size_t, size_t, FILE* restrict) ../../zircon/third_party/ulib/musl/src/stdio/fread.c:22 <libr.so>+0x168a6c #1.2 0x0000421373b5f4ec in ubsan_GetStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:41 <libclang_rt.asan.so>+0x3d4ec #1.1 0x0000421373b5f4ec in MaybePrintStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:51 <libclang_rt.asan.so>+0x3d4ec #1 0x0000421373b5f4ec in ~ScopedReport() compiler-rt/lib/ubsan/ubsan_diag.cpp:387 <libclang_rt.asan.so>+0x3d4ec #2 0x0000421373b62684 in handlePointerOverflowImpl() compiler-rt/lib/ubsan/ubsan_handlers.cpp:809 <libclang_rt.asan.so>+0x40684 #3 0x0000421373b6239c in compiler-rt/lib/ubsan/ubsan_handlers.cpp:815 < libclang_rt.asan.so>+0x4039c #4 0x00008037bf602a6c in fread(void* restrict, size_t, size_t, FILE* restrict) ../../zircon/third_party/ulib/musl/src/stdio/fread.c:22 <libc.so>+0x168a6c #5 0x00004347972c0934 in FT_Stream_Seek(FT_Stream, FT_ULong) ../../third_party/freetype2/src/base/ftstream.c:64 <libfreetype2.so>+0xf1934 I think instead of `nmemb = 0` on line 10 that should just return. I've confirmed glibc does a similar check and avoids UB in this case. See https://sourceware.org/git/?p=glibc.git;a=blob;f=libio/iofread.c;hb=HEAD#l35 and https://godbolt.org/z/cYc9rG1ea. Please CC me on responses as I am not a subscriber to this mailing list per the guidance on https://musl.libc.org/support.html. Thank you. Tamir [-- Attachment #2: Type: text/html, Size: 2839 bytes --]
On Fri, Feb 24, 2023 at 07:52:11AM -0500, Tamir Duberstein wrote:
> Hello, it's me again! I previously reported undefined behavior in
> getdelim.c in https://www.openwall.com/lists/musl/2021/08/30/1, and just
> noticed this week that it has been fixed. Thank you!
>
> After pulling in the latest changes, we now trip over UB in fread.c at
> https://git.musl-libc.org/cgit/musl/tree/src/stdio/fread.c#n21 on a
> `fread(NULL, 1, 0, ...)` call. `dest` is `NULL`, and incrementing a null
> pointer (even by zero) is UB. Here's the stack trace:
>
> .../../zircon/third_party/ulib/musl/src/stdio/fread.c:22:10: runtime error:
> applying zero offset to null pointer
> #0 0x00008037bf602a6c in fread(void* restrict, size_t, size_t, FILE*
> restrict) ../../zircon/third_party/ulib/musl/src/stdio/fread.c:22
> <libr.so>+0x168a6c
> #1.2 0x0000421373b5f4ec in ubsan_GetStackTrace()
> compiler-rt/lib/ubsan/ubsan_diag.cpp:41 <libclang_rt.asan.so>+0x3d4ec
> #1.1 0x0000421373b5f4ec in MaybePrintStackTrace()
> compiler-rt/lib/ubsan/ubsan_diag.cpp:51 <libclang_rt.asan.so>+0x3d4ec
> #1 0x0000421373b5f4ec in ~ScopedReport()
> compiler-rt/lib/ubsan/ubsan_diag.cpp:387 <libclang_rt.asan.so>+0x3d4ec
> #2 0x0000421373b62684 in handlePointerOverflowImpl()
> compiler-rt/lib/ubsan/ubsan_handlers.cpp:809 <libclang_rt.asan.so>+0x40684
> #3 0x0000421373b6239c in compiler-rt/lib/ubsan/ubsan_handlers.cpp:815 <
> libclang_rt.asan.so>+0x4039c
> #4 0x00008037bf602a6c in fread(void* restrict, size_t, size_t, FILE*
> restrict) ../../zircon/third_party/ulib/musl/src/stdio/fread.c:22
> <libc.so>+0x168a6c
> #5 0x00004347972c0934 in FT_Stream_Seek(FT_Stream, FT_ULong)
> .../../third_party/freetype2/src/base/ftstream.c:64 <libfreetype2.so>+0xf1934
>
> I think instead of `nmemb = 0` on line 10 that should just return.
>
> I've confirmed glibc does a similar check and avoids UB in this case. See
> https://sourceware.org/git/?p=glibc.git;a=blob;f=libio/iofread.c;hb=HEAD#l35
> and https://godbolt.org/z/cYc9rG1ea.
>
> Please CC me on responses as I am not a subscriber to this mailing list
> per the guidance on https://musl.libc.org/support.html.
Is there any indication that passing NULL as the first argument to
fread is not itself undefined? Normally I would expect that to be the
case.
Rich
[-- Attachment #1: Type: text/plain, Size: 887 bytes --] on Fri, 24 Feb 2023 08:34:14 -0500 you (Rich Felker <dalias@libc.org>) wrote: > Is there any indication that passing NULL as the first argument to > fread is not itself undefined? Normally I would expect that to be the > case. I don't think so. The corresponding text of the C standard clearly indicates that the first argument is expected to point to an array. By that the provisions of 7.1.4 take effect: If an argument to a function has an invalid value (such as ..., or a null pointer, ...) ..., the behavior is undefined. Thanks Jₑₙₛ -- :: ICube :::::::::::::::::::::::::::::: deputy director :: :: Université de Strasbourg :::::::::::::::::::::: ICPS :: :: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus :: :: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 :: :: https://icube-icps.unistra.fr/index.php/Jens_Gustedt :: [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 195 bytes --]
On Fri, Feb 24, 2023 at 08:34:14AM -0500, Rich Felker wrote:
> Is there any indication that passing NULL as the first argument to
> fread is not itself undefined?
C99 says the following:
| If size or nmemb is zero, fread returns zero and the contents of the
| array and the state of the stream remain unchanged.
It doesn't explicitly mention weather stream can be NULL or not in case
of 0 size/nmemb - but regardless, is there any actual reason for not
returning early?
The following should be OK as far as I see:
- if (!size) nmemb = 0;
+ if (!size || !nmemb) return 0;
or `if (!len) return 0;` could also work if multiplication overflow is
not a concern.
- NRK
On Fri, Feb 24, 2023 at 07:55:11PM +0600, NRK wrote:
> On Fri, Feb 24, 2023 at 08:34:14AM -0500, Rich Felker wrote:
> > Is there any indication that passing NULL as the first argument to
> > fread is not itself undefined?
>
> C99 says the following:
>
> | If size or nmemb is zero, fread returns zero and the contents of the
> | array and the state of the stream remain unchanged.
>
> It doesn't explicitly mention weather stream can be NULL or not in case
> of 0 size/nmemb - but regardless, is there any actual reason for not
> returning early?
>
> The following should be OK as far as I see:
>
> - if (!size) nmemb = 0;
> + if (!size || !nmemb) return 0;
>
> or `if (!len) return 0;` could also work if multiplication overflow is
> not a concern.
stdio functions are required (by POSIX) to behave as if they take a
mutex on the FILE. If fread with a length of zero makes forward
progress when another thread holds the lock, this is non-conforming.
Rich
On Fri, Feb 24, 2023 at 09:07:41AM -0500, Rich Felker wrote:
> stdio functions are required (by POSIX) to behave as if they take a
> mutex on the FILE. If fread with a length of zero makes forward
> progress when another thread holds the lock, this is non-conforming.
OK, I see. Wasn't aware of that.
On a sidenote, if I'm reading the right function (libio/iofread.c) then
glibc is taking the lock _after_ they do the size check.
if (bytes_requested == 0)
return 0;
_IO_acquire_lock (fp);
/* ... */
- NRK
[-- Attachment #1: Type: text/plain, Size: 702 bytes --] We could take the lock and still avoid UB with an early return. On Fri, Feb 24, 2023, 09:17 NRK <nrk@disroot.org> wrote: > On Fri, Feb 24, 2023 at 09:07:41AM -0500, Rich Felker wrote: > > stdio functions are required (by POSIX) to behave as if they take a > > mutex on the FILE. If fread with a length of zero makes forward > > progress when another thread holds the lock, this is non-conforming. > > OK, I see. Wasn't aware of that. > > On a sidenote, if I'm reading the right function (libio/iofread.c) then > glibc is taking the lock _after_ they do the size check. > > if (bytes_requested == 0) > return 0; > _IO_acquire_lock (fp); > /* ... */ > > - NRK > [-- Attachment #2: Type: text/html, Size: 1046 bytes --]
On Fri, Feb 24, 2023 at 09:42:00AM -0500, Tamir Duberstein wrote:
> We could take the lock and still avoid UB with an early return.
As Jens has pointed out, the UB in this case is the caller calling fread
with NULL - not in musl.
And on a sidenote, I've always found - especially for the various mem*
functions - accepting 0 size but not accepting NULL arg (when n is 0) to
be a poor choice. A lot of the value that accepting 0 size provides is
diminished by not accepting NULL.
And this affects more than just libc, too. Compilers like gcc/clang will
see a call like `memcmp(p, q, 0)` and will ""determine"" `p` and `q` are
non-null (which can lead to deleting any subsequent null-checks on those
pointers).
But anyways, that was just a small rant.
As things currently are, *even if* musl deal with the NULL pointer - any
caller calling fread with NULL is still in danger from compilers and
needs to fix it on their side.
- NRK
[-- Attachment #1: Type: text/plain, Size: 1764 bytes --] I agree, the caller's behavior is UB. I'll send them (freetype2) a patch. That said, do we want to avoid internal UB here anyway? - As mentioned earlier, glibc avoids the UB (and the lock). - llvm-libc does the same starting with https://github.com/llvm/llvm-project/commit/53c251b - uclibc avoids the UB but still locks: https://github.com/gittup/uClibc/blob/9dbf00b/libc/stdio/fread.c#L25 - FreeBSD avoids the UB but still locks: https://svnweb.freebsd.org/base/head/lib/libc/stdio/fread.c?view=markup#l76 - Android (bionic) avoids the UB but still locks: https://cs.android.com/android/platform/superproject/+/master:bionic/libc/stdio/stdio.cpp;l=1099;drc=4aa8f499f21ebf84101de34d68682d5388667001 Does this persuade? On Fri, Feb 24, 2023 at 10:13 AM NRK <nrk@disroot.org> wrote: > On Fri, Feb 24, 2023 at 09:42:00AM -0500, Tamir Duberstein wrote: > > We could take the lock and still avoid UB with an early return. > > As Jens has pointed out, the UB in this case is the caller calling fread > with NULL - not in musl. > > And on a sidenote, I've always found - especially for the various mem* > functions - accepting 0 size but not accepting NULL arg (when n is 0) to > be a poor choice. A lot of the value that accepting 0 size provides is > diminished by not accepting NULL. > > And this affects more than just libc, too. Compilers like gcc/clang will > see a call like `memcmp(p, q, 0)` and will ""determine"" `p` and `q` are > non-null (which can lead to deleting any subsequent null-checks on those > pointers). > > But anyways, that was just a small rant. > > As things currently are, *even if* musl deal with the NULL pointer - any > caller calling fread with NULL is still in danger from compilers and > needs to fix it on their side. > > - NRK > [-- Attachment #2: Type: text/html, Size: 2658 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1454 bytes --] on Fri, 24 Feb 2023 11:12:11 -0500 you (Tamir Duberstein <tamird@google.com>) wrote: > I agree, the caller's behavior is UB. I'll send them (freetype2) a > patch. > > That said, do we want to avoid internal UB here anyway? I am not sure that I even understand what "internal UB" is supposed to mean. > - As mentioned earlier, glibc avoids the UB (and the lock). > - llvm-libc does the same starting with > https://github.com/llvm/llvm-project/commit/53c251b > - uclibc avoids the UB but still locks: > https://github.com/gittup/uClibc/blob/9dbf00b/libc/stdio/fread.c#L25 > - FreeBSD avoids the UB but still locks: > https://svnweb.freebsd.org/base/head/lib/libc/stdio/fread.c?view=markup#l76 > - Android (bionic) avoids the UB but still locks: > https://cs.android.com/android/platform/superproject/+/master:bionic/libc/stdio/stdio.cpp;l=1099;drc=4aa8f499f21ebf84101de34d68682d5388667001 > > Does this persuade? Me personally not much. The only thing that would help applications to write portable code is to put an attribute on the pointer argument such that bad calls get diagnosed if possible. Jₑₙₛ -- :: ICube :::::::::::::::::::::::::::::: deputy director :: :: Université de Strasbourg :::::::::::::::::::::: ICPS :: :: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus :: :: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 :: :: https://icube-icps.unistra.fr/index.php/Jens_Gustedt :: [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1779 bytes --] Internal UB means "applying zero offset to null pointer" when the caller passes (NULL, _, 0, _). Does such an attribute exist? On Fri, Feb 24, 2023 at 11:40 AM Jₑₙₛ Gustedt <jens.gustedt@inria.fr> wrote: > > on Fri, 24 Feb 2023 11:12:11 -0500 you (Tamir Duberstein > <tamird@google.com>) wrote: > > > I agree, the caller's behavior is UB. I'll send them (freetype2) a > > patch. > > > > That said, do we want to avoid internal UB here anyway? > > I am not sure that I even understand what "internal UB" is supposed to > mean. > > > - As mentioned earlier, glibc avoids the UB (and the lock). > > - llvm-libc does the same starting with > > https://github.com/llvm/llvm-project/commit/53c251b > > - uclibc avoids the UB but still locks: > > https://github.com/gittup/uClibc/blob/9dbf00b/libc/stdio/fread.c#L25 > > - FreeBSD avoids the UB but still locks: > > > https://svnweb.freebsd.org/base/head/lib/libc/stdio/fread.c?view=markup#l76 > > - Android (bionic) avoids the UB but still locks: > > > https://cs.android.com/android/platform/superproject/+/master:bionic/libc/stdio/stdio.cpp;l=1099;drc=4aa8f499f21ebf84101de34d68682d5388667001 > > > > Does this persuade? > > Me personally not much. The only thing that would help applications to > write portable code is to put an attribute on the pointer argument > such that bad calls get diagnosed if possible. > > Jₑₙₛ > > -- > :: ICube :::::::::::::::::::::::::::::: deputy director :: > :: Université de Strasbourg :::::::::::::::::::::: ICPS :: > :: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus :: > :: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 > <+33%203%2068%2085%2045%2036> :: > :: https://icube-icps.unistra.fr/index.php/Jens_Gustedt :: > [-- Attachment #2: Type: text/html, Size: 2956 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1774 bytes --] On Fri, Feb 24, 2023 at 8:40 AM Jₑₙₛ Gustedt <jens.gustedt@inria.fr> wrote: > > on Fri, 24 Feb 2023 11:12:11 -0500 you (Tamir Duberstein > <tamird@google.com>) wrote: > > > I agree, the caller's behavior is UB. I'll send them (freetype2) a > > patch. > > > > That said, do we want to avoid internal UB here anyway? > > I am not sure that I even understand what "internal UB" is supposed to > mean. > > > - As mentioned earlier, glibc avoids the UB (and the lock). > > - llvm-libc does the same starting with > > https://github.com/llvm/llvm-project/commit/53c251b > > - uclibc avoids the UB but still locks: > > https://github.com/gittup/uClibc/blob/9dbf00b/libc/stdio/fread.c#L25 > > - FreeBSD avoids the UB but still locks: > > > https://svnweb.freebsd.org/base/head/lib/libc/stdio/fread.c?view=markup#l76 > > - Android (bionic) avoids the UB but still locks: > > > https://cs.android.com/android/platform/superproject/+/master:bionic/libc/stdio/stdio.cpp;l=1099;drc=4aa8f499f21ebf84101de34d68682d5388667001 > > > > Does this persuade? > > Me personally not much. The only thing that would help applications to > write portable code is to put an attribute on the pointer argument > such that bad calls get diagnosed if possible. > (bionic's actually working on the larger "annotate all the functions" project, but hasn't got as far as stdio.h yet :-) ) > Jₑₙₛ > > -- > :: ICube :::::::::::::::::::::::::::::: deputy director :: > :: Université de Strasbourg :::::::::::::::::::::: ICPS :: > :: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus :: > :: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 > <+33%203%2068%2085%2045%2036> :: > :: https://icube-icps.unistra.fr/index.php/Jens_Gustedt :: > [-- Attachment #2: Type: text/html, Size: 3125 bytes --]
[-- Attachment #1: Type: text/plain, Size: 940 bytes --] Tamir, on Fri, 24 Feb 2023 11:42:30 -0500 you (Tamir Duberstein <tamird@google.com>) wrote: > Internal UB means "applying zero offset to null pointer" when the > caller passes (NULL, _, 0, _). So you mean internal misbehavior? > Does such an attribute exist? Yes, gcc has the nonnull old-style attribute, and I think that the C23 versions of gcc and clang will have something like `[[gnu::nonnull]]`. Having this as attribute of the function and not of the pointer is a bit unfortunate. For non-void pointers you could also use the `[static 1]` array parameter notation instead of a pointer. Jₑₙₛ -- :: ICube :::::::::::::::::::::::::::::: deputy director :: :: Université de Strasbourg :::::::::::::::::::::: ICPS :: :: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus :: :: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 :: :: https://icube-icps.unistra.fr/index.php/Jens_Gustedt :: [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1268 bytes --] On Fri, Feb 24, 2023 at 9:00 AM Jₑₙₛ Gustedt <jens.gustedt@inria.fr> wrote: > Tamir, > > on Fri, 24 Feb 2023 11:42:30 -0500 you (Tamir Duberstein > <tamird@google.com>) wrote: > > > Internal UB means "applying zero offset to null pointer" when the > > caller passes (NULL, _, 0, _). > > So you mean internal misbehavior? > > > Does such an attribute exist? > > Yes, gcc has the nonnull old-style attribute, and I think that the C23 > versions of gcc and clang will have something like `[[gnu::nonnull]]`. > > Having this as attribute of the function and not of the pointer is a > bit unfortunate. For non-void pointers you could also use the `[static > 1]` array parameter notation instead of a pointer. > bionic's using clang's _Nullable and _Nonnull, which do let you express this: https://clang.llvm.org/docs/AttributeReference.html#nullability-attributes > Jₑₙₛ > > -- > :: ICube :::::::::::::::::::::::::::::: deputy director :: > :: Université de Strasbourg :::::::::::::::::::::: ICPS :: > :: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus :: > :: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 > <+33%203%2068%2085%2045%2036> :: > :: https://icube-icps.unistra.fr/index.php/Jens_Gustedt :: > [-- Attachment #2: Type: text/html, Size: 2118 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1557 bytes --] Clang's qualifiers fail to parse in GCC, so __attribute__((nonnull (1))) seems like the only somewhat portable solution. Would you folks actually consider using it? On Fri, Feb 24, 2023 at 12:07 PM enh <enh@google.com> wrote: > > > On Fri, Feb 24, 2023 at 9:00 AM Jₑₙₛ Gustedt <jens.gustedt@inria.fr> > wrote: > >> Tamir, >> >> on Fri, 24 Feb 2023 11:42:30 -0500 you (Tamir Duberstein >> <tamird@google.com>) wrote: >> >> > Internal UB means "applying zero offset to null pointer" when the >> > caller passes (NULL, _, 0, _). >> >> So you mean internal misbehavior? >> >> > Does such an attribute exist? >> >> Yes, gcc has the nonnull old-style attribute, and I think that the C23 >> versions of gcc and clang will have something like `[[gnu::nonnull]]`. >> >> Having this as attribute of the function and not of the pointer is a >> bit unfortunate. For non-void pointers you could also use the `[static >> 1]` array parameter notation instead of a pointer. >> > > bionic's using clang's _Nullable and _Nonnull, which do let you express > this: > https://clang.llvm.org/docs/AttributeReference.html#nullability-attributes > > >> Jₑₙₛ >> >> -- >> :: ICube :::::::::::::::::::::::::::::: deputy director :: >> :: Université de Strasbourg :::::::::::::::::::::: ICPS :: >> :: INRIA Nancy Grand Est :::::::::::::::::::::::: Camus :: >> :: :::::::::::::::::::::::::::::::::::: ☎ +33 368854536 >> <+33%203%2068%2085%2045%2036> :: >> :: https://icube-icps.unistra.fr/index.php/Jens_Gustedt :: >> > [-- Attachment #2: Type: text/html, Size: 2682 bytes --]
On Fri, Feb 24, 2023 at 05:40:10PM +0100, Jₑₙₛ Gustedt wrote:
>
> on Fri, 24 Feb 2023 11:12:11 -0500 you (Tamir Duberstein
> <tamird@google.com>) wrote:
>
> > I agree, the caller's behavior is UB. I'll send them (freetype2) a
> > patch.
> >
> > That said, do we want to avoid internal UB here anyway?
>
> I am not sure that I even understand what "internal UB" is supposed to mean.
>
> > - As mentioned earlier, glibc avoids the UB (and the lock).
> > - llvm-libc does the same starting with
> > https://github.com/llvm/llvm-project/commit/53c251b
> > - uclibc avoids the UB but still locks:
> > https://github.com/gittup/uClibc/blob/9dbf00b/libc/stdio/fread.c#L25
> > - FreeBSD avoids the UB but still locks:
> > https://svnweb.freebsd.org/base/head/lib/libc/stdio/fread.c?view=markup#l76
> > - Android (bionic) avoids the UB but still locks:
> > https://cs.android.com/android/platform/superproject/+/master:bionic/libc/stdio/stdio.cpp;l=1099;drc=4aa8f499f21ebf84101de34d68682d5388667001
> >
> > Does this persuade?
>
> Me personally not much. The only thing that would help applications to
> write portable code is to put an attribute on the pointer argument
> such that bad calls get diagnosed if possible.
Likewise. The recent atol thread seems relevant, where my view was
that it's better to allow the overflow to happen when the caller
invokes UB by passing an argument that overflows, so that a libc built
with sanitizers would naturally cause a trap. The same applies here.
If we make no change, a libc built with sanitizers would automatically
trap when NULL is passed to fread.
Rich