* [PATCH] Don't return new thread ID from pthread_create. @ 2018-08-15 23:09 Piotr Tworek 2018-08-15 23:29 ` Rich Felker 0 siblings, 1 reply; 3+ messages in thread From: Piotr Tworek @ 2018-08-15 23:09 UTC (permalink / raw) To: musl; +Cc: 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Don't return new thread ID from pthread_create. 2018-08-15 23:09 [PATCH] Don't return new thread ID from pthread_create Piotr Tworek @ 2018-08-15 23:29 ` Rich Felker 2018-08-16 6:34 ` tworaz 0 siblings, 1 reply; 3+ messages in thread From: Rich Felker @ 2018-08-15 23:29 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Don't return new thread ID from pthread_create. 2018-08-15 23:29 ` Rich Felker @ 2018-08-16 6:34 ` tworaz 0 siblings, 0 replies; 3+ messages in thread From: tworaz @ 2018-08-16 6:34 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-16 6:34 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-15 23:09 [PATCH] Don't return new thread ID from pthread_create Piotr Tworek 2018-08-15 23:29 ` Rich Felker 2018-08-16 6:34 ` tworaz
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).