Corinna Vinschen wrote:
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.
Oops. Before this stmt was added I was relying on sigtimedwait() to validate.
But doing math on the values here does demand validation here. I will fix.
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.
OK, will change. I was using msecs because the code only cares whether it's
zero or nonzero and I couldn't see wasting 8-byte values on that. But
GetTickCount64() motivates for the longer variables... So will be fixed.
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)
I used these in my signal.cc updates but somehow forgot this AIO stuff is now
inside Cygwin DLL so can use the same defines. Will fix.
+ [...]
+ 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.
My XXX concern was whether an app could get stuck here and not be abortable.
But I take your comments to mean a non-maskable signal will break out of the
sigtimedwait(), so e.g. Ctrl-C, or SIGTERM from outside, could interrupt the app.
+ 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.
OK, will fix.
+ /* Adjust timeout to account for time just waited */
+ msecs -= (time1 - time0);
+ if (msecs < 0)
This can't happen then.
Right.
+ 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.
I'll revisit this issue. This internal aiosuspend() routine is called from both
aio_suspend() and lio_listio(). Those two functions have conflicting
protections on args passed to them and I had some trouble coming up with
something that would compile cleanly. As I say, I will look at this again.
..mark
