Re: serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK

[Alexandre Julliard]

Wolfgang Walter  writes:
> set status field of piosb to STATUS_PENDING before calling wait_on(). If wait_on
> returns with STATUS_PENDING don't touch piosb.
>
> Reason: if wait_on returns with STATUS_PENDING it started a thread which
> itself modifies the status field.

In general the iosb is not modified until the operation completes.

serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK

[Wolfgang Walter]

Am Dienstag, 19. April 2011 schrieb Alexandre Julliard:
> Wolfgang Walter  writes:
> > set status field of piosb to STATUS_PENDING before calling wait_on(). If
> > wait_on returns with STATUS_PENDING don't touch piosb.
> >
> > Reason: if wait_on returns with STATUS_PENDING it started a thread which
> > itself modifies the status field.
>
> In general the iosb is not modified until the operation completes.

Sorry, don't understand what you mean.

Without that patch an application of us does not work correctly (it hangs 
regularly).

This is the unpatched code:
1036 static inline NTSTATUS io_control(HANDLE hDevice,
....
1256     case IOCTL_SERIAL_WAIT_ON_MASK:
1257         if (lpOutBuffer && nOutBufferSize == sizeof(DWORD))
1258         {
1259             if (!(status = wait_on(hDevice, fd, hEvent, piosb, 
lpOutBuffer)))
1260                 sz = sizeof(DWORD);
1261         }
1262         else
1263             status = STATUS_INVALID_PARAMETER;
1264         break;
...

1272     }
1273     if (needs_close) close( fd );
1274  error:
1275     piosb->u.Status = status;
1276     piosb->Information = sz;
1277     if (hEvent && status != STATUS_PENDING) NtSetEvent(hEvent, NULL);
1278     return status;
1279 }

So this is my theorie:

* wait_on() is called.

* wait_on() calls RtlQueueWorkItem(wait_for_event, commio, 0 /* FIXME */)
  and returns with STATUS_PENDING

* wait_for_event() finishes and sets iosb before io_control() continues

* io_control sets iosb to STATUS_PENDING

=> hang

I think it is not correct to manipulate iosb once 
RtlQueueWorkItem(wait_for_event, commio, 0 /* FIXME */) has been called.

Regards,

serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK

[Alexandre Julliard]

Wolfgang Walter  writes:
> So this is my theorie:
> -
>
> wait_on() is called.
> -
>
> wait_on() calls RtlQueueWorkItem(wait_for_event, commio, 0 /* FIXME */)
> and returns with STATUS_PENDING
> -
>
> wait_for_event() finishes and sets iosb before io_control() continues
> -
>
> io_control sets iosb to STATUS_PENDING

The last one is a bug. iosb should only be set once we have a
result. Check how the other async I/O functions do it.

serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK

[Wolfgang Walter]

Am Dienstag, 19. April 2011 schrieb Alexandre Julliard:
> Wolfgang Walter  writes:
> > So this is my theorie:
> > -
> >
> > wait_on() is called.
> > -
> >
> > wait_on() calls RtlQueueWorkItem(wait_for_event, commio, 0 /* FIXME */)
> > and returns with STATUS_PENDING
> > -
> >
> > wait_for_event() finishes and sets iosb before io_control() continues
> > -
> >
> > io_control sets iosb to STATUS_PENDING
>
> The last one is a bug. iosb should only be set once we have a
> result. Check how the other async I/O functions do it.

It is done similar elsewhere when getting asynchron, i.e.

in dlls/ws2_32/socket.c: WS2_ConnectEx():

or dlls/kernel32/file.c: WriteFile()

* set iosb to STATUS_PENDING,
* call function
* if it returns with STATUS_PENDING don't touch iosb.

Regards,