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

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

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

Attachment: signature.asc
Description: PGP signature