[Bug 51828] New: Simucube 2: All applications using raw HID access to communicate with devices, stopped tracking steering axis movement

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51828

Bug ID: 51828
           Summary: Simucube 2: All applications using raw HID access to
                    communicate with devices, stopped tracking steering
                    axis movement
           Product: Wine
           Version: 6.18
          Hardware: x86-64
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: hid
          Assignee: [e-mail]
          Reporter: [e-mail]
                CC: [e-mail]
   Regression SHA1: d40d8d968658dca4a75afc97f9e48acda0654b0f
      Distribution: ArchLinux

Created attachment 70716
  --> https://bugs.winehq.org/attachment.cgi?id=70716
0001-hidclass.sys-Fix-input-reports-with-ReportID-being-d.patch

Found out that the dropping of input packets with lengths different than
desc->InputLength, which was introduced with the regression commit is causing
this issue.
Since the Simucube 2 is a HID PID device, it uses multiple HID reports (with
defined ReportIDs), which are different lengths. For such devices
desc->InputLength specifies the biggest possible buffer length, that would fit
any of the input reports. If the most essential input report for tracking the
steering axis movements is smaller than desc->InputLength, it'll be dropped by
that check. Only the reports with the exact same size go through in non polled
mode.
In this case with Simucube 2 the biggest input report is of 61 bytes, and is
vendor defined. All the other input reports are smaller sizes, and will get
dropped.

Attached a patch which fixes the issue. It also fixes possible memory access
error with report->buffer while copying it to irp->AssociatedIrp.SystemBuffer,
due to the internal buffer for hid_report being just report->length which could
be less than or equal to desc->InputLength.
The patch is based to the current master
(aa629c4c7225166f4ee46476d98702df2e142711).

[Bug 51828] Simucube 2: All applications using raw HID access to communicate with devices, stopped tracking steering axis movement

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51828

--- Comment #1 from Ivo Ivanov  ---
Created attachment 70717
  --> https://bugs.winehq.org/attachment.cgi?id=70717
wine_6.18.log

wine-6.18-187-gaa629c4c722
WINEDEBUG=+timestamp,+pid,+hid,+hidp,+hid_report,+plugplay

[Bug 51828] Simucube 2: All applications using raw HID access to communicate with devices, stopped tracking steering axis movement

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51828

--- Comment #2 from Rémi Bernon  ---
Thanks, I sent some updated patches to fix this too
(https://source.winehq.org/patches/data/216075,
https://source.winehq.org/patches/data/216074,
https://source.winehq.org/patches/data/216076).

Note that the unit tests show that reports *must* be dropped when their size is
invalid.

But, like for the reports coming from user-space, it most likely checks that
the report length matches the declaration for the report id, which is what I
implemented.

[Bug 51828] Simucube 2: All applications using raw HID access to communicate with devices, stopped tracking steering axis movement

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51828

Ivo Ivanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #3 from Ivo Ivanov  ---
(In reply to Rémi Bernon from comment #2)
> Thanks, I sent some updated patches to fix this too
> (https://source.winehq.org/patches/data/216075,
> https://source.winehq.org/patches/data/216074,
> https://source.winehq.org/patches/data/216076).

Indeed this is much more complete solution. I can confirm that everything works
properly now, the reports get through the check as expected. Tested with
5a8dcb062793fbb68997e1b54ebc2666a2b2834d from yesterday.
> Note that the unit tests show that reports *must* be dropped when their size
> is invalid.
>
> But, like for the reports coming from user-space, it most likely checks that
> the report length matches the declaration for the report id, which is what I
> implemented.

Do the unit tests reflect the behavior on Windows? Just trying to understand
better the tests creation process.

[Bug 51828] Simucube 2: All applications using raw HID access to communicate with devices, stopped tracking steering axis movement

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51828

Ivo Ivanov  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |9411ecf6707bd40eddca4e21f43
                   |                            |e1b09282db7a0

[Bug 51828] Simucube 2: All applications using raw HID access to communicate with devices, stopped tracking steering axis movement

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51828

--- Comment #4 from Rémi Bernon  ---
Yes the tests are meant to check how things behave on Windows, and validate
that Wine does the same. They are run when we submit patches, as on
https://testbot.winehq.org/index.pl, and regularly to track regressions on
https://test.winehq.org

[Bug 51828] Simucube 2: All applications using raw HID access to communicate with devices, stopped tracking steering axis movement

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51828

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Alexandre Julliard  ---
Closing bugs fixed in 6.19.

[Bug 51828] Simucube 2: All applications using raw HID access to communicate with devices, stopped tracking steering axis movement

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51828

Gijs Vermeulen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |regression