[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAMbhsRTrqwB98Jo2kUmt33J=0oA6wTJ9e_+Mr6a8P=yj8HtQxg@mail.gmail.com>
Date: Thu, 3 Sep 2026 18:24:19 -0700
From: Colin Cross <ccross@...roid.com>
To: musl@...ts.openwall.com
Subject: Re: Fix deadlock in timer_create when syscall fails
On Thu, Sep 3, 2026 at 4:27 PM Szabolcs Nagy <nsz@...t70.net> wrote:
>
> * Colin Cross <ccross@...roid.com> [2026-09-03 10:26:12 -0700]:
> > Subject: [PATCH] SIGEV_THREAD timers: fix deadlock when timer_create syscall
> > fails
> >
> > sem_wait is a cancellation point. If the parent thread in timer_create
> > sets td->cancel = 1 before the child thread reaches sem_wait then
> > the thread will exit and sem_post will never be called, causing
> > the parent thread to deadlock in sem_wait.
> >
> > 3ad3fa962efee12067d68c3405a537dce156a7ac set td->cancel = 1 to fix
> > a thread leak when the syscall failed. The switch to two-way semaphores
> > cde213f9c3ac1aa168581222edee6a6642113323 allows checking if self->timer_id
> > is -1 instead of self->cancel because timer_delete can't have been called
> > before the sem_post allows the parent thread to return from timer_create.
>
> this looks ok, i have minor comments only
>
> your orig mail had
>
> This was observed in a test that called
> timer_create(CLOCK_BOOTTIME_ALARM, SIGEV_THREAD, ...) without
> CAP_WAKE_ALARM, causing the kernel to return EPERM.
>
> i prefer to include such details in the commit.
>
> > ---
> > src/time/timer_create.c | 22 +++++++++++++++-------
> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/time/timer_create.c b/src/time/timer_create.c
> > index cc6c2236..a89f45af 100644
> > --- a/src/time/timer_create.c
> > +++ b/src/time/timer_create.c
> > @@ -45,19 +45,28 @@ static void *start(void *arg)
> > pthread_t self = __pthread_self();
> > struct start_args *args = arg;
> > jmp_buf jb;
> > + int cs;
>
> not needed.
Oops, I'll drop that in the next version.
> >
> > void (*notify)(union sigval) = args->sev->sigev_notify_function;
> > union sigval val = args->sev->sigev_value;
> > + int cancel = 0;
> >
> > - /* The two-way semaphore synchronization ensures that we see
> > - * self->cancel set by the parent if timer creation failed or
> > - * self->timer_id if it succeeded, and informs the parent that
> > - * we are done accessing the arguments so that the parent can
> > - * proceed past their block lifetime. */
> > + /* Waiting on sem1 ensures we see self->timer_id. */
> > while (sem_wait(&args->sem1));
> > +
> > + /* If self->timer_id is -1 while the parent thread is still in
> > + * timer_create (before the sem_post to sem2) then the timer was
> > + * never created. If it is -1 later after the sem_post then
> > + * it was cancelled via timer_delete. */
> > + if (self->timer_id < 0)
> > + cancel = 1;
> > +
> > + /* Incrementing sem2 informs the parent that we are done checking
> > + * the initial value of self->timer_id, and accessing the arguments
> > + * so that the parent can proceed past their block lifetime. */
> > sem_post(&args->sem2);
> >
> > - if (self->cancel)
> > + if (cancel)
> > return 0;
>
> or
>
> int timerid;
> while (sem_wait(...));
> timerid = self->timer_id; // only one if this way
> sem_post(...);
> if (timerid < 0) return 0;
I considered that, but it seemed confusing to create a local timerid
variable when the later loop needs to re-read self->timer_id each
time.
> > for (;;) {
> > siginfo_t si;
> > @@ -135,7 +144,6 @@ int timer_create(clockid_t clk, struct sigevent *restrict evp, timer_t *restrict
> > ksev.sigev_tid = td->tid;
> > if (syscall(SYS_timer_create, clk, &ksev, &timerid) < 0) {
> > timerid = -1;
> > - td->cancel = 1;
> > }
> > td->timer_id = timerid;
> > sem_post(&args.sem1);
> > --
> > 2.55.0.979.g7e5102b832-goog
> >
>
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
