[PATCH v3 1/3] POSIX Asynchronous I/O support: aio files
Wed Jul 18 05:55:00 GMT 2018
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.
>> + 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.
More information about the Cygwin-patches