On Wed, May 09, 2018 at 11:54:15AM +0300, Paul Irofti wrote:
> On Wed, May 09, 2018 at 10:37:14AM +0200, Martin Pieuchot wrote:
> > On 08/05/18(Tue) 14:57, Scott Cheloha wrote:
> > > Hi,
> > >
> > > futex(2) doesn't do any range checking for timeout for FUTEX_WAIT,
> > > though recent Linux does so. I assume we'd also want to validate
> > > timeout before waiting.
> >
> > Could you use timespecfix() for that? Look how pselect(2) or ppoll(2)
> > do it. Btw pirofti@ had a similar diff.
>
> [...]
>
> I was keeping that change for after the new semaphore implementation goes
> in so I can more easily separate fallout sources if any.
>
> So I will commit this change afterwards if you guys don't mind or don't
> see this crashing actual applications in the wild.
That works for me.
One question below.
> > > Index: sys/kern/sys_futex.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/kern/sys_futex.c,v
> > > retrieving revision 1.7
> > > diff -u -p -r1.7 sys_futex.c
> > > --- sys/kern/sys_futex.c 24 Apr 2018 17:19:35 -0000 1.7
> > > +++ sys/kern/sys_futex.c 8 May 2018 19:49:27 -0000
> > > @@ -214,6 +214,8 @@ futex_wait(uint32_t *uaddr, uint32_t val
> > > if (KTRPOINT(p, KTR_STRUCT))
> > > ktrabstimespec(p, &ts);
> > > #endif
> > > + if (ts.tv_sec < 0 || ts.tv_nsec < 0 || ts.tv_nsec >= 1000000000)
> > > + return EINVAL;
> > > to_ticks = (uint64_t)hz * ts.tv_sec +
> > > (ts.tv_nsec + tick * 1000 - 1) / (tick * 1000) + 1;
> > > if (to_ticks > INT_MAX)
>
> This is better:
>
>
> Index: sys/kern/sys_futex.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sys_futex.c,v
> retrieving revision 1.6
> diff -u -p -u -p -r1.6 sys_futex.c
> --- sys/kern/sys_futex.c 8 Jan 2018 22:33:13 -0000 1.6
> +++ sys/kern/sys_futex.c 4 Apr 2018 11:53:59 -0000
> @@ -210,6 +210,8 @@ futex_wait(uint32_t *uaddr, uint32_t val
>
> if ((error = copyin(timeout, &ts, sizeof(ts))))
> return error;
> + if ((error = timespecfix(&ts)))
> + return error;
> #ifdef KTRACE
> if (KTRPOINT(p, KTR_STRUCT))
> ktrabstimespec(p, &ts);
Is it better to filter for non-normal input before the trace? My thought
was that it'd be useful to trace the input before returning EINVAL, but
maybe I'm missing a drawback there.
-Scott
> > > Index: regress/sys/kern/futex/futex.c
> > > ===================================================================
> > > RCS file: /cvs/src/regress/sys/kern/futex/futex.c,v
> > > retrieving revision 1.2
> > > diff -u -p -r1.2 futex.c
> > > --- regress/sys/kern/futex/futex.c 30 Apr 2017 10:11:03 -0000
> > > 1.2
> > > +++ regress/sys/kern/futex/futex.c 8 May 2018 19:49:27 -0000
> > > @@ -37,7 +37,7 @@ int
> > > main(int argc, char *argv[])
> > > {
> > > struct sigaction sa;
> > > - struct timespec abs = { 0, 5000 };
> > > + struct timespec abs = { 0, 5000 }, bogus;
> > > pthread_t thread;
> > >
> > > /* Invalid operation */
> > > @@ -48,6 +48,17 @@ main(int argc, char *argv[])
> > >
> > > /* If (lock != 1) return EAGAIN */
> > > assert(futex_twait(&lock, 1, 0, NULL) == EAGAIN);
> > > +
> > > + /* Invalid timeout */
> > > + bogus.tv_sec = -1;
> > > + bogus.tv_nsec = 0;
> > > + assert(futex(&lock, FUTEX_WAIT, lock, &bogus, NULL) == EINVAL);
> > > + bogus.tv_sec = 0;
> > > + bogus.tv_nsec = -1;
> > > + assert(futex(&lock, FUTEX_WAIT, lock, &bogus, NULL) == EINVAL);
> > > + bogus.tv_sec = 0;
> > > + bogus.tv_nsec = 1000000001;
> > > + assert(futex(&lock, FUTEX_WAIT, lock, &bogus, NULL) == EINVAL);
> > >
> > > /* Deadlock for 5000ns */
> > > assert(futex_twait(&lock, 0, CLOCK_REALTIME, &abs) == ETIMEDOUT);
>
> I like the regress test :)
