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.
Yes, my diff attempted to fix kern/ in totality but guenther@ objected
with:
> Could you use the timespecfix() function in the kernel? I believe the
> same should be done for futex(2).
No. timespecfix() is only appropriate for *relative* timespec values and
not *absolute* timespec value and __thrsleep(2) takes an absolute value.
All the tests of tv_sec in timespecfix() are inappropriate for absolute
timespec values.
He did ok the futex_wait bits but wanted further testing:
Per my other email, the __thrsleep(9) and dovutimens(9) changes are
incorrect. The sys___thrsigdivert(9) and futex_wait(9) changes seem sane,
but have to be tested.
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.
> > 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);
> > 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 :)
