mailing list of musl libc
 help / color / mirror / code / Atom feed
* [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).