Re: [PATCH 5/5] winebus.sys: Use irp IoStatus.Status consistently.

[Zebediah Figura (she/her)]

On 8/12/21 2:47 AM, Rémi Bernon wrote:
> case IOCTL_HID_READ_REPORT:
>           {
>               TRACE_(hid_report)("IOCTL_HID_READ_REPORT\n");
> -            status = ext->vtbl->begin_report_processing(device);
> -            if (status != STATUS_SUCCESS)
> -            {
> -                irp->IoStatus.Status = status;
> -                break;
> -            }
> +            irp->IoStatus.Status = ext->vtbl->begin_report_processing(device);
> +            if (irp->IoStatus.Status != STATUS_SUCCESS) break;
>               if (!ext->last_report_read)
>               {
> -                irp->IoStatus.Status = status = deliver_last_report(ext,
> +                irp->IoStatus.Status = deliver_last_report(ext,
>                       buffer_len, irp->UserBuffer, &irp->IoStatus.Information);
>                   ext->last_report_read = TRUE;
>               }
>               else
>               {
>                   InsertTailList(&ext->irp_queue, &irp->Tail.Overlay.ListEntry);
> -                status = STATUS_PENDING;
> +                irp->IoStatus.Status = STATUS_PENDING;
>               }
>               break;
>           }

If you return STATUS_PENDING and queue the IRP here...
> @@ -990,9 +982,8 @@ static NTSTATUS WINAPI hid_internal_dispatch(DEVICE_OBJECT *device, IRP *irp)
>
> LeaveCriticalSection(&ext->cs);

...and release the CS here, then the IRP can be completed before you 
access it...
> -    if (status != STATUS_PENDING)
> -        IoCompleteRequest(irp, IO_NO_INCREMENT);
> -
> +    status = irp->IoStatus.Status;

...here.

This is kind of the reason that I've gone for the pattern

     switch (ioctl)
     {
         case X:
             status = STATUS_SUCCESS;
             break;

         case Y:
             status = STATUS_PENDING;
             IoMarkIrpPending(irp);
             queue_irp(irp);
             break;
     }

     if (status != STATUS_PENDING)
     {
         irp->IoStatus.Status = status;
         IoCompleteRequest(irp);
     }
     return status;

in other ntoskrnl code; it's almost the same amount of code and less 
easy to get wrong.
> +    if (status != STATUS_PENDING) IoCompleteRequest(irp, IO_NO_INCREMENT);
>       return status;
>   }