[PATCH 1/7] ntoskrnl.exe: Send IRP_MN_SURPRISE_REMOVAL to the device stack first.
Instead of sending both IRP_MN_SURPRISE_REMOVAL and IRP_MN_REMOVE_DEVICE to all children first. They may have pending IRPs sent to their parent PDO driver, which need to be cancelled before IRP_MN_REMOVE_DEVICE can complete. This is the case for hidclass.sys and its thread calling the lower driver for instance. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/pnp.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c index e425712d738..d8eb62bc17a 100644 --- a/dlls/ntoskrnl.exe/pnp.c +++ b/dlls/ntoskrnl.exe/pnp.c @@ -350,21 +350,20 @@ static void enumerate_new_device( DEVICE_OBJECT *device, HDEVINFO set ) start_device( device, set, &sp_device ); } -static void remove_device( DEVICE_OBJECT *device ) +static void send_remove_device_irp( DEVICE_OBJECT *device, UCHAR code ) { struct wine_device *wine_device = CONTAINING_RECORD(device, struct wine_device, device_obj); - TRACE("Removing device %p.\n", device); + TRACE( "Removing device %p, code %x.\n", device, code ); if (wine_device->children) { ULONG i; for (i = 0; i < wine_device->children->Count; ++i) - remove_device( wine_device->children->Objects[i] ); + send_remove_device_irp( wine_device->children->Objects[i], code ); } - send_pnp_irp( device, IRP_MN_SURPRISE_REMOVAL ); - send_pnp_irp( device, IRP_MN_REMOVE_DEVICE ); + send_pnp_irp( device, code ); } static BOOL device_in_list( const DEVICE_RELATIONS *list, const DEVICE_OBJECT *device ) @@ -441,7 +440,8 @@ static void handle_bus_relations( DEVICE_OBJECT *parent ) if (!device_in_list( relations, child )) { TRACE("Removing device %p.\n", child); - remove_device( child ); + send_remove_device_irp( child, IRP_MN_SURPRISE_REMOVAL ); + send_remove_device_irp( child, IRP_MN_REMOVE_DEVICE ); } ObDereferenceObject( child ); } @@ -1105,7 +1105,10 @@ void pnp_manager_stop_driver( struct wine_driver *driver ) struct root_pnp_device *device, *next; LIST_FOR_EACH_ENTRY_SAFE( device, next, &driver->root_pnp_devices, struct root_pnp_device, entry ) - remove_device( device->device ); + { + send_remove_device_irp( device->device, IRP_MN_SURPRISE_REMOVAL ); + send_remove_device_irp( device->device, IRP_MN_REMOVE_DEVICE ); + } } void pnp_manager_stop(void) @@ -1179,7 +1182,8 @@ void CDECL wine_enumerate_root_devices( const WCHAR *driver_name ) { TRACE("Removing device %s.\n", debugstr_w(pnp_device->id)); - remove_device( pnp_device->device ); + send_remove_device_irp( pnp_device->device, IRP_MN_SURPRISE_REMOVAL ); + send_remove_device_irp( pnp_device->device, IRP_MN_REMOVE_DEVICE ); } list_move_head( &driver->root_pnp_devices, &new_list ); -- 2.32.0
Marking input report read requests as pending and queueing them instead of returning STATUS_NOT_IMPLEMENTED. Windows also calls IRP_MN_(QUERY|CANCEL)_REMOVE_DEVICE on device initialization and we have to implement them for the test to not timeout. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver_hid.c | 99 +++++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 3 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/driver_hid.c b/dlls/ntoskrnl.exe/tests/driver_hid.c index 049b4232753..a86e8e444c6 100644 --- a/dlls/ntoskrnl.exe/tests/driver_hid.c +++ b/dlls/ntoskrnl.exe/tests/driver_hid.c @@ -42,10 +42,64 @@ static UNICODE_STRING control_symlink; static unsigned int got_start_device; static DWORD report_id; +struct irp_queue +{ + KSPIN_LOCK lock; + LIST_ENTRY list; +}; + +static IRP *irp_queue_pop(struct irp_queue *queue) +{ + KIRQL irql; + IRP *irp; + + KeAcquireSpinLock(&queue->lock, &irql); + if (IsListEmpty(&queue->list)) irp = NULL; + else irp = CONTAINING_RECORD(RemoveHeadList(&queue->list), IRP, Tail.Overlay.ListEntry); + KeReleaseSpinLock(&queue->lock, irql); + + return irp; +} + +static void irp_queue_push(struct irp_queue *queue, IRP *irp) +{ + KIRQL irql; + + KeAcquireSpinLock(&queue->lock, &irql); + InsertTailList(&queue->list, &irp->Tail.Overlay.ListEntry); + KeReleaseSpinLock(&queue->lock, irql); +} + +static void irp_queue_clear(struct irp_queue *queue) +{ + IRP *irp; + + while ((irp = irp_queue_pop(queue))) + { + irp->IoStatus.Status = STATUS_DELETE_PENDING; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } +} + +static void irp_queue_init(struct irp_queue *queue) +{ + KeInitializeSpinLock(&queue->lock); + InitializeListHead(&queue->list); +} + +struct hid_device +{ + BOOL removed; + KSPIN_LOCK lock; + struct irp_queue irp_queue; +}; + static NTSTATUS WINAPI driver_pnp(DEVICE_OBJECT *device, IRP *irp) { IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); HID_DEVICE_EXTENSION *ext = device->DeviceExtension; + struct hid_device *impl = ext->MiniDeviceExtension; + KIRQL irql; if (winetest_debug > 1) trace("pnp %#x\n", stack->MinorFunction); @@ -53,17 +107,35 @@ static NTSTATUS WINAPI driver_pnp(DEVICE_OBJECT *device, IRP *irp) { case IRP_MN_START_DEVICE: ++got_start_device; + impl->removed = FALSE; + KeInitializeSpinLock(&impl->lock); + irp_queue_init(&impl->irp_queue); IoSetDeviceInterfaceState(&control_symlink, TRUE); irp->IoStatus.Status = STATUS_SUCCESS; break; case IRP_MN_SURPRISE_REMOVAL: case IRP_MN_QUERY_REMOVE_DEVICE: + KeAcquireSpinLock(&impl->lock, &irql); + impl->removed = TRUE; + KeReleaseSpinLock(&impl->lock, irql); + irp_queue_clear(&impl->irp_queue); + irp->IoStatus.Status = STATUS_SUCCESS; + break; + + case IRP_MN_CANCEL_REMOVE_DEVICE: + KeAcquireSpinLock(&impl->lock, &irql); + impl->removed = FALSE; + KeReleaseSpinLock(&impl->lock, irql); + irp->IoStatus.Status = STATUS_SUCCESS; + break; + case IRP_MN_STOP_DEVICE: irp->IoStatus.Status = STATUS_SUCCESS; break; case IRP_MN_REMOVE_DEVICE: + irp_queue_clear(&impl->irp_queue); IoSetDeviceInterfaceState(&control_symlink, FALSE); irp->IoStatus.Status = STATUS_SUCCESS; break; @@ -317,10 +389,14 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) static BOOL test_failed; IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp); + HID_DEVICE_EXTENSION *ext = device->DeviceExtension; + struct hid_device *impl = ext->MiniDeviceExtension; const ULONG in_size = stack->Parameters.DeviceIoControl.InputBufferLength; const ULONG out_size = stack->Parameters.DeviceIoControl.OutputBufferLength; const ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; NTSTATUS ret; + BOOL removed; + KIRQL irql; if (winetest_debug > 1) trace("ioctl %#x\n", code); @@ -328,6 +404,17 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) irp->IoStatus.Information = 0; + KeAcquireSpinLock(&impl->lock, &irql); + removed = impl->removed; + KeReleaseSpinLock(&impl->lock, irql); + + if (removed) + { + irp->IoStatus.Status = STATUS_DELETE_PENDING; + IoCompleteRequest(irp, IO_NO_INCREMENT); + return STATUS_DELETE_PENDING; + } + switch (code) { case IOCTL_HID_GET_DEVICE_DESCRIPTOR: @@ -398,7 +485,9 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) } if (out_size != expected_size) test_failed = TRUE; - ret = STATUS_NOT_IMPLEMENTED; + IoMarkIrpPending(irp); + irp_queue_push(&impl->irp_queue, irp); + ret = STATUS_PENDING; break; } @@ -494,8 +583,11 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) ret = STATUS_NOT_IMPLEMENTED; } - irp->IoStatus.Status = ret; - IoCompleteRequest(irp, IO_NO_INCREMENT); + if (ret != STATUS_PENDING) + { + irp->IoStatus.Status = ret; + IoCompleteRequest(irp, IO_NO_INCREMENT); + } return ret; } @@ -555,6 +647,7 @@ NTSTATUS WINAPI DriverEntry(DRIVER_OBJECT *driver, UNICODE_STRING *registry) { .Revision = HID_REVISION, .DriverObject = driver, + .DeviceExtensionSize = sizeof(struct hid_device), .RegistryPath = registry, }; UNICODE_STRING name_str; -- 2.32.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver_hid.c | 29 +++++++++++++++++++++++----- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 17 ++++++++++------ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/driver_hid.c b/dlls/ntoskrnl.exe/tests/driver_hid.c index a86e8e444c6..d38135f807e 100644 --- a/dlls/ntoskrnl.exe/tests/driver_hid.c +++ b/dlls/ntoskrnl.exe/tests/driver_hid.c @@ -41,6 +41,7 @@ static UNICODE_STRING control_symlink; static unsigned int got_start_device; static DWORD report_id; +static DWORD polled; struct irp_queue { @@ -485,9 +486,19 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) } if (out_size != expected_size) test_failed = TRUE; - IoMarkIrpPending(irp); - irp_queue_push(&impl->irp_queue, irp); - ret = STATUS_PENDING; + if (polled) + { + memset(irp->UserBuffer, 0xa5, expected_size); + if (report_id) ((char *)irp->UserBuffer)[0] = report_id; + irp->IoStatus.Information = 3; + ret = STATUS_SUCCESS; + } + else + { + IoMarkIrpPending(irp); + irp_queue_push(&impl->irp_queue, irp); + ret = STATUS_PENDING; + } break; } @@ -498,8 +509,9 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) ok(!in_size, "got input size %u\n", in_size); ok(out_size == sizeof(*packet), "got output size %u\n", out_size); - todo_wine_if(packet->reportId == 0x5a) - ok(packet->reportId == report_id, "got packet report id %u\n", packet->reportId); + todo_wine_if(packet->reportId == 0x5a || (polled && report_id && packet->reportId == 0)) + ok(packet->reportId == report_id, "report %d, polled %d got packet report id %u\n", + report_id, polled, packet->reportId); todo_wine_if(packet->reportBufferLen == 21 || packet->reportBufferLen == 22) ok(packet->reportBufferLen >= expected_size, "got packet buffer len %u, expected %d or more\n", packet->reportBufferLen, expected_size); @@ -669,6 +681,13 @@ NTSTATUS WINAPI DriverEntry(DRIVER_OBJECT *driver, UNICODE_STRING *registry) ok(!ret, "ZwQueryValueKey returned %#x\n", ret); memcpy(&report_id, buffer + info_size, size - info_size); + RtlInitUnicodeString(&name_str, L"PolledMode"); + size = info_size + sizeof(polled); + ret = ZwQueryValueKey(hkey, &name_str, KeyValuePartialInformation, buffer, size, &size); + ok(!ret, "ZwQueryValueKey returned %#x\n", ret); + memcpy(&polled, buffer + info_size, size - info_size); + params.DevicesArePolled = polled; + driver->DriverExtension->AddDevice = driver_add_device; driver->DriverUnload = driver_unload; driver->MajorFunction[IRP_MJ_PNP] = driver_pnp; diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 7e42c2e5cac..df80a28baf1 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -2637,7 +2637,7 @@ static void test_hidp(HANDLE file, int report_id) CloseHandle(file); } -static void test_hid_device(DWORD report_id) +static void test_hid_device(DWORD report_id, DWORD polled) { char buffer[200]; SP_DEVICE_INTERFACE_DETAIL_DATA_A *iface_detail = (void *)buffer; @@ -2652,7 +2652,7 @@ static void test_hid_device(DWORD report_id) HDEVINFO set; HANDLE file; - winetest_push_context("report %d", report_id); + winetest_push_context("report %d, polled %d", report_id, polled); set = SetupDiGetClassDevsA(&GUID_DEVINTERFACE_HID, NULL, NULL, DIGCF_DEVICEINTERFACE | DIGCF_PRESENT); ok(set != INVALID_HANDLE_VALUE, "failed to get device list, error %#x\n", GetLastError()); @@ -2696,7 +2696,7 @@ static void test_hid_device(DWORD report_id) winetest_pop_context(); } -static void test_hid_driver(struct testsign_context *ctx, DWORD report_id) +static void test_hid_driver(struct testsign_context *ctx, DWORD report_id, DWORD polled) { static const char hardware_id[] = "test_hardware_id\0"; char path[MAX_PATH], dest[MAX_PATH], *filepart; @@ -2721,6 +2721,9 @@ static void test_hid_driver(struct testsign_context *ctx, DWORD report_id) status = RegSetValueExW(hkey, L"ReportID", 0, REG_DWORD, (void *)&report_id, sizeof(report_id)); ok(!status, "RegSetValueExW returned %#x\n", status); + status = RegSetValueExW(hkey, L"PolledMode", 0, REG_DWORD, (void *)&polled, sizeof(polled)); + ok(!status, "RegSetValueExW returned %#x\n", status); + load_resource(L"driver_hid.dll", driver_filename); ret = MoveFileExW(driver_filename, L"winetest.sys", MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING); ok(ret, "failed to move file, error %u\n", GetLastError()); @@ -2768,7 +2771,7 @@ static void test_hid_driver(struct testsign_context *ctx, DWORD report_id) /* Tests. */ - test_hid_device(report_id); + test_hid_device(report_id, polled); /* Clean up. */ @@ -2899,8 +2902,10 @@ START_TEST(ntoskrnl) test_pnp_driver(&ctx); subtest("driver_hid"); - test_hid_driver(&ctx, 0); - test_hid_driver(&ctx, 1); + test_hid_driver(&ctx, 0, FALSE); + test_hid_driver(&ctx, 1, FALSE); + test_hid_driver(&ctx, 0, TRUE); + test_hid_driver(&ctx, 1, TRUE); out: testsign_cleanup(&ctx); -- 2.32.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=93719 Your paranoid android. === debiant2 (32 bit report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36387 bytes) === debiant2 (32 bit Arabic:Morocco report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36394 bytes) === debiant2 (32 bit German report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36396 bytes) === debiant2 (32 bit French report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36394 bytes) === debiant2 (32 bit Hebrew:Israel report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36105 bytes) === debiant2 (32 bit Japanese:Japan report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36394 bytes) === debiant2 (32 bit Chinese:China report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36386 bytes) === debiant2 (64 bit WoW report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36392 bytes)
On 7/6/21 11:38 AM, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=93719
Your paranoid android.
=== debiant2 (32 bit report) ===
Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36387 bytes)
=== debiant2 (32 bit Arabic:Morocco report) ===
Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36394 bytes)
=== debiant2 (32 bit German report) ===
Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36396 bytes)
=== debiant2 (32 bit French report) ===
Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36394 bytes)
=== debiant2 (32 bit Hebrew:Israel report) ===
Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36105 bytes)
=== debiant2 (32 bit Japanese:Japan report) ===
Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36394 bytes)
=== debiant2 (32 bit Chinese:China report) ===
Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36386 bytes)
=== debiant2 (64 bit WoW report) ===
Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (36392 bytes)
Well, there's a lot of todo right now but I'll fix (most of) them later. -- Rémi Bernon <rbernon(a)codeweavers.com>
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/hidclass.sys/device.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index d3f5fa90fc7..5dd4aadb899 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -703,6 +703,20 @@ NTSTATUS WINAPI pdo_write(DEVICE_OBJECT *device, IRP *irp) return STATUS_DELETE_PENDING; } + if (!irpsp->Parameters.Write.Length) + { + irp->IoStatus.Status = STATUS_INVALID_USER_BUFFER; + IoCompleteRequest( irp, IO_NO_INCREMENT ); + return irp->IoStatus.Status; + } + + if (irpsp->Parameters.Write.Length < data->caps.OutputReportByteLength) + { + irp->IoStatus.Status = STATUS_INVALID_PARAMETER; + IoCompleteRequest( irp, IO_NO_INCREMENT ); + return irp->IoStatus.Status; + } + irp->IoStatus.Information = 0; TRACE_(hid_report)("Device %p Buffer length %i Buffer %p\n", device, irpsp->Parameters.Write.Length, irp->AssociatedIrp.SystemBuffer); -- 2.32.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/driver_hid.c | 27 +++++++++++++++++ dlls/ntoskrnl.exe/tests/ntoskrnl.c | 43 +++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/dlls/ntoskrnl.exe/tests/driver_hid.c b/dlls/ntoskrnl.exe/tests/driver_hid.c index d38135f807e..1d5f7563e3f 100644 --- a/dlls/ntoskrnl.exe/tests/driver_hid.c +++ b/dlls/ntoskrnl.exe/tests/driver_hid.c @@ -502,6 +502,33 @@ static NTSTATUS WINAPI driver_internal_ioctl(DEVICE_OBJECT *device, IRP *irp) break; } + case IOCTL_HID_WRITE_REPORT: + { + HID_XFER_PACKET *packet = irp->UserBuffer; + ULONG expected_size = 2; + + todo_wine + ok(in_size == sizeof(*packet), "got input size %u\n", in_size); + todo_wine + ok(!out_size, "got output size %u\n", out_size); + ok(packet->reportBufferLen >= expected_size, "got report size %u\n", packet->reportBufferLen); + + if (report_id) + { + todo_wine_if(packet->reportBuffer[0] == 0xa5) + ok(packet->reportBuffer[0] == report_id, "got report id %x\n", packet->reportBuffer[0]); + } + else + { + todo_wine + ok(packet->reportBuffer[0] == 0xcd, "got first byte %x\n", packet->reportBuffer[0]); + } + + irp->IoStatus.Information = 3; + ret = STATUS_SUCCESS; + break; + } + case IOCTL_HID_GET_INPUT_REPORT: { HID_XFER_PACKET *packet = irp->UserBuffer; diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index df80a28baf1..9990b18aa67 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -2633,6 +2633,47 @@ static void test_hidp(HANDLE file, int report_id) todo_wine ok(value == 3, "got length %u, expected 3\n", value); + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, 0, &value, NULL); + ok(!ret, "WriteFile succeeded\n"); + ok(GetLastError() == ERROR_INVALID_USER_BUFFER, "WriteFile returned error %u\n", GetLastError()); + ok(value == 0, "WriteFile returned %x\n", value); + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, caps.OutputReportByteLength - 1, &value, NULL); + ok(!ret, "WriteFile succeeded\n"); + ok(GetLastError() == ERROR_INVALID_PARAMETER || GetLastError() == ERROR_INVALID_USER_BUFFER, + "WriteFile returned error %u\n", GetLastError()); + ok(value == 0, "WriteFile returned %x\n", value); + + memset(report, 0xcd, sizeof(report)); + report[0] = 0xa5; + SetLastError(0xdeadbeef); + ret = WriteFile(file, report, caps.OutputReportByteLength * 2, &value, NULL); + if (report_id || broken(!ret) /* w7u */) + { + todo_wine + ok(!ret, "WriteFile succeeded\n"); + todo_wine + ok(GetLastError() == ERROR_INVALID_PARAMETER, "WriteFile returned error %u\n", GetLastError()); + todo_wine + ok(value == 0, "WriteFile wrote %u\n", value); + SetLastError(0xdeadbeef); + report[0] = report_id; + ret = WriteFile(file, report, caps.OutputReportByteLength, &value, NULL); + } + + if (report_id) + { + ok(ret, "WriteFile failed, last error %u\n", GetLastError()); + ok(value == 2, "WriteFile wrote %u\n", value); + } + else + { + ok(ret, "WriteFile failed, last error %u\n", GetLastError()); + todo_wine ok(value == 3, "WriteFile wrote %u\n", value); + } + + HidD_FreePreparsedData(preparsed_data); CloseHandle(file); } @@ -2680,7 +2721,7 @@ static void test_hid_device(DWORD report_id, DWORD polled) todo_wine ok(found, "didn't find device\n"); - file = CreateFileA(iface_detail->DevicePath, FILE_READ_ACCESS, + file = CreateFileA(iface_detail->DevicePath, FILE_READ_ACCESS | FILE_WRITE_ACCESS, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); ok(file != INVALID_HANDLE_VALUE, "got error %u\n", GetLastError()); -- 2.32.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=93721 Your paranoid android. === debiant2 (32 bit report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (37900 bytes) === debiant2 (32 bit Arabic:Morocco report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (37902 bytes) === debiant2 (32 bit German report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (37904 bytes) === debiant2 (32 bit French report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (37906 bytes) === debiant2 (32 bit Hebrew:Israel report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (37902 bytes) === debiant2 (32 bit Japanese:Japan report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (37904 bytes) === debiant2 (32 bit Chinese:China report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (37892 bytes) === debiant2 (64 bit WoW report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (37898 bytes)
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/hidclass.sys/device.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index 5dd4aadb899..da1814587c7 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -597,6 +597,7 @@ NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp) { HID_XFER_PACKET *packet; BASE_DEVICE_EXTENSION *ext = device->DeviceExtension; + const WINE_HIDP_PREPARSED_DATA *data = ext->u.pdo.preparsed_data; UINT buffer_size = RingBuffer_GetBufferSize(ext->u.pdo.ring_buffer); NTSTATUS rc = STATUS_SUCCESS; IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp); @@ -615,6 +616,13 @@ NTSTATUS WINAPI pdo_read(DEVICE_OBJECT *device, IRP *irp) return STATUS_DELETE_PENDING; } + if (irpsp->Parameters.Read.Length < data->caps.InputReportByteLength) + { + irp->IoStatus.Status = STATUS_INVALID_BUFFER_SIZE; + IoCompleteRequest( irp, IO_NO_INCREMENT ); + return STATUS_INVALID_BUFFER_SIZE; + } + packet = malloc(buffer_size); ptr = PtrToUlong( irp->Tail.Overlay.OriginalFileObject->FsContext ); -- 2.32.0
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 9990b18aa67..1eab88311ab 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1676,7 +1676,7 @@ static BOOL sync_ioctl(HANDLE file, DWORD code, void *in_buf, DWORD in_len, void return ret; } -static void test_hidp(HANDLE file, int report_id) +static void test_hidp(HANDLE file, int report_id, BOOL polled) { const HIDP_CAPS expect_hidp_caps[] = { @@ -2674,6 +2674,27 @@ static void test_hidp(HANDLE file, int report_id) } + memset(report, 0xcd, sizeof(report)); + SetLastError(0xdeadbeef); + ret = ReadFile(file, report, 0, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_USER_BUFFER, "ReadFile failed, last error %u\n", GetLastError()); + ok(value == 0, "ReadFile returned %x\n", value); + SetLastError(0xdeadbeef); + ret = ReadFile(file, report, caps.InputReportByteLength - 1, &value, NULL); + ok(!ret && GetLastError() == ERROR_INVALID_USER_BUFFER, "ReadFile failed, last error %u\n", GetLastError()); + ok(value == 0, "ReadFile returned %x\n", value); + + if (polled) + { + memset(report, 0xcd, sizeof(report)); + SetLastError(0xdeadbeef); + ret = ReadFile(file, report, caps.InputReportByteLength, &value, NULL); + todo_wine ok(ret, "ReadFile failed, last error %u\n", GetLastError()); + todo_wine ok(value == (report_id ? 3 : 4), "ReadFile returned %x\n", value); + todo_wine ok(report[0] == report_id, "unexpected report data\n"); + } + + HidD_FreePreparsedData(preparsed_data); CloseHandle(file); } @@ -2725,7 +2746,7 @@ static void test_hid_device(DWORD report_id, DWORD polled) FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL); ok(file != INVALID_HANDLE_VALUE, "got error %u\n", GetLastError()); - test_hidp(file, report_id); + test_hidp(file, report_id, polled); CloseHandle(file); -- 2.32.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=93723 Your paranoid android. === debiant2 (32 bit report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (38469 bytes) === debiant2 (32 bit Arabic:Morocco report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (38467 bytes) === debiant2 (32 bit German report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (38471 bytes) === debiant2 (32 bit French report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (38471 bytes) === debiant2 (32 bit Hebrew:Israel report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (38469 bytes) === debiant2 (32 bit Japanese:Japan report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (38467 bytes) === debiant2 (32 bit Chinese:China report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (38461 bytes) === debiant2 (64 bit WoW report) === Report validation errors: ntoskrnl.exe:ntoskrnl prints too much data (38463 bytes)
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
participants (3)
-
Marvin -
Rémi Bernon -
Zebediah Figura (she/her)