[proposal] Add detection of thread ID in pthread-related interfaces

[proposal] Add detection of thread ID in pthread-related interfaces

[pengyuanhong]

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

Hello,

I find that all pthread-related interfaces directly access the input
parameter `pthread_t` without any check. If I pass an invalid thread ID
(e.g. an exited thread ID) to these interfaces, then segment fault
happens.

Both glibc and freebsd can do simple detection of thread ID(pthread_t)
passed by user and return ESRCH when no thread can be found. They
put all threads in a list or hash table, and update this list or table every
time a thread is created or exits.

From the user's point of view, segment fault is unbearable and is not
*         recoverable in most cases. Instead, returning an error of ESRCH
*         seems more acceptable.

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

Re: [proposal] Add detection of thread ID in pthread-related interfaces

[Szabolcs Nagy]

* pengyuanhong <pengyuanhong@huawei.com> [2019-06-11 11:36:59 +0000]:
> I find that all pthread-related interfaces directly access the input
> parameter `pthread_t` without any check. If I pass an invalid thread ID
> (e.g. an exited thread ID) to these interfaces, then segment fault
> happens.
> 
> Both glibc and freebsd can do simple detection of thread ID(pthread_t)
> passed by user and return ESRCH when no thread can be found. They

that's a historical bug in posix: it required ESRCH
which is not possible when the thread id is reused,
so all such requirments were removed in posix 2008
https://collaboration.opengroup.org/austin/interps/documents/14366/AI-142.txt

passing invalid id is simply undefined now, an
implementation may still detect the condition and
then posix recommends ESRCH in the rationale
presumably for backward compatibility:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_join.html

but since applications must not rely on ESRCH,
it has questionable value: the obvious safe and
secure handling of ub is guaranteed immediate
crash: the api contract got violated, the
application is misbehaving so there is no reason
to believe returning an error will make it
non-misbehave.

the only way forward is to fix the broken
application code, the runtime and tooling can
only help by providing better diagnostics
about what went wrong.

> put all threads in a list or hash table, and update this list or table every
> time a thread is created or exits.
> 
> >From the user's point of view, segment fault is unbearable and is not
> *         recoverable in most cases. Instead, returning an error of ESRCH
> *         seems more acceptable.

Re: [proposal] Add detection of thread ID in pthread-related interfaces

[Rich Felker]

On Tue, Jun 11, 2019 at 03:57:42PM +0200, Szabolcs Nagy wrote:
> * pengyuanhong <pengyuanhong@huawei.com> [2019-06-11 11:36:59 +0000]:
> > I find that all pthread-related interfaces directly access the input
> > parameter `pthread_t` without any check. If I pass an invalid thread ID
> > (e.g. an exited thread ID) to these interfaces, then segment fault
> > happens.
> > 
> > Both glibc and freebsd can do simple detection of thread ID(pthread_t)
> > passed by user and return ESRCH when no thread can be found. They
> 
> that's a historical bug in posix: it required ESRCH
> which is not possible when the thread id is reused,
> so all such requirments were removed in posix 2008
> https://collaboration.opengroup.org/austin/interps/documents/14366/AI-142.txt
> 
> passing invalid id is simply undefined now, an
                                         ^^^^

It was always undefined; the text stating that it's undefined was
present in old POSIX too. The "shall fail" text in the ESRCH errors
was if "the implementation has detected..." or similar, which oddly
imposed a requirement to report something if it was detected, but
didn't (and fundamentally couldn't, since it's undetectable with
identifier reusage) impose a requirement to detect.

The committee recognized that this made no sense and fixed it by
removing the "shall fail" text. This was not a functional change, only
a clarification, and thus didn't require a new version of the
standard, just a TC.

Rich

Re: Re: [proposal] Add detection of thread ID in pthread-related interfaces

[Li Yu]

Thanks for your quick and detailed replies first.

As our copy of POSIX 2003.1™-2017, it said such texts in RATIONALE section of pthread_cancel() feature,

'If an implementation detects use of a thread ID after the end of its lifetime, it is recommended
that the function should fail and report an [ESRCH] error. '

I think that it may be a recommended bebhavior in recent revison of POSIX spec.

Another side, in real use cases, many applications are wrote under a major libc implementation first, instead of be wrote according to POSIX spec texts, so I personally think that compatiblity of libc implementation is important than POSIX spec texts, always. In fact, we don't have too many widely usable UNIX variants now.

Last,  I think that features of various libc implementations are different is easy to understand and accept, however, someone are crash for same feature is not welcome :) If we wanted to use musl as a core libc in a open system to support various even third-party closed-sources applications, the every new crash after porting new system is not a good news. so, it seem that eveny such open system need to maintain a in-house patch set to provide better robustness.

En, I see, may be, the design goal of musl is not for open system ?

Thanks

--------------
Li Yu 


>On Tue, Jun 11, 2019 at 03:57:42PM +0200, Szabolcs Nagy wrote:
>> * pengyuanhong <pengyuanhong@huawei.com> [2019-06-11 11:36:59 +0000]:
>> > I find that all pthread-related interfaces directly access the input
>> > parameter `pthread_t` without any check. If I pass an invalid thread ID
>> > (e.g. an exited thread ID) to these interfaces, then segment fault
>> > happens.
>> >
>> > Both glibc and freebsd can do simple detection of thread ID(pthread_t)
>> > passed by user and return ESRCH when no thread can be found. They
>>
>> that's a historical bug in posix: it required ESRCH
>> which is not possible when the thread id is reused,
>> so all such requirments were removed in posix 2008
>> https://collaboration.opengroup.org/austin/interps/documents/14366/AI-142.txt
>>
>> passing invalid id is simply undefined now, an
>                                         ^^^^
>
>It was always undefined; the text stating that it's undefined was
>present in old POSIX too. The "shall fail" text in the ESRCH errors
>was if "the implementation has detected..." or similar, which oddly
>imposed a requirement to report something if it was detected, but
>didn't (and fundamentally couldn't, since it's undetectable with
>identifier reusage) impose a requirement to detect.
>
>The committee recognized that this made no sense and fixed it by
>removing the "shall fail" text. This was not a functional change, only
>a clarification, and thus didn't require a new version of the
>standard, just a TC.
>
>Rich
>

Re: Re: [proposal] Add detection of thread ID in pthread-related interfaces

[Rich Felker]

On Tue, Jun 11, 2019 at 11:58:00PM +0800, Li Yu wrote:
> 
> Thanks for your quick and detailed replies first.
> 
> As our copy of POSIX 2003.1™-2017, it said such texts in RATIONALE section of pthread_cancel() feature,
> 
> 'If an implementation detects use of a thread ID after the end of its lifetime, it is recommended
> that the function should fail and report an [ESRCH] error. '
> 
> I think that it may be a recommended bebhavior in recent revison of POSIX spec.
> 
> Another side, in real use cases, many applications are wrote under a
> major libc implementation first, instead of be wrote according to
> POSIX spec texts, so I personally think that compatiblity of libc
> implementation is important than POSIX spec texts, always. In fact,
> we don't have too many widely usable UNIX variants now.

If it was written to glibc and relies on this behavior, you have an
extremely dangerous use-after-free bug that was not caught. Under
slightly different usage patterns, you could very well end up not
getting an error, but instead cancelling, sending a signal to,
detaching, or joining an unrelated thread that was created later. This
has all of the cascading fault problems inherent in use-after-free,
fd-use-after-close, etc.

By crashing immediately, musl has alerted you to the issue and ensured
that it gets fixed before something blows up in production.

I've advocated a lot on this issue, and glibc has also adopted this
convention based on text I wrote:

https://sourceware.org/glibc/wiki/Style_and_Conventions#Invalid_pointers

> Last,  I think that features of various libc implementations are
> different is easy to understand and accept, however, someone are
> crash for same feature is not welcome :) If we wanted to use musl as
> a core libc in a open system to support various even third-party
> closed-sources applications, the every new crash after porting new
> system is not a good news. so, it seem that eveny such open system
> need to maintain a in-house patch set to provide better robustness.

You're looking at this backwards. You've gained the ability to
immediately catch your dangerous bugs before you ship them rather than
having them only show up when a particular race is hit.

Rich

Re: [proposal] Add detection of thread ID in pthread-related interfaces

[Rich Felker]

On Tue, Jun 11, 2019 at 11:36:59AM +0000, pengyuanhong wrote:
> Hello,
> 
> I find that all pthread-related interfaces directly access the input
> parameter `pthread_t` without any check. If I pass an invalid thread ID
> (e.g. an exited thread ID) to these interfaces, then segment fault
> happens.
> 
> Both glibc and freebsd can do simple detection of thread ID(pthread_t)
> passed by user and return ESRCH when no thread can be found. They
> put all threads in a list or hash table, and update this list or table every
> time a thread is created or exits.

This description of why glibc returns ESRCH is incorrect; it has
nothing to do with keeping a list, which would require >O(1) lookup
and global synchronization on each operation referring to a thread id.
Rather, they just don't free exited threads, but keep the memory
cached to reuse for future calls to pthread_create with a marker that
it's not currently live. This allows trivial detection and reporting
that the id is not currently valid, but also encourages rapid reuse of
the same ids that were just freed, making use-after-free bugs with
pthread_t's much more dangerous.

Rich