* [PATCH] time: Fix bug in timer_create @ 2019-09-24 11:16 changdiankang 2019-09-25 17:19 ` Rich Felker 0 siblings, 1 reply; 4+ messages in thread From: changdiankang @ 2019-09-24 11:16 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 1158 bytes --] time: Fix bug in timer_create When create a SIGEV_THREAD timer, in helper thread "start", "id" is assigned with self->timer_id immediately after pthread_create. But the real timer_id is assigned to self->timer_id after SYS_timer_create. So "id" in "start" always be 0, and timer_delete will always fail. Put assignement of "id" after pthread_barrier_wait can fix this bug. Signed-off-by: Chang Diankang <changdiankang@huawei.com<mailto:changdiankang@huawei.com>> diff --git a/src/time/timer_create.c b/src/time/timer_create.c index c5e40a1..4172b9e 100644 --- a/src/time/timer_create.c +++ b/src/time/timer_create.c @@ -48,13 +48,14 @@ static void *start(void *arg) { pthread_t self = __pthread_self(); struct start_args *args = arg; - int id = self->timer_id; + int id; jmp_buf jb; void (*notify)(union sigval) = args->sev->sigev_notify_function; union sigval val = args->sev->sigev_value; pthread_barrier_wait(&args->b); + id = self->timer_id; for (;;) { siginfo_t si; while (sigwaitinfo(SIGTIMER_SET, &si) < 0); [-- Attachment #2: Type: text/html, Size: 4729 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] time: Fix bug in timer_create 2019-09-24 11:16 [PATCH] time: Fix bug in timer_create changdiankang @ 2019-09-25 17:19 ` Rich Felker 2019-09-25 17:34 ` Rich Felker 0 siblings, 1 reply; 4+ messages in thread From: Rich Felker @ 2019-09-25 17:19 UTC (permalink / raw) To: musl; +Cc: changdiankang On Tue, Sep 24, 2019 at 11:16:41AM +0000, changdiankang wrote: > time: Fix bug in timer_create > > When create a SIGEV_THREAD timer, in helper thread "start", > "id" is assigned with self->timer_id immediately after pthread_create. > But the real timer_id is assigned to self->timer_id after SYS_timer_create. > So "id" in "start" always be 0, and timer_delete will always fail. > Put assignement of "id" after pthread_barrier_wait can fix this bug. > > Signed-off-by: Chang Diankang <changdiankang@huawei.com<mailto:changdiankang@huawei.com>> > > diff --git a/src/time/timer_create.c b/src/time/timer_create.c > index c5e40a1..4172b9e 100644 > --- a/src/time/timer_create.c > +++ b/src/time/timer_create.c > @@ -48,13 +48,14 @@ static void *start(void *arg) > { > pthread_t self = __pthread_self(); > struct start_args *args = arg; > - int id = self->timer_id; > + int id; > jmp_buf jb; > > void (*notify)(union sigval) = args->sev->sigev_notify_function; > union sigval val = args->sev->sigev_value; > > pthread_barrier_wait(&args->b); > + id = self->timer_id; > for (;;) { > siginfo_t si; > while (sigwaitinfo(SIGTIMER_SET, &si) < 0); > Thanks! In the future, please send patches as attachments rather than inline since it looks like your mailer software is corrupting the patch (converting tabs to spaces, maybe other problems too). I can apply it manually though. Rich ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] time: Fix bug in timer_create 2019-09-25 17:19 ` Rich Felker @ 2019-09-25 17:34 ` Rich Felker 2019-09-26 3:41 ` Rich Felker 0 siblings, 1 reply; 4+ messages in thread From: Rich Felker @ 2019-09-25 17:34 UTC (permalink / raw) To: musl; +Cc: changdiankang On Wed, Sep 25, 2019 at 01:19:05PM -0400, Rich Felker wrote: > On Tue, Sep 24, 2019 at 11:16:41AM +0000, changdiankang wrote: > > time: Fix bug in timer_create > > > > When create a SIGEV_THREAD timer, in helper thread "start", > > "id" is assigned with self->timer_id immediately after pthread_create. > > But the real timer_id is assigned to self->timer_id after SYS_timer_create. > > So "id" in "start" always be 0, and timer_delete will always fail. > > Put assignement of "id" after pthread_barrier_wait can fix this bug. > > > > Signed-off-by: Chang Diankang <changdiankang@huawei.com<mailto:changdiankang@huawei.com>> > > > > diff --git a/src/time/timer_create.c b/src/time/timer_create.c > > index c5e40a1..4172b9e 100644 > > --- a/src/time/timer_create.c > > +++ b/src/time/timer_create.c > > @@ -48,13 +48,14 @@ static void *start(void *arg) > > { > > pthread_t self = __pthread_self(); > > struct start_args *args = arg; > > - int id = self->timer_id; > > + int id; > > jmp_buf jb; > > > > void (*notify)(union sigval) = args->sev->sigev_notify_function; > > union sigval val = args->sev->sigev_value; > > > > pthread_barrier_wait(&args->b); > > + id = self->timer_id; > > for (;;) { > > siginfo_t si; > > while (sigwaitinfo(SIGTIMER_SET, &si) < 0); > > > > Thanks! In the future, please send patches as attachments rather than > inline since it looks like your mailer software is corrupting the > patch (converting tabs to spaces, maybe other problems too). I can > apply it manually though. Actually, I don't think this patch is a complete fix. After the barrier wait passes, the caller can immediately call timer_delete, which sets the sign bit of ->timer_id, possibly before the start function reads the initial value of ->timer_id and saves it. I'll extend this to a full fix. Just masking off the bit would be a hack that would work, but would still leave the formal data race, so I think it would make sense to just drop use of the barrier and instead use a semaphore that both sides have to post (timer thread does wait->post, calling thread does post->wait). That would be nice in terms of getting rid of linking dependency on pthread barriers too. Rich ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] time: Fix bug in timer_create 2019-09-25 17:34 ` Rich Felker @ 2019-09-26 3:41 ` Rich Felker 0 siblings, 0 replies; 4+ messages in thread From: Rich Felker @ 2019-09-26 3:41 UTC (permalink / raw) To: musl On Wed, Sep 25, 2019 at 01:34:24PM -0400, Rich Felker wrote: > On Wed, Sep 25, 2019 at 01:19:05PM -0400, Rich Felker wrote: > > On Tue, Sep 24, 2019 at 11:16:41AM +0000, changdiankang wrote: > > > time: Fix bug in timer_create > > > > > > When create a SIGEV_THREAD timer, in helper thread "start", > > > "id" is assigned with self->timer_id immediately after pthread_create. > > > But the real timer_id is assigned to self->timer_id after SYS_timer_create. > > > So "id" in "start" always be 0, and timer_delete will always fail. > > > Put assignement of "id" after pthread_barrier_wait can fix this bug. > > > > > > Signed-off-by: Chang Diankang <changdiankang@huawei.com<mailto:changdiankang@huawei.com>> > > > > > > diff --git a/src/time/timer_create.c b/src/time/timer_create.c > > > index c5e40a1..4172b9e 100644 > > > --- a/src/time/timer_create.c > > > +++ b/src/time/timer_create.c > > > @@ -48,13 +48,14 @@ static void *start(void *arg) > > > { > > > pthread_t self = __pthread_self(); > > > struct start_args *args = arg; > > > - int id = self->timer_id; > > > + int id; > > > jmp_buf jb; > > > > > > void (*notify)(union sigval) = args->sev->sigev_notify_function; > > > union sigval val = args->sev->sigev_value; > > > > > > pthread_barrier_wait(&args->b); > > > + id = self->timer_id; > > > for (;;) { > > > siginfo_t si; > > > while (sigwaitinfo(SIGTIMER_SET, &si) < 0); > > > > > > > Thanks! In the future, please send patches as attachments rather than > > inline since it looks like your mailer software is corrupting the > > patch (converting tabs to spaces, maybe other problems too). I can > > apply it manually though. > > Actually, I don't think this patch is a complete fix. After the > barrier wait passes, the caller can immediately call timer_delete, > which sets the sign bit of ->timer_id, possibly before the start > function reads the initial value of ->timer_id and saves it. > > I'll extend this to a full fix. Just masking off the bit would be a > hack that would work, but would still leave the formal data race, so I > think it would make sense to just drop use of the barrier and instead > use a semaphore that both sides have to post (timer thread does > wait->post, calling thread does post->wait). That would be nice in > terms of getting rid of linking dependency on pthread barriers too. It turns out it was easiest to just move the load of timer_id to the deletion code path. All of this will go away at some point anyway when we stop using kernel timer objects for SIGEV_THREAD timers and instead just use clock_nanosleep and manual overrun counting. Rich ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-26 3:41 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-24 11:16 [PATCH] time: Fix bug in timer_create changdiankang 2019-09-25 17:19 ` Rich Felker 2019-09-25 17:34 ` Rich Felker 2019-09-26 3:41 ` Rich Felker
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).