Re: serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK

[Alexandre Julliard]

Wolfgang Walter <wolfgang.walter(a)stwm.de> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org

serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK

[Wolfgang Walter]

Am Dienstag, 19. April 2011 schrieb Alexandre Julliard:
> Wolfgang Walter <wolfgang.walter(a)stwm.de> 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,
-- 
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts

serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK

[Alexandre Julliard]

Wolfgang Walter <wolfgang.walter(a)stwm.de> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org

serial: Fix race for IOCTL_SERIAL_WAIT_ON_MASK

[Wolfgang Walter]

Am Dienstag, 19. April 2011 schrieb Alexandre Julliard:
> Wolfgang Walter <wolfgang.walter(a)stwm.de> 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,
-- 
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts