Re: [PATCH 2/8] ntdll: rework the handling of server ioctls a little bit to avoid a crash with later patches

[Alexandre Julliard]

Bernhard Loos  writes:
> @@ -1273,10 +1282,16 @@ static NTSTATUS server_ioctl_file( HANDLE handle, HANDLE event,
>  if (wait_handle)
>  {
> -
>    NtWaitForSingleObject( wait_handle, (options & FILE_SYNCHRONOUS_IO_ALERT), NULL );
> -
>    status = io->u.Status;
> -
>    status = NtWaitForSingleObject( wait_handle, (options & FILE_SYNCHRONOUS_IO_ALERT), NULL );
> -
>    if (status == STATUS_USER_APC)
> -
>    {
> -
>        async->interrupted = TRUE;
> -
>        status = STATUS_CANCELLED; /* not really, the ioctl completes and event and the handle
> -
>                                      itself get signaled */
> -
>    }

This looks wrong. You can't claim it was cancelled if it's still
running.

[PATCH 2/8] ntdll: rework the handling of server ioctls a little bit to avoid a crash with later patches

[Bernhard Loos]

On Thu, Jul 28, 2011 at 7:46 PM, Alexandre Julliard  wrote:
> Bernhard Loos  writes:
> > @@ -1273,10 +1282,16 @@ static NTSTATUS server_ioctl_file( HANDLE handle, HANDLE event,
> >
> > if (wait_handle)
> >      {
> > - NtWaitForSingleObject( wait_handle, (options & FILE_SYNCHRONOUS_IO_ALERT), NULL );
> > - status = io->u.Status;
> > - status = NtWaitForSingleObject( wait_handle, (options & FILE_SYNCHRONOUS_IO_ALERT), NULL );
> > - if (status == STATUS_USER_APC)
> > - {
> > - async->interrupted = TRUE;
> > - status = STATUS_CANCELLED; /* not really, the ioctl completes and event and the handle
> > - itself get signaled */
> > - }
>
> This looks wrong. You can't claim it was cancelled if it's still
> running.
>
> --
> Alexandre Julliard
> [e-mail]

I'm not really sure, what to do at this point. I can't exactly return
ERROR_SUCCESS because the operation is not completed yet. At best, I
can reenter the wait, but this will break in places, where the caller
depends on DeviceIoControl returning in case of a scheduled user apc.

Bernhard Loos

[PATCH 2/8] ntdll: rework the handling of server ioctls a little bit to avoid a crash with later patches

[Alexandre Julliard]

Bernhard Loos  writes:
> On Thu, Jul 28, 2011 at 7:46 PM, Alexandre Julliard  wrote:
> > Bernhard Loos  writes:
> > > @@ -1273,10 +1282,16 @@ static NTSTATUS server_ioctl_file( HANDLE handle, HANDLE event,
> > >
> > > if (wait_handle)
> > >      {
> > > - NtWaitForSingleObject( wait_handle, (options & FILE_SYNCHRONOUS_IO_ALERT), NULL );
> > > - status = io->u.Status;
> > > - status = NtWaitForSingleObject( wait_handle, (options & FILE_SYNCHRONOUS_IO_ALERT), NULL );
> > > - if (status == STATUS_USER_APC)
> > > - {
> > > - async->interrupted = TRUE;
> > > - status = STATUS_CANCELLED; /* not really, the ioctl completes and event and the handle
> > > - itself get signaled */
> > > - }
> >
> > This looks wrong. You can't claim it was cancelled if it's still
> > running.
> >
> > --
> > Alexandre Julliard
> > [e-mail]
>
> I'm not really sure, what to do at this point. I can't exactly return
> ERROR_SUCCESS because the operation is not completed yet. At best, I
> can reenter the wait, but this will break in places, where the caller
> depends on DeviceIoControl returning in case of a scheduled user apc.

If the operation is supposed to be cancelled, then it should be
cancelled properly. This probably needs to be done server-side.