Re: [PATCH 1/6] wineusb.sys: Implement URB_FUNCTION_GET_DESCRIPTOR_FROM_DEVICE.

[Thomas Faber]

I'm a bit late here, but FYI for future work:

On 2020-04-18 01:03, Zebediah Figura wrote:
> +static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device_obj, IRP *irp)
> +{
> - IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp);
> - ULONG code = stack->Parameters.DeviceIoControl.IoControlCode;
> - struct usb_device *device = device_obj->DeviceExtension;
> - NTSTATUS status = STATUS_NOT_IMPLEMENTED;
> -
> - TRACE("device_obj %p, irp %p, code %#x.\n", device_obj, irp, code);
> -
> - switch (code)
> - {
> -
>    case IOCTL_INTERNAL_USB_SUBMIT_URB:
> -
>        status = usb_submit_urb(device, irp);
> -
>        break;
> -
> -
>    default:
> -
>        FIXME("Unhandled ioctl %#x (device %#x, access %#x, function %#x, method %#x).\n",
> -
>                code, code >> 16, (code >> 14) & 3, (code >> 2) & 0xfff, code & 3);
> - }
> -
> - if (status == STATUS_PENDING)
> - {
> -
>    IoMarkIrpPending(irp);

This code pattern is almost always incorrect.
If your handler function returned STATUS_PENDING, it normally means the IRP has already been added to a queue and a worker thread may pick it up and complete it concurrently.
Because IoCompleteRequest frees the memory associated with the IRP, this can therefore result in a use after free.
The correct pattern is to call IoMarkIrpPending before enqueuing the IRP.

Thanks.
-Thomas