Re: [PATCH v3 1/3] POSIX Asynchronous I/O support: aio files

[Mark Geisert <mark at maxrnd dot com>]

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