[musl] Difference in pthread behavior between glibc and musl

[musl] Difference in pthread behavior between glibc and musl

[Appelmans, Madeleine]

[-- Attachment #1.1: Type: text/plain, Size: 818 bytes --]

Hello,

There seems to be a difference in pthread behavior when compiling with glibc and using the musl-gcc wrapper. The attached snippet of code creates a destructor attribute which deletes a pthread key. The code never actually creates the pthread key. This code segfaults when compiled using musl-gcc, and does not segfault when compiled with gcc.

Best guess at what is going on: When creating a pthread key, musl initializes a field called tsd<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n37>. When deleting a key, musl assumes that initialization has been done, and dereferences tsd without checking that it exists: see here<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n65>. This dereference may be the source of the segfault.

Thanks,
Madeleine

[-- Attachment #1.2: Type: text/html, Size: 2894 bytes --]

[-- Attachment #2: pthread_test.c --]
[-- Type: application/octet-stream, Size: 217 bytes --]

#include <pthread.h>

static pthread_key_t per_thread_key;

static void __attribute__((destructor)) key_cleanup(void)
{
    pthread_key_delete(per_thread_key);
}

int main(int argc, char **argv)
{
    // Do nothing
}

Re: [musl] Difference in pthread behavior between glibc and musl

[Markus Wichmann]

Am Tue, Jul 11, 2023 at 07:19:50PM +0000 schrieb Appelmans, Madeleine:
> Hello,
>
> There seems to be a difference in pthread behavior when compiling with
> glibc and using the musl-gcc wrapper. The attached snippet of code
> creates a destructor attribute which deletes a pthread key. The code
> never actually creates the pthread key. This code segfaults when
> compiled using musl-gcc, and does not segfault when compiled with gcc.
>

Undefined behavior is undefined. And it is undefined behavior to be
calling pthread_key_delete() with a key that was not previously acquired
from pthread_key_create(). POSIX encourages - but does not require -
implementations to detect invalid keys and return an error then. And
musl doesn't.

Ciao,
Markus

Re: [musl] Difference in pthread behavior between glibc and musl

[Rich Felker]

On Tue, Jul 11, 2023 at 07:19:50PM +0000, Appelmans, Madeleine wrote:
> Hello,
> 
> There seems to be a difference in pthread behavior when compiling
> with glibc and using the musl-gcc wrapper. The attached snippet of
> code creates a destructor attribute which deletes a pthread key. The
> code never actually creates the pthread key. This code segfaults
> when compiled using musl-gcc, and does not segfault when compiled
> with gcc.
> 
> Best guess at what is going on: When creating a pthread key, musl
> initializes a field called
> tsd<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n37>.
> When deleting a key, musl assumes that initialization has been done,
> and dereferences tsd without checking that it exists: see
> here<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n65>.
> This dereference may be the source of the segfault.

This is completely expected; the behavior is undefined because you
passed to pthread_key_delete a value which was not acquired via
pthread_key_create.

If it happens not to crash on glibc, that doesn't mean it's okay to do
it. It will end up deleting whatever key happens to correspond to the
zero-initialized pthread_key_t object, which may be a key that was
allocated for use by some other part of the program when it called
pthread_key_create. (In other words, you have a type of double-free or
use-after-free bug.) Your program logic must ensure you refrain from
doing that.

Rich

Re: [musl] Difference in pthread behavior between glibc and musl

[Gabriel Ravier]

On 7/12/23 04:48, Rich Felker wrote:
> On Tue, Jul 11, 2023 at 07:19:50PM +0000, Appelmans, Madeleine wrote:
>> Hello,
>>
>> There seems to be a difference in pthread behavior when compiling
>> with glibc and using the musl-gcc wrapper. The attached snippet of
>> code creates a destructor attribute which deletes a pthread key. The
>> code never actually creates the pthread key. This code segfaults
>> when compiled using musl-gcc, and does not segfault when compiled
>> with gcc.
>>
>> Best guess at what is going on: When creating a pthread key, musl
>> initializes a field called
>> tsd<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n37>.
>> When deleting a key, musl assumes that initialization has been done,
>> and dereferences tsd without checking that it exists: see
>> here<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n65>.
>> This dereference may be the source of the segfault.
> This is completely expected; the behavior is undefined because you
> passed to pthread_key_delete a value which was not acquired via
> pthread_key_create.
>
> If it happens not to crash on glibc, that doesn't mean it's okay to do
> it. It will end up deleting whatever key happens to correspond to the
> zero-initialized pthread_key_t object, which may be a key that was
> allocated for use by some other part of the program when it called
> pthread_key_create. (In other words, you have a type of double-free or
> use-after-free bug.) Your program logic must ensure you refrain from
> doing that.
>
> Rich

Hmmm, this does indeed seem to be the case ever since 
SUSv4/XPG7/POSIX.1-2008 removed the EINVAL error from the specification 
of pthread_key_delete, but the requirement to detect this error is 
present in SUSv3/XPG6/POSIX.1-2001 (up to and including the 2004 
edition). so if you want musl to be able to say it conforms to that 
standard, it would have to implement this, which after a quick look at 
musl's source code w.r.t. pthread keys doesn't seem particularly burdensome.

Personally I'd be in favor of having this detection occur regardless of 
whether musl aims for conformance to older standards given that POSIX 
still now explicitly recommends it even in the standards where it is not 
mandatory anymore, which is something I've usually seen done in cases 
where they wanted to specify a behavior but had to back down on making 
it mandatory on the basis of certain implementations complaining that it 
was too complicated to implement and/or wanting to retain another 
behavior for backwards compatibility or other stuff like that - i.e. 
they would very much prefer implementations where it isn't particularly 
burdensome to implement it to do so.

Re: [musl] Difference in pthread behavior between glibc and musl

[Rich Felker]

On Thu, Jul 13, 2023 at 03:00:55AM +0200, Gabriel Ravier wrote:
> On 7/12/23 04:48, Rich Felker wrote:
> >On Tue, Jul 11, 2023 at 07:19:50PM +0000, Appelmans, Madeleine wrote:
> >>Hello,
> >>
> >>There seems to be a difference in pthread behavior when compiling
> >>with glibc and using the musl-gcc wrapper. The attached snippet of
> >>code creates a destructor attribute which deletes a pthread key. The
> >>code never actually creates the pthread key. This code segfaults
> >>when compiled using musl-gcc, and does not segfault when compiled
> >>with gcc.
> >>
> >>Best guess at what is going on: When creating a pthread key, musl
> >>initializes a field called
> >>tsd<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n37>.
> >>When deleting a key, musl assumes that initialization has been done,
> >>and dereferences tsd without checking that it exists: see
> >>here<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n65>.
> >>This dereference may be the source of the segfault.
> >This is completely expected; the behavior is undefined because you
> >passed to pthread_key_delete a value which was not acquired via
> >pthread_key_create.
> >
> >If it happens not to crash on glibc, that doesn't mean it's okay to do
> >it. It will end up deleting whatever key happens to correspond to the
> >zero-initialized pthread_key_t object, which may be a key that was
> >allocated for use by some other part of the program when it called
> >pthread_key_create. (In other words, you have a type of double-free or
> >use-after-free bug.) Your program logic must ensure you refrain from
> >doing that.
> >
> >Rich
> 
> Hmmm, this does indeed seem to be the case ever since
> SUSv4/XPG7/POSIX.1-2008 removed the EINVAL error from the
> specification of pthread_key_delete, but the requirement to detect
> this error is present in SUSv3/XPG6/POSIX.1-2001 (up to and
> including the 2004 edition). so if you want musl to be able to say
> it conforms to that standard, it would have to implement this, which
> after a quick look at musl's source code w.r.t. pthread keys doesn't
> seem particularly burdensome.

There was never a requirement to detect UAF/DF type errors. There was
a requirement ("shall fail"), *if the implementation does detect it*,
to fail with EINVAL. This was nonsensical because there was no
requirement to do the detection (and in general the detection is
impossible; that's the nature of UAF/DF) so the confusing text was
fixed. But there was no change in the actual requirement.

> Personally I'd be in favor of having this detection occur regardless
> of whether musl aims for conformance to older standards given that
> POSIX still now explicitly recommends it even in the standards where
> it is not mandatory anymore, which is something I've usually seen

It is not "explicitly recommended", and it's against best practices to
return an error rather than trapping on UB.

Rich

Re: [musl] Difference in pthread behavior between glibc and musl

[Gabriel Ravier]

On 7/13/23 03:50, Rich Felker wrote:
> On Thu, Jul 13, 2023 at 03:00:55AM +0200, Gabriel Ravier wrote:
>> On 7/12/23 04:48, Rich Felker wrote:
>>> On Tue, Jul 11, 2023 at 07:19:50PM +0000, Appelmans, Madeleine wrote:
>>>> Hello,
>>>>
>>>> There seems to be a difference in pthread behavior when compiling
>>>> with glibc and using the musl-gcc wrapper. The attached snippet of
>>>> code creates a destructor attribute which deletes a pthread key. The
>>>> code never actually creates the pthread key. This code segfaults
>>>> when compiled using musl-gcc, and does not segfault when compiled
>>>> with gcc.
>>>>
>>>> Best guess at what is going on: When creating a pthread key, musl
>>>> initializes a field called
>>>> tsd<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n37>.
>>>> When deleting a key, musl assumes that initialization has been done,
>>>> and dereferences tsd without checking that it exists: see
>>>> here<https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_key_create.c#n65>.
>>>> This dereference may be the source of the segfault.
>>> This is completely expected; the behavior is undefined because you
>>> passed to pthread_key_delete a value which was not acquired via
>>> pthread_key_create.
>>>
>>> If it happens not to crash on glibc, that doesn't mean it's okay to do
>>> it. It will end up deleting whatever key happens to correspond to the
>>> zero-initialized pthread_key_t object, which may be a key that was
>>> allocated for use by some other part of the program when it called
>>> pthread_key_create. (In other words, you have a type of double-free or
>>> use-after-free bug.) Your program logic must ensure you refrain from
>>> doing that.
>>>
>>> Rich
>> Hmmm, this does indeed seem to be the case ever since
>> SUSv4/XPG7/POSIX.1-2008 removed the EINVAL error from the
>> specification of pthread_key_delete, but the requirement to detect
>> this error is present in SUSv3/XPG6/POSIX.1-2001 (up to and
>> including the 2004 edition). so if you want musl to be able to say
>> it conforms to that standard, it would have to implement this, which
>> after a quick look at musl's source code w.r.t. pthread keys doesn't
>> seem particularly burdensome.
> There was never a requirement to detect UAF/DF type errors. There was
> a requirement ("shall fail"), *if the implementation does detect it*,
> to fail with EINVAL. This was nonsensical because there was no
> requirement to do the detection (and in general the detection is
> impossible; that's the nature of UAF/DF) so the confusing text was
> fixed. But there was no change in the actual requirement.
>
>> Personally I'd be in favor of having this detection occur regardless
>> of whether musl aims for conformance to older standards given that
>> POSIX still now explicitly recommends it even in the standards where
>> it is not mandatory anymore, which is something I've usually seen
> It is not "explicitly recommended", and it's against best practices to
> return an error rather than trapping on UB.
>
> Rich

Well, the standard states that:

 > If an implementation detects that the value specified by the key 
argument to pthread_key_delete( ) does not refer to a a key value 
obtained from pthread_key_create( ) or refers to a key that has been 
deleted with pthread_key_delete( ), it is recommended that the function 
should fail and report an [EINVAL] error.

so I don't get you mean by 'It is not "explicitly recommended"': I can't 
imagine any statement that would make it more clear it is explicitly 
recommended. Though, on the other hand, I generally agree that it's not 
a great idea to just return an error on invalid keys instead of 
crashing, even though doing so is consistent with e.g. EBADF being 
returned for invalid file descriptors in system calls, even though an 
invalid fd pretty much systematically indicates a grave bug in a program.

Re: [musl] Difference in pthread behavior between glibc and musl

[Markus Wichmann]

Am Thu, Jul 13, 2023 at 04:02:54AM +0200 schrieb Gabriel Ravier:
> so I don't get you mean by 'It is not "explicitly recommended"':

Reading comprehension, please! The sentence you quoted contains a
condition; one that Rich already pointed out to you. The condition is
not fulfilled; musl does not detect that the key is invalid, and
therefore the entire thing is null for this implementation. Let alone
that the sentence is in a section explicitly marked "informative" (as
opposed to normative).

Ciao,
Markus

Re: [musl] Difference in pthread behavior between glibc and musl

[Gabriel Ravier]

On 7/13/23 18:27, Markus Wichmann wrote:
> Am Thu, Jul 13, 2023 at 04:02:54AM +0200 schrieb Gabriel Ravier:
>> so I don't get you mean by 'It is not "explicitly recommended"':
> Reading comprehension, please! The sentence you quoted contains a
> condition; one that Rich already pointed out to you. The condition is
> not fulfilled; musl does not detect that the key is invalid, and
> therefore the entire thing is null for this implementation. Let alone
> that the sentence is in a section explicitly marked "informative" (as
> opposed to normative).
>
> Ciao,
> Markus

I know it's not normative - that doesn't seem like it would change 
anything given that regardless is a recommendation, not a requirement.

The explanation you've given does makes sense though - the wording seems 
oddly ambiguous, but that does seem to be what it's trying to say.