Hey Mark, I just belatedly noticed a few problems in aiosuspend:
On Jul 15 01:20, Mark Geisert wrote:
> +static int
> +aiosuspend (const struct aiocb *const aiolist[],
> + int nent, const struct timespec *timeout)
> +{
> + /* Returns lowest list index of completed aios, else 'nent' if all
> completed.
> + * If none completed on entry, wait for interval specified by 'timeout'.
> + */
> + DWORD msecs = 0;
> + int res;
> + sigset_t sigmask;
> + siginfo_t si;
> + DWORD time0, time1;
^^^^^^^^^^^^^^^^^^^^^^^
see below
> + struct timespec *to = (struct timespec *) timeout;
> +
> + if (to)
> + msecs = (1000 * to->tv_sec) + ((to->tv_nsec + 999999) / 1000000);
You're not checking the content of timeout for validity, tv_sec >= 0 and
0 <= tv_nsec <= 999999999.
I'm not sure why you break the timespec down to msecs anyway. The
timespec value is ultimately used for a timer with 100ns resolution.
Why not stick to 64 bit 100ns values instead? They are used in a
lot of places.
Last but not least, please don't use numbers like 1000 or 999999 or
1000000 when converting time values. We have macros for that defined
in hires.h:
/* # of nanosecs per second. */
#define NSPERSEC (1000000000LL)
/* # of 100ns intervals per second. */
#define NS100PERSEC (10000000LL)
/* # of microsecs per second. */
#define USPERSEC (1000000LL)
/* # of millisecs per second. */
#define MSPERSEC (1000L)
> + [...]
> + time0 = GetTickCount ();
> + //XXX Is it possible have an empty signal mask ...
No, because some signals are non-maskable.
> + //XXX ... and infinite timeout?
Yes, if timeout is a NULL pointer.
> + res = sigtimedwait (&sigmask, &si, to);
> + if (res == -1)
> + return -1; /* Return with errno set by failed sigtimedwait() */
> + time1 = GetTickCount ();
This is unsafe. As a 32 bit function GetTickCount wraps around roughly
every 49 days. Use ULONGLONG GetTickCount64() instead.
> + /* Adjust timeout to account for time just waited */
> + msecs -= (time1 - time0);
> + if (msecs < 0)
This can't happen then.
> + to->tv_sec = msecs / 1000;
> + to->tv_nsec = (msecs % 1000) * 1000000;
Uh oh, you're changing caller values, despite timeout being const.
`to' shouldn't be a pointer, but a local struct timespec instead.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
signature.asc
Description: PGP signature
