[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20171109215331.GN1627@brightrain.aerifal.cx>
Date: Thu, 9 Nov 2017 16:53:31 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: misc minor undefined behaviors in musl
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
