* [musl] [PATCH] fix thread leak on timer_create(SIGEV_THREAD) failure @ 2022-09-08 9:18 Alexey Izbyshev 2022-09-12 14:22 ` Rich Felker 0 siblings, 1 reply; 3+ messages in thread From: Alexey Izbyshev @ 2022-09-08 9:18 UTC (permalink / raw) To: musl After commit 5b74eed3b301e2227385f3bf26d3bb7c2d822cf8 the timer thread doesn't check whether timer_create() actually created the timer, proceeding to wait for a signal that might never arrive. We can't fix this by simply checking for a negative timer_id after pthread_barrier_wait() because we have no way to distinguish a timer creation failure and a request to delete a timer with INT_MAX id if it happens to arrive quickly (a variation of this bug existed before 5b74eed3b301e2227385f3bf26d3bb7c2d822cf8, where the timer would be leaked in this case). So (ab)use cancel field of pthread_t instead. --- src/time/timer_create.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/time/timer_create.c b/src/time/timer_create.c index 4bef2390..cd32c945 100644 --- a/src/time/timer_create.c +++ b/src/time/timer_create.c @@ -43,6 +43,8 @@ static void *start(void *arg) union sigval val = args->sev->sigev_value; pthread_barrier_wait(&args->b); + if (self->cancel) + return 0; for (;;) { siginfo_t si; while (sigwaitinfo(SIGTIMER_SET, &si) < 0); @@ -113,8 +115,10 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict ksev.sigev_signo = SIGTIMER; ksev.sigev_notify = SIGEV_THREAD_ID; ksev.sigev_tid = td->tid; - if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0) + if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0) { timerid = -1; + td->cancel = 1; + } td->timer_id = timerid; pthread_barrier_wait(&args.b); if (timerid < 0) return -1; -- 2.37.2 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [musl] [PATCH] fix thread leak on timer_create(SIGEV_THREAD) failure 2022-09-08 9:18 [musl] [PATCH] fix thread leak on timer_create(SIGEV_THREAD) failure Alexey Izbyshev @ 2022-09-12 14:22 ` Rich Felker 2022-09-12 14:57 ` Alexey Izbyshev 0 siblings, 1 reply; 3+ messages in thread From: Rich Felker @ 2022-09-12 14:22 UTC (permalink / raw) To: Alexey Izbyshev; +Cc: musl On Thu, Sep 08, 2022 at 12:18:56PM +0300, Alexey Izbyshev wrote: > After commit 5b74eed3b301e2227385f3bf26d3bb7c2d822cf8 the timer thread > doesn't check whether timer_create() actually created the timer, > proceeding to wait for a signal that might never arrive. We can't fix > this by simply checking for a negative timer_id after > pthread_barrier_wait() because we have no way to distinguish a timer > creation failure and a request to delete a timer with INT_MAX id if it > happens to arrive quickly (a variation of this bug existed before > 5b74eed3b301e2227385f3bf26d3bb7c2d822cf8, where the timer would be > leaked in this case). So (ab)use cancel field of pthread_t instead. > --- > src/time/timer_create.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/src/time/timer_create.c b/src/time/timer_create.c > index 4bef2390..cd32c945 100644 > --- a/src/time/timer_create.c > +++ b/src/time/timer_create.c > @@ -43,6 +43,8 @@ static void *start(void *arg) > union sigval val = args->sev->sigev_value; > > pthread_barrier_wait(&args->b); > + if (self->cancel) > + return 0; > for (;;) { > siginfo_t si; > while (sigwaitinfo(SIGTIMER_SET, &si) < 0); > @@ -113,8 +115,10 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict > ksev.sigev_signo = SIGTIMER; > ksev.sigev_notify = SIGEV_THREAD_ID; > ksev.sigev_tid = td->tid; > - if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0) > + if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0) { > timerid = -1; > + td->cancel = 1; > + } > td->timer_id = timerid; > pthread_barrier_wait(&args.b); > if (timerid < 0) return -1; > -- > 2.37.2 I'm not really happy with overloading td->cancel like this, but it's probably the best fix at present. The long-term direction for this functionality is hopefully removing the use of kernel timer objects entirely for SIGEV_THREAD timers and instead implementing them with clock_nanosleep, which would eliminate the possibility of SYS_timer_create failure, the possibility of leak, the td->timer_id member, the SIGTIMER reserved signal, and basically all the hacks going on here. Rich ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [musl] [PATCH] fix thread leak on timer_create(SIGEV_THREAD) failure 2022-09-12 14:22 ` Rich Felker @ 2022-09-12 14:57 ` Alexey Izbyshev 0 siblings, 0 replies; 3+ messages in thread From: Alexey Izbyshev @ 2022-09-12 14:57 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 2180 bytes --] On 2022-09-12 17:22, Rich Felker wrote: > On Thu, Sep 08, 2022 at 12:18:56PM +0300, Alexey Izbyshev wrote: >> After commit 5b74eed3b301e2227385f3bf26d3bb7c2d822cf8 the timer thread >> doesn't check whether timer_create() actually created the timer, >> proceeding to wait for a signal that might never arrive. We can't fix >> this by simply checking for a negative timer_id after >> pthread_barrier_wait() because we have no way to distinguish a timer >> creation failure and a request to delete a timer with INT_MAX id if it >> happens to arrive quickly (a variation of this bug existed before >> 5b74eed3b301e2227385f3bf26d3bb7c2d822cf8, where the timer would be >> leaked in this case). So (ab)use cancel field of pthread_t instead. >> --- >> src/time/timer_create.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/src/time/timer_create.c b/src/time/timer_create.c >> index 4bef2390..cd32c945 100644 >> --- a/src/time/timer_create.c >> +++ b/src/time/timer_create.c >> @@ -43,6 +43,8 @@ static void *start(void *arg) >> union sigval val = args->sev->sigev_value; >> >> pthread_barrier_wait(&args->b); >> + if (self->cancel) >> + return 0; >> for (;;) { >> siginfo_t si; >> while (sigwaitinfo(SIGTIMER_SET, &si) < 0); >> @@ -113,8 +115,10 @@ int timer_create(clockid_t clk, struct sigevent >> *restrict evp, timer_t *restrict >> ksev.sigev_signo = SIGTIMER; >> ksev.sigev_notify = SIGEV_THREAD_ID; >> ksev.sigev_tid = td->tid; >> - if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0) >> + if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0) { >> timerid = -1; >> + td->cancel = 1; >> + } >> td->timer_id = timerid; >> pthread_barrier_wait(&args.b); >> if (timerid < 0) return -1; >> -- >> 2.37.2 > > I'm not really happy with overloading td->cancel like this, but it's > probably the best fix at present. I'm not terribly happy too, but short of pulling the real pthread_cancel() here, the only other reasonable fix that I could come up with was to make the timer thread allocate the space for the deleted flag on its stack, like in the attached patch, but it felt a bit heavy. Alexey [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: timer-create-thread-leak.patch --] [-- Type: text/x-diff; name=timer-create-thread-leak.patch, Size: 2064 bytes --] diff --git a/src/time/timer_create.c b/src/time/timer_create.c index 4bef2390..117e5ec6 100644 --- a/src/time/timer_create.c +++ b/src/time/timer_create.c @@ -11,9 +11,15 @@ struct ksigevent { int sigev_tid; }; +struct worker { + pid_t tid; + volatile int deleted; +}; + struct start_args { pthread_barrier_t b; struct sigevent *sev; + struct worker *w; }; static void dummy_0() @@ -38,11 +44,15 @@ static void *start(void *arg) pthread_t self = __pthread_self(); struct start_args *args = arg; jmp_buf jb; + struct worker w = { self->tid }; void (*notify)(union sigval) = args->sev->sigev_notify_function; union sigval val = args->sev->sigev_value; + args->w = &w; pthread_barrier_wait(&args->b); + if (self->timer_id < 0) + return 0; for (;;) { siginfo_t si; while (sigwaitinfo(SIGTIMER_SET, &si) < 0); @@ -51,9 +61,9 @@ static void *start(void *arg) notify(val); pthread_cleanup_pop(1); } - if (self->timer_id < 0) break; + if (w.deleted) break; } - __syscall(SYS_timer_delete, self->timer_id & INT_MAX); + __syscall(SYS_timer_delete, self->timer_id); return 0; } @@ -118,7 +128,7 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict td->timer_id = timerid; pthread_barrier_wait(&args.b); if (timerid < 0) return -1; - *res = (void *)(INTPTR_MIN | (uintptr_t)td>>1); + *res = (void *)(INTPTR_MIN | (uintptr_t)args.w>>1); break; default: errno = EINVAL; diff --git a/src/time/timer_delete.c b/src/time/timer_delete.c index b0bfac09..80baa920 100644 --- a/src/time/timer_delete.c +++ b/src/time/timer_delete.c @@ -5,9 +5,12 @@ int timer_delete(timer_t t) { if ((intptr_t)t < 0) { - pthread_t td = (void *)((uintptr_t)t << 1); - a_store(&td->timer_id, td->timer_id | INT_MIN); - __syscall(SYS_tkill, td->tid, SIGTIMER); + struct worker { + pid_t tid; + volatile int deleted; + } *w = (void *)((uintptr_t)t << 1); + a_store(&w->deleted, 1); + __syscall(SYS_tkill, w->tid, SIGTIMER); return 0; } return __syscall(SYS_timer_delete, t); ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-12 14:57 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-08 9:18 [musl] [PATCH] fix thread leak on timer_create(SIGEV_THREAD) failure Alexey Izbyshev 2022-09-12 14:22 ` Rich Felker 2022-09-12 14:57 ` Alexey Izbyshev
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).