[PATCH] Don't return new thread ID from pthread_create.

[PATCH] Don't return new thread ID from pthread_create.

[Piotr Tworek]

From: Piotr Tworek <tworaz666@gmail.com>

According to documentation pthread_create is supposed to return 0 on
success or error code in case of failure. Unfortunately this is not
always the case in musl. In case the application has called
pthread_attr_setinheritsched(&attr, PTHREAD_EXPLICIT_SCHED),
pthread_create will return new thread ID. To make matters worse such non
zero value does not mean thread creation failed. Its quite the oposite.
Thread was created succesfully and start routine passed to pthread_create
will actually get invoked.

To fix the problem simply drop the return statement from the last
if statement in musl's pthread_create. The value currently being returned
from there is a return code from __clone. Negative values indicating
failure have already been handled by previous if statement and positive
value indicates everything went correctly and pthread_create should return 0.
---
 src/thread/pthread_create.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index 2df2e9f9..792cb42e 100644
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -306,7 +306,6 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att
 
 	if (do_sched) {
 		__futexwait(&ssa.futex, -1, 1);
-		if (ret) return ret;
 	}
 
 	*res = new;
-- 
2.16.4

Re: [PATCH] Don't return new thread ID from pthread_create.

[Rich Felker]

On Thu, Aug 16, 2018 at 01:09:33AM +0200, Piotr Tworek wrote:
> From: Piotr Tworek <tworaz666@gmail.com>
> 
> According to documentation pthread_create is supposed to return 0 on
> success or error code in case of failure. Unfortunately this is not
> always the case in musl. In case the application has called
> pthread_attr_setinheritsched(&attr, PTHREAD_EXPLICIT_SCHED),
> pthread_create will return new thread ID. To make matters worse such non
> zero value does not mean thread creation failed. Its quite the oposite.
> Thread was created succesfully and start routine passed to pthread_create
> will actually get invoked.
> 
> To fix the problem simply drop the return statement from the last
> if statement in musl's pthread_create. The value currently being returned
> from there is a return code from __clone. Negative values indicating
> failure have already been handled by previous if statement and positive
> value indicates everything went correctly and pthread_create should return 0.
> ---
>  src/thread/pthread_create.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
> index 2df2e9f9..792cb42e 100644
> --- a/src/thread/pthread_create.c
> +++ b/src/thread/pthread_create.c
> @@ -306,7 +306,6 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att
>  
>  	if (do_sched) {
>  		__futexwait(&ssa.futex, -1, 1);
> -		if (ret) return ret;
>  	}
>  
>  	*res = new;

You're right that there's a bug, but the fix is not correct, and in
some sense makes the problem worse -- pthread_create will wrongly
return success with an invalid pthread_t as the result (since
__start_sched detached itself and exited).

The problem is just that the value of ret isn't being updated to match
the error returned from __start_sched. Rather than:

 	if (do_sched) {
 		__futexwait(&ssa.futex, -1, 1);
-		if (ret) return ret;
 	}

it should look like (untested):

 	if (do_sched) {
 		__futexwait(&ssa.futex, -1, 1);
+		ret = ssa.futex;
		if (ret) return ret;
 	}

Note that this is a new regression and not in any released version
yet. Let me know if you see any problems with the fix I just proposed
and if you get a chance to test that it works, and I'll make sure the
fix goes in for this release.

Rich

Re: [PATCH] Don't return new thread ID from pthread_create.

[tworaz]

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

Hi Rich,

You're absolutely right, my fix is not correct. I should have probably
spent a bit more time analysing the code before making the change ;) I can
confirm your fix solves my original problem. Qt's QThread should be once
again usable on musl based systems :)

/ptw

On Thu, Aug 16, 2018 at 1:30 AM Rich Felker <dalias@libc.org> wrote:

> On Thu, Aug 16, 2018 at 01:09:33AM +0200, Piotr Tworek wrote:
> > From: Piotr Tworek <tworaz666@gmail.com>
> >
> > According to documentation pthread_create is supposed to return 0 on
> > success or error code in case of failure. Unfortunately this is not
> > always the case in musl. In case the application has called
> > pthread_attr_setinheritsched(&attr, PTHREAD_EXPLICIT_SCHED),
> > pthread_create will return new thread ID. To make matters worse such non
> > zero value does not mean thread creation failed. Its quite the oposite.
> > Thread was created succesfully and start routine passed to pthread_create
> > will actually get invoked.
> >
> > To fix the problem simply drop the return statement from the last
> > if statement in musl's pthread_create. The value currently being returned
> > from there is a return code from __clone. Negative values indicating
> > failure have already been handled by previous if statement and positive
> > value indicates everything went correctly and pthread_create should
> return 0.
> > ---
> >  src/thread/pthread_create.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
> > index 2df2e9f9..792cb42e 100644
> > --- a/src/thread/pthread_create.c
> > +++ b/src/thread/pthread_create.c
> > @@ -306,7 +306,6 @@ int __pthread_create(pthread_t *restrict res, const
> pthread_attr_t *restrict att
> >
> >       if (do_sched) {
> >               __futexwait(&ssa.futex, -1, 1);
> > -             if (ret) return ret;
> >       }
> >
> >       *res = new;
>
> You're right that there's a bug, but the fix is not correct, and in
> some sense makes the problem worse -- pthread_create will wrongly
> return success with an invalid pthread_t as the result (since
> __start_sched detached itself and exited).
>
> The problem is just that the value of ret isn't being updated to match
> the error returned from __start_sched. Rather than:
>
>         if (do_sched) {
>                 __futexwait(&ssa.futex, -1, 1);
> -               if (ret) return ret;
>         }
>
> it should look like (untested):
>
>         if (do_sched) {
>                 __futexwait(&ssa.futex, -1, 1);
> +               ret = ssa.futex;
>                 if (ret) return ret;
>         }
>
> Note that this is a new regression and not in any released version
> yet. Let me know if you see any problems with the fix I just proposed
> and if you get a chance to test that it works, and I'll make sure the
> fix goes in for this release.
>
> Rich
>

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