* [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).