[PATCH v2 1/3] user32: Implement SendInput size parameter check.

[Rémi Bernon]

On 3/15/21 7:26 PM, Rémi Bernon wrote:
> On 3/9/21 7:39 PM, Rémi Bernon wrote:
>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>> ---
>>
>> v2: Fix WoW64 and 32bit test results, a bit or rewrite of PATCH 3 to
>>      use of the INPUT_WINE flag as implicit way to inform of injected
>>      messages.
>>
>> Supersedes: 201308-201310
>>
>>   dlls/user32/input.c       | 18 +++++++++++
>>   dlls/user32/tests/input.c | 63 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 81 insertions(+)
>>
>> diff --git a/dlls/user32/input.c b/dlls/user32/input.c
>> index e06f8b4413e..e9a74f177b9 100644
>> --- a/dlls/user32/input.c
>> +++ b/dlls/user32/input.c
>> @@ -182,6 +182,24 @@ UINT WINAPI SendInput( UINT count, LPINPUT 
>> inputs, int size )
>>       UINT i;
>>       NTSTATUS status;
>> +    if (size != sizeof(INPUT))
>> +    {
>> +        SetLastError( ERROR_INVALID_PARAMETER );
>> +        return 0;
>> +    }
>> +
>> +    if (!count)
>> +    {
>> +        SetLastError( ERROR_INVALID_PARAMETER );
>> +        return 0;
>> +    }
>> +
>> +    if (!inputs)
>> +    {
>> +        SetLastError( ERROR_NOACCESS );
>> +        return 0;
>> +    }
>> +
>>       for (i = 0; i < count; i++)
>>       {
>>           if (inputs[i].type == INPUT_MOUSE)
>> diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c
>> index 9d75daa0bd5..7e8829ce375 100644
>> --- a/dlls/user32/tests/input.c
>> +++ b/dlls/user32/tests/input.c
>> @@ -4118,6 +4118,68 @@ static void 
>> test_UnregisterDeviceNotification(void)
>>       ok(ret == FALSE, "Unregistering NULL Device Notification 
>> returned: %d\n", ret);
>>   }
>> +static void test_SendInput(void)
>> +{
>> +    INPUT input[16];
>> +    UINT res, i;
>> +    HWND hwnd;
>> +
>> +    hwnd = CreateWindowW( L"static", L"test", WS_OVERLAPPED, 0, 0, 
>> 100, 100, 0, 0, 0, 0 );
>> +    ok(hwnd != 0, "CreateWindowW failed\n");
>> +
>> +    ShowWindow( hwnd, SW_SHOWNORMAL );
>> +    UpdateWindow( hwnd );
>> +    SetForegroundWindow( hwnd );
>> +    SetFocus( hwnd );
>> +    empty_message_queue();
>> +
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(0, NULL, 0);
>> +    ok(res == 0 && GetLastError() == ERROR_INVALID_PARAMETER, 
>> "SendInput returned %u, error %#x\n", res, GetLastError());
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(1, NULL, 0);
>> +    ok(res == 0 && GetLastError() == ERROR_INVALID_PARAMETER, 
>> "SendInput returned %u, error %#x\n", res, GetLastError());
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(1, NULL, sizeof(*input));
>> +    ok(res == 0 && (GetLastError() == ERROR_NOACCESS || 
>> GetLastError() == ERROR_INVALID_PARAMETER), "SendInput returned %u, 
>> error %#x\n", res, GetLastError());
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(0, input, sizeof(*input));
>> +    ok(res == 0 && GetLastError() == ERROR_INVALID_PARAMETER, 
>> "SendInput returned %u, error %#x\n", res, GetLastError());
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(0, NULL, sizeof(*input));
>> +    ok(res == 0 && GetLastError() == ERROR_INVALID_PARAMETER, 
>> "SendInput returned %u, error %#x\n", res, GetLastError());
>> +
>> +    memset(input, 0, sizeof(input));
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(1, input, sizeof(*input));
>> +    ok(res == 1 && GetLastError() == 0xdeadbeef, "SendInput returned 
>> %u, error %#x\n", res, GetLastError());
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(16, input, sizeof(*input));
>> +    ok(res == 16 && GetLastError() == 0xdeadbeef, "SendInput returned 
>> %u, error %#x\n", res, GetLastError());
>> +
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(1, input, 0);
>> +    ok(res == 0 && GetLastError() == ERROR_INVALID_PARAMETER, 
>> "SendInput returned %u, error %#x\n", res, GetLastError());
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(1, input, sizeof(*input) + 1);
>> +    ok(res == 0 && GetLastError() == ERROR_INVALID_PARAMETER, 
>> "SendInput returned %u, error %#x\n", res, GetLastError());
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(1, input, sizeof(*input) - 1);
>> +    ok(res == 0 && GetLastError() == ERROR_INVALID_PARAMETER, 
>> "SendInput returned %u, error %#x\n", res, GetLastError());
>> +
>> +    for (i = 0; i < ARRAY_SIZE(input); ++i) input[i].type = 
>> INPUT_KEYBOARD;
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(16, input, offsetof(INPUT, ki) + 
>> sizeof(KEYBDINPUT));
>> +    ok(res == 0 && GetLastError() == ERROR_INVALID_PARAMETER, 
>> "SendInput returned %u, error %#x\n", res, GetLastError());
>> +    SetLastError(0xdeadbeef);
>> +    res = SendInput(16, input, sizeof(*input));
>> +    ok(res == 16 && GetLastError() == 0xdeadbeef, "SendInput returned 
>> %u, error %#x\n", res, GetLastError());
>> +    empty_message_queue();
>> +
>> +    trace("done\n");
>> +    DestroyWindow(hwnd);
>> +}
>> +
>>   START_TEST(input)
>>   {
>>       char **argv;
>> @@ -4140,6 +4202,7 @@ START_TEST(input)
>>           return;
>>       }
>> +    test_SendInput();
>>       test_Input_blackbox();
>>       test_Input_whitebox();
>>       test_Input_unicode();
>>
> 
> So, should I keep the custom __wine_send_input entry point and extend it 
> to accept a dynamically sized structure instead?

I'm not completely sure what is wrong here, but it I understand it's not 
liked very much. I can see that patch 3 has an issue where we probably 
shouldn't call update_mouse_coords for internal messages.

Anyway it should be possible to send HID reports without this and, I 
think, only using the current and public Win32 structures. My current 
plan is the following, and although I haven't implemented it fully yet I 
think it could work:

- Use __wine_send_input to send all HID input, using an INPUT structure, 
with INPUT_HARDWARE type and WM_INPUT / WM_INPUT_DEVICE_CHANGE uMsg.

   - For WM_INPUT message, it'd be difficult to pass a HRAWINPUT handle 
as wParamL/H as afaics there's no way to convert a RAWINPUT struct or 
pointer to a 32bit handle.

   - For WM_INPUT_DEVICE_CHANGE, we could use the lparam as the device 
index, as it's supposed to be, but we would then still miss the usage 
page / usage field.

- So, in both cases, this INPUT will instead be implicitly followed in 
memory with a RAWINPUT structure, that send_hardware_message will expect 
for these internal / INPUT_HARDWARE / uMsg combination, and the INPUT 
wParamL/H would be used as a size information (for instance).

- The RAWINPUT structure will be dynamically sized and hold the HID 
report as well. In the WM_INPUT_DEVICE_CHANGE case, when a device is 
added, the report will be the device descriptor report, which will 
provide the device usage (page) bytes.

- Because other WM_INPUT reports won't have the device usage (page) 
bytes anymore, we will instead have to keep the list of known rawinput 
devices on the wineserver side, with their associated rawinput device 
index, and match the hDevice field of the RAWINPUT structures to figure 
their usage (page) and find interested processes.

Is that a better plan?

In all cases, I think the first two patches here are still useful to 
check that we can use INPUT_HARDWARE type for internal use (and disallow 
it for external use).

Cheers,
-- 
Rémi Bernon <rbernon at codeweavers.com>