mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] undefined behavior in fread.c
@ 2023-02-24 12:52 Tamir Duberstein
  2023-02-24 13:34 ` Rich Felker
  0 siblings, 1 reply; 16+ messages in thread
From: Tamir Duberstein @ 2023-02-24 12:52 UTC (permalink / raw)
  To: musl

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 12:52 [musl] undefined behavior in fread.c Tamir Duberstein
@ 2023-02-24 13:34 ` Rich Felker
  2023-02-24 13:53   ` Jₑₙₛ Gustedt
  2023-02-24 13:55   ` NRK
  0 siblings, 2 replies; 16+ messages in thread
From: Rich Felker @ 2023-02-24 13:34 UTC (permalink / raw)
  To: Tamir Duberstein; +Cc: musl

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 13:34 ` Rich Felker
@ 2023-02-24 13:53   ` Jₑₙₛ Gustedt
  2023-02-24 13:55   ` NRK
  1 sibling, 0 replies; 16+ messages in thread
From: Jₑₙₛ Gustedt @ 2023-02-24 13:53 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl, Tamir Duberstein

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 13:34 ` Rich Felker
  2023-02-24 13:53   ` Jₑₙₛ Gustedt
@ 2023-02-24 13:55   ` NRK
  2023-02-24 14:07     ` Rich Felker
  1 sibling, 1 reply; 16+ messages in thread
From: NRK @ 2023-02-24 13:55 UTC (permalink / raw)
  To: musl; +Cc: Tamir Duberstein

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 13:55   ` NRK
@ 2023-02-24 14:07     ` Rich Felker
  2023-02-24 14:17       ` NRK
  0 siblings, 1 reply; 16+ messages in thread
From: Rich Felker @ 2023-02-24 14:07 UTC (permalink / raw)
  To: NRK; +Cc: musl, Tamir Duberstein

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 14:07     ` Rich Felker
@ 2023-02-24 14:17       ` NRK
  2023-02-24 14:42         ` Tamir Duberstein
  0 siblings, 1 reply; 16+ messages in thread
From: NRK @ 2023-02-24 14:17 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl, Tamir Duberstein

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 14:17       ` NRK
@ 2023-02-24 14:42         ` Tamir Duberstein
  2023-02-24 15:13           ` NRK
  0 siblings, 1 reply; 16+ messages in thread
From: Tamir Duberstein @ 2023-02-24 14:42 UTC (permalink / raw)
  To: NRK; +Cc: Rich Felker, musl

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 14:42         ` Tamir Duberstein
@ 2023-02-24 15:13           ` NRK
  2023-02-24 16:12             ` Tamir Duberstein
  0 siblings, 1 reply; 16+ messages in thread
From: NRK @ 2023-02-24 15:13 UTC (permalink / raw)
  To: Tamir Duberstein; +Cc: Rich Felker, musl

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 15:13           ` NRK
@ 2023-02-24 16:12             ` Tamir Duberstein
  2023-02-24 16:40               ` Jₑₙₛ Gustedt
  0 siblings, 1 reply; 16+ messages in thread
From: Tamir Duberstein @ 2023-02-24 16:12 UTC (permalink / raw)
  To: NRK; +Cc: Rich Felker, musl

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 16:12             ` Tamir Duberstein
@ 2023-02-24 16:40               ` Jₑₙₛ Gustedt
  2023-02-24 16:42                 ` Tamir Duberstein
                                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Jₑₙₛ Gustedt @ 2023-02-24 16:40 UTC (permalink / raw)
  To: Tamir Duberstein; +Cc: musl, NRK, Rich Felker

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 16:40               ` Jₑₙₛ Gustedt
@ 2023-02-24 16:42                 ` Tamir Duberstein
  2023-02-24 17:00                   ` Jₑₙₛ Gustedt
  2023-02-24 16:42                 ` enh
  2023-02-24 20:07                 ` Rich Felker
  2 siblings, 1 reply; 16+ messages in thread
From: Tamir Duberstein @ 2023-02-24 16:42 UTC (permalink / raw)
  To: Jₑₙₛ Gustedt; +Cc: musl, NRK, Rich Felker

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 16:40               ` Jₑₙₛ Gustedt
  2023-02-24 16:42                 ` Tamir Duberstein
@ 2023-02-24 16:42                 ` enh
  2023-02-24 20:07                 ` Rich Felker
  2 siblings, 0 replies; 16+ messages in thread
From: enh @ 2023-02-24 16:42 UTC (permalink / raw)
  To: musl; +Cc: Tamir Duberstein, NRK, Rich Felker

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 16:42                 ` Tamir Duberstein
@ 2023-02-24 17:00                   ` Jₑₙₛ Gustedt
  2023-02-24 17:07                     ` enh
  0 siblings, 1 reply; 16+ messages in thread
From: Jₑₙₛ Gustedt @ 2023-02-24 17:00 UTC (permalink / raw)
  To: Tamir Duberstein; +Cc: musl, NRK, Rich Felker

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 17:00                   ` Jₑₙₛ Gustedt
@ 2023-02-24 17:07                     ` enh
  2023-02-24 17:32                       ` Tamir Duberstein
  0 siblings, 1 reply; 16+ messages in thread
From: enh @ 2023-02-24 17:07 UTC (permalink / raw)
  To: musl; +Cc: Tamir Duberstein, NRK, Rich Felker

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 17:07                     ` enh
@ 2023-02-24 17:32                       ` Tamir Duberstein
  0 siblings, 0 replies; 16+ messages in thread
From: Tamir Duberstein @ 2023-02-24 17:32 UTC (permalink / raw)
  To: enh; +Cc: musl, NRK, Rich Felker

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

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

* Re: [musl] undefined behavior in fread.c
  2023-02-24 16:40               ` Jₑₙₛ Gustedt
  2023-02-24 16:42                 ` Tamir Duberstein
  2023-02-24 16:42                 ` enh
@ 2023-02-24 20:07                 ` Rich Felker
  2 siblings, 0 replies; 16+ messages in thread
From: Rich Felker @ 2023-02-24 20:07 UTC (permalink / raw)
  To: Jₑₙₛ Gustedt; +Cc: Tamir Duberstein, musl, NRK

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

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

end of thread, other threads:[~2023-02-24 20:07 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-24 12:52 [musl] undefined behavior in fread.c Tamir Duberstein
2023-02-24 13:34 ` Rich Felker
2023-02-24 13:53   ` Jₑₙₛ Gustedt
2023-02-24 13:55   ` NRK
2023-02-24 14:07     ` Rich Felker
2023-02-24 14:17       ` NRK
2023-02-24 14:42         ` Tamir Duberstein
2023-02-24 15:13           ` NRK
2023-02-24 16:12             ` Tamir Duberstein
2023-02-24 16:40               ` Jₑₙₛ Gustedt
2023-02-24 16:42                 ` Tamir Duberstein
2023-02-24 17:00                   ` Jₑₙₛ Gustedt
2023-02-24 17:07                     ` enh
2023-02-24 17:32                       ` Tamir Duberstein
2023-02-24 16:42                 ` enh
2023-02-24 20:07                 ` 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).