misc minor undefined behaviors in musl

misc minor undefined behaviors in musl

[Pascal Cuoq]

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

Hello,

this message contains a collection of three issues that we have found running musl's source code inside tis-interpreter. They are all very minor, so it seems like the best format may be for me to put all of them in a single message, so that subscribers that aren't interested by C minutiae ignore the discussion more easily.

I can provide more detail on each of them, but again perhaps I'll only do this if someone thinks the individual issue deserves it. Unlike UB that the musl developers are certainly aware of but that involve difficult trade-offs (e.g. HASZERO in string functions), these seemed to be easy to fix if one deems it worth it, and a fix is suggested each time.

a) pointer arithmetic on NULL pointer in __fputwc_unlocked

• f->wpos may be NULL when reaching the test:
 } else if (f->wpos + MB_LEN_MAX < f->wend) {
 Pointer arithmetic using a null pointer is UB.

• fix: add a f->wpos != NULL test.

b) __pthread_tsd_run_dtors does not have a prototype

• in cleanup_fromsig, __pthread_tsd_run_dtors is called with an argument:
https://git.musl-libc.org/cgit/musl/tree/src/time/timer_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n27
It is defined without a prototype here:
https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n38
It is also called without an argument here:
https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n38
and weak_alias'd to dummy_0 here:
https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n18

• it seems like the above is on purpose, especially the definition of __pthread_tsd_run_dtors without a prototype, which is still valid in C11and does not mean the same thing as expecting void. Still, for the sake of simplicity, would it be possible to uniformize to __pthread_tsd_run_dtors always taking an argument? Can the invocation without an argument in pthread_create.c end up being linked with an implementation of __pthread_tsd_run_dtors that takes an argument and actually uses it?

c) definition of MAP_FAILED

• tis-interpreter does not like the definition of MAP_FAILED as (void*)-1. While mmap is a bit outside the scope of what it was initially designed to do, some simple uses of mmap can be emulated pretty well on a case-by-case basis by making mmap return the address of a preallocated array of chars. When we do this, then the comparison to MAP_FAILED at the call site becomes the next problem, because unlike (void *)0, nothing guarantees that (void*)-1 does not happen to be, by bad luck, the address of the array.

• we fixed this by defining MAP_FAILED as the address of a const char variable defined for this purpose. Since everyone else is defining MAP_FAILED as (void*)-1, this will never be wrong, but I wonder whether someone can see ways in which our solution is better or worse than the original.


Best regards,

Pascal






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

Re: misc minor undefined behaviors in musl

[Rich Felker]

On Thu, Nov 09, 2017 at 09:19:03PM +0000, Pascal Cuoq wrote:
> Hello,
> 
> this message contains a collection of three issues that we have
> found running musl's source code inside tis-interpreter. They are
> all very minor, so it seems like the best format may be for me to
> put all of them in a single message, so that subscribers that aren't
> interested by C minutiae ignore the discussion more easily.
> 
> I can provide more detail on each of them, but again perhaps I'll
> only do this if someone thinks the individual issue deserves it.
> Unlike UB that the musl developers are certainly aware of but that
> involve difficult trade-offs (e.g. HASZERO in string functions),
> these seemed to be easy to fix if one deems it worth it, and a fix
> is suggested each time.
> 
> a) pointer arithmetic on NULL pointer in __fputwc_unlocked
> 
> • f->wpos may be NULL when reaching the test:
>  } else if (f->wpos + MB_LEN_MAX < f->wend) {
>  Pointer arithmetic using a null pointer is UB.
> 
> • fix: add a f->wpos != NULL test.

Normally I'd want to just compare f->wend - f->wpos, but subtracting 2
null pointers is also a problem. This problem already exists in
several other places. I believe the only clean solution that doesn't
worsen codegen is to assign a dummy value instead of null, but then
we'd have to do it uniformly everywhere AND fix a few places where
check-against-null is used to see if it's initialized to the desired
(read/write) mode. This is all a huge mess of a known issue, and I'm
not sure how to solve it cleanly withour major uglification and/or
harm to code generation.

> b) __pthread_tsd_run_dtors does not have a prototype
> 
> • in cleanup_fromsig, __pthread_tsd_run_dtors is called with an argument:
> https://git.musl-libc.org/cgit/musl/tree/src/time/timer_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n27
> It is defined without a prototype here:
> https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n38
> It is also called without an argument here:
> https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n38
> and weak_alias'd to dummy_0 here:
> https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n18
> 
> • it seems like the above is on purpose, especially the definition
> of __pthread_tsd_run_dtors without a prototype, which is still valid
> in C11and does not mean the same thing as expecting void. Still, for
> the sake of simplicity, would it be possible to uniformize to
> __pthread_tsd_run_dtors always taking an argument? Can the
> invocation without an argument in pthread_create.c end up being
> linked with an implementation of __pthread_tsd_run_dtors that takes
> an argument and actually uses it?

This looks like just a failure to update the one in timer_create.c.
Rather than spending time analzing the issue we should just fix it;
the argument is not needed.

> c) definition of MAP_FAILED
> 
> • tis-interpreter does not like the definition of MAP_FAILED as
> (void*)-1. While mmap is a bit outside the scope of what it was
> initially designed to do, some simple uses of mmap can be emulated
> pretty well on a case-by-case basis by making mmap return the
> address of a preallocated array of chars. When we do this, then the
> comparison to MAP_FAILED at the call site becomes the next problem,
> because unlike (void *)0, nothing guarantees that (void*)-1 does not
> happen to be, by bad luck, the address of the array.

Multiple things guarantee this. For one, MAP_FAILED is just defined by
POSIX that way, so the implementation of mmap must ensure that the
return value on success is never equal to MAP_FAILED, whatever the
implementation defines it as. But that's already guaranteed for this
particular value anyway, since mmap is required to return page-aligned
memory.

> • we fixed this by defining MAP_FAILED as the address of a const char
> variable defined for this purpose. Since everyone else is defining
> MAP_FAILED as (void*)-1, this will never be wrong, but I wonder
> whether someone can see ways in which our solution is better or
> worse than the original.

It's a nice idea, but this can't actually be changed because it's
application ABI. The application needs to be able to compare against
MAP_FAILED to determine if mmap failed.

Rich

Re: misc minor undefined behaviors in musl

[Rich Felker]

On Thu, Nov 09, 2017 at 04:53:31PM -0500, Rich Felker wrote:
> > b) __pthread_tsd_run_dtors does not have a prototype
> > 
> > • in cleanup_fromsig, __pthread_tsd_run_dtors is called with an argument:
> > https://git.musl-libc.org/cgit/musl/tree/src/time/timer_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n27
> > It is defined without a prototype here:
> > https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n38
> > It is also called without an argument here:
> > https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n38
> > and weak_alias'd to dummy_0 here:
> > https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_create.c?id=1b9406b03c0a94ebe2076a8fc1746a8c45e78a83#n18
> > 
> > • it seems like the above is on purpose, especially the definition
> > of __pthread_tsd_run_dtors without a prototype, which is still valid
> > in C11and does not mean the same thing as expecting void. Still, for
> > the sake of simplicity, would it be possible to uniformize to
> > __pthread_tsd_run_dtors always taking an argument? Can the
> > invocation without an argument in pthread_create.c end up being
> > linked with an implementation of __pthread_tsd_run_dtors that takes
> > an argument and actually uses it?
> 
> This looks like just a failure to update the one in timer_create.c.
> Rather than spending time analzing the issue we should just fix it;
> the argument is not needed.

Indeed, commit a6054e3c94aa0491d7366e4b05ae0d73f661bfe2 introduced the
mismatch. Fixing it. Thanks!

Rich

Re: misc minor undefined behaviors in musl

[Pascal Cuoq]

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

Hello,

On 10 Nov 2017, at 02:09, Rich Felker <dalias@libc.org<mailto:dalias@libc.org>> wrote:

Indeed, commit a6054e3c94aa0491d7366e4b05ae0d73f661bfe2 introduced the
mismatch. Fixing it.

While you are focusing on this part of the code, how about changing the definition of __pthread_tsd_run_dtors in src/thread/pthread_key_create.c to one with a prototype?

void __pthread_tsd_run_dtors(void)
{

This is what made me unsure you even wanted to hear about the mismatch.

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