[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