Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sendReport data size validation is platform-dependent #86

Open
yume-chan opened this issue Dec 3, 2021 · 3 comments
Open

sendReport data size validation is platform-dependent #86

yume-chan opened this issue Dec 3, 2021 · 3 comments
Assignees

Comments

@yume-chan
Copy link

I'm working on https://github.com/yume-chan/g-web which can change settings on Logitech mouses.

  • On Windows 10, Microsoft Edge 98.0.1092.0, it works.
  • On macOS 12.6, Microsoft Edge 98.0.1092.0, the sendReport calls got rejected with error DOMException: Failed to write the report..

edge://device-log:

HID Event [17:32:54] Failed to set report: 0xe0005000

After investigation, I found it's caused by the mismatch between my sendReport data size and expected report size.

The report id 17 is like

{
    "items": [
        {
            "hasNull": false,
            "hasPreferredState": true,
            "isAbsolute": true,
            "isArray": true,
            "isBufferedBytes": false,
            "isConstant": false,
            "isLinear": true,
            "isRange": false,
            "isVolatile": false,
            "logicalMaximum": 0,
            "logicalMinimum": 255,
            "physicalMaximum": 0,
            "physicalMinimum": 0,
            "reportCount": 19,
            "reportSize": 8,
            "unitExponent": 0,
            "unitFactorCurrentExponent": 0,
            "unitFactorLengthExponent": 0,
            "unitFactorLuminousIntensityExponent": 0,
            "unitFactorMassExponent": 0,
            "unitFactorTemperatureExponent": 0,
            "unitFactorTimeExponent": 0,
            "unitSystem": "none",
            "usages": [
                4278190082
            ],
            "wrap": false
        }
    ],
    "reportId": 17
}

On Windows I can send only 3 bytes to it, but on macOS I must send exactly 19 bytes.

https://github.com/yume-chan/g-web/blob/c474f86663b03a17b2456b3af589d63047d58b1b/libraries/hidpp/src/hidpp.ts#L212-L225

public sendPacket(
  reportId: number,
  reportSize: number,
  data: ArrayBuffer
) {
  // On macOS, the `data` must be exactly `reportSize` length
  if (data.byteLength < reportSize) {
    const temp = new ArrayBuffer(reportSize);
    new Uint8Array(temp).set(new Uint8Array(data), 0);
    data = temp;
  }

  return this.device.sendReport(reportId, data);
}

The spec of sendReport has no requirement on the data size, so I guess the current implementation is platform-dependent. But as a Web standard, I expect a consistent and documented behavior across platforms.

@nondebug
Copy link
Collaborator

nondebug commented Dec 7, 2021

Ideally, WebHID sends every report without modifying the report data and lets the device (not the browser or operating system) decide whether the report is valid. When you tried to send a 3 byte report, the device stalled waiting for more data which is what it says it should do according to its report descriptor. The report descriptor defines the exact length of each supported report, but some devices have errors in the report descriptor that enable them to accept reports with "wrong" lengths. Sometimes an error makes a device unusable or unstable through the platform API, but if it's still usable and stable at the platform level then (ideally) it should be usable in WebHID.

Windows complicates this. Instead of requiring the report buffer to be the correct length for that report, Windows requires the buffer to be large enough to hold the largest report of that type (input, output, or feature). To work around this quirk, the Chromium implementation always copies output reports into a correctly-sized buffer before handing it to the Windows API. This explains why you never saw any errors when testing with too-short reports on Windows, the browser padded it for you.

One option is to simulate a write failure on Windows if the buffer is shorter than the report length described in the report descriptor. The problem is we can't actually implement this reliably because the Windows platform HID API doesn't provide the length of each report.

Another option is to replicate the Windows buffer resizing behavior on other platforms so all platforms are equally tolerant of too-short reports. I'm reluctant to recommend this since it would potentially break devices with report descriptor errors that require the shorter buffer.

A third option is to notify the developer of the potential inconsistency without causing an error. This only makes sense on Windows since on other platforms the transfer will fail anyway.

On Windows we don't know the exact report length because of how the platform HID API works. We can iterate over all the items in a report, but "constant" items (typically used for padding) are skipped. When padding appears within a report we can detect it as a gap between other non-constant items, but not when it appears at the end of a report. In practice this means we can find a minimum bound for the report's length which should be enough to display a warning that the report buffer seems too short. The usefulness of a warning would be limited since we don't actually know the correct size, the developer will need to do additional investigation to determine it.

@yume-chan
Copy link
Author

yume-chan commented Dec 7, 2021

The problem is we can't actually implement this reliably because the Windows platform HID API doesn't provide the length of each report.

I only checked my device on both Windows and macOS, but it looks like all reportCount and reportSize values are identical https://www.diffchecker.com/A7GhDmOL (left is Windows and right is macOS)

Windows requires the buffer to be large enough to hold the largest report of that type (input, output, or feature).

So in theory I can also pass a larger data to an output report? What will happen then?

the developer will need to do additional investigation to determine it.

The developer usually have the device's documentation, so they should know the correct report size. Maybe the spec can mention "data passed to sendReport should always have correct size" to prevent developers like me trying to take a shortcut.

@nondebug nondebug self-assigned this Dec 7, 2021
@nondebug
Copy link
Collaborator

nondebug commented Dec 7, 2021

left is Windows and right is macOS

reportId 32 has an example of what I'm talking about. Notice how on Windows there are two items while on macOS there's only one. The extra item on Windows has isConstant=true, reportCount=1, reportSize=136. That's 17 bytes of constant padding to extend the length of the report to the maximum input report length (31 bytes).

So in theory I can also pass a larger data to an output report? What will happen then?

I think this depends on the device, but if the device wasn't expecting you to send more data then you'd get a device error of some sort. If the device expects you to send the larger report then it could succeed.

The developer usually have the device's documentation, so they should know the correct report size.

Maybe, maybe not. Part of the benefit of HID is you can write generic drivers that work using the information in the report descriptor without relying on prior knowledge about specific devices. If you're writing a generic driver then you may not know the report size and will need to construct it from the information provided by the API.

Maybe the spec can mention "data passed to sendReport should always have correct size" to prevent developers like me trying to take a shortcut.

SGTM, I'll add a note to the spec that mentions the platform inconsistency and gives a recommendation for how to compute the size of the report buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants