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 <bernhardloos(a)googlemail.com> 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
julliard(a)winehq.org

[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 <julliard(a)winehq.org> wrote:
> Bernhard Loos <bernhardloos(a)googlemail.com> 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
> julliard(a)winehq.org

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 <bernhardloos(a)googlemail.com> writes:
> On Thu, Jul 28, 2011 at 7:46 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
> > Bernhard Loos <bernhardloos(a)googlemail.com> 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
> > julliard(a)winehq.org
>
> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org