fix(hid): clamp the hid_read() length to the buffer, not to reportSize()

[nigelfenton]

Not live today, and I want to be plain about that up front: every parser on the tree returns ≤ 64, so nothing currently overflows. What this fixes is that the safety of HidEncoderManager::poll() is a property of its subclasses rather than of itself, and nothing says so.

Split out of #4786, where the Antares scanner flagged the file. Its stated mechanism was wrong — hid_read's return is checked (<0 disconnect, ==0 break) and the parse is bounded by res, so the reported short-read overflow does not exist. One step across from it, though, there is something real.

The problem

uint8_t m_buf[64]{};                                          // HidEncoderManager.h:187
int res = hid_read(m_device, m_buf, m_parser->reportSize());  // .cpp:591

reportSize() is a virtual — it answers "how big is this device's report", a fact about the device. m_buf is a fixed 64 bytes, a fact about us. Passing the first as the bound on a write into the second means every future parser override is silently a memory-safety decision, and the override sites do not declare that.

The margin is not comfortable, either — it is zero. TMate2Parser::reportSize() already returns exactly 64.

Why this is not a hypothetical device

From HidDeviceParser.h, on the StreamDeck+:

14-byte reports (device HID descriptor advertises 512 but only first 14 bytes carry event data...)

So we already talk to hardware whose real report is 512 bytes. That parser returns 14 purely because the trailing bytes carry nothing we decode. A parser written instead to honour its device's actual descriptor would be reasonable, correct by its own lights, and would smash a 64-byte stack buffer on the first packet it received.

The change

const size_t readLen = std::min(m_parser->reportSize(), sizeof(m_buf));

Hoisted out of the loop since it cannot vary within one poll(). <algorithm> was already included. No behaviour change for any current parser: 32, 6, 5, 5, 14 and 64 all pass through untouched.

Test

hid_report_size_clamp_test pins the clamp, not the current parser sizes — a test that only asserted today's six values would pass just as happily with the std::min() deleted.

Verified by breaking it. Removing the clamp fails 4 checks:

FAIL: one byte over capacity is clamped, not passed through
FAIL: a StreamDeck+-sized descriptor report (512) is clamped to the buffer
FAIL: a full 16-bit report size is clamped to the buffer
FAIL: no report size in 0..600 yields a length past the buffer
EXIT=1

...while the six real parser sizes still pass, which is the correct signature: the clamp is inert for existing hardware and load-bearing for anything larger. Restored, it prints all checks passed / exit 0.

No Qt, no aethercore, no hidapi link — the arithmetic is the whole subject, so the test builds and runs on configurations where HID support is compiled out.

Scope

m_buf is the only fixed-size HID read buffer here. I have not audited other hid_read call sites outside this class.

Not exercised on hardware. The clamp is unreachable with any device I own — every parser I could physically test returns ≤ 64, which is precisely why the change is a guard rather than a fix. Reviewed by reading plus the deliberate-break test above.

[aethersdr-agent]

Good change, and I appreciate that the description leads with "this is not live today" instead of dressing a hardening patch up as a fix. I checked the surrounding code: HidDeviceParser::reportSize() is size_t so std::min deduces cleanly, <algorithm> is already included, and every parser in HidDeviceParser.cpp bounds itself on the len argument (if (len < N) return {}) rather than on its own reportSize() — so clamping the read can't turn the write overflow into a read overflow one layer down. Your scope note is also accurate: the only other hid_read() in the tree, UlanziDialWindowsManager.cpp:141, already passes sizeof(buf).

The one thing I'd want addressed is the test. It re-implements the clamp in its own translation unit rather than exercising the production expression, so two claims made in its header comment aren't true of it as written — including the "deleting the std::min() in poll() fails this" claim, which is the load-bearing one.

Would like fixed before merge

• tests/hid_report_size_clamp_test.cpp:37-41 — the test asserts against its own copy of the expression; deleting the clamp from poll() leaves it green. Two header comments claim otherwise.

Polish

• src/core/HidEncoderManager.cpp:605 — a parser asking for more than 64 now silently short-reads forever; worth making that audible.

Non-blocking notes

• The comment block is long for a one-line change, but the reasoning is the valuable part here and I'd rather have it in the file than in the PR description only. Leaving it.

---

_{🤖 aethersdr-agent · cost: $3.0368 · model: claude-opus-5}

[aethersdr-agent]

This is the part I'd like changed. readLenFor() is a copy of the expression, not the expression — nothing in this TU includes HidEncoderManager.cpp or reaches poll(). So:

• Line 19-20's claim, "Deleting the std::min() in poll() fails the oversize case below", isn't true of this test. Deleting it from poll() leaves every check here green; what you actually broke to get the four FAILs in the PR description was this local copy.
• Line 32-34's claim, "a change to the real one that forgets this test shows up as a failure here", is also not true in the direction that matters: widen m_buf to 128 and this still passes at 64.

The clamp is right, so this is about the test earning its keep rather than about the fix. Cheapest way to make it real is to put the bound where both sides can see it — e.g. a static constexpr std::size_t kReadBufSize = 64; in HidEncoderManager.h used for both m_buf[kReadBufSize] and the std::min, and have the test #include "core/HidEncoderManager.h" for the constant (it's header-only, no link needed) plus a static_assert(sizeof(...)). Then a buffer-size change propagates instead of drifting.

If coupling the test to the header isn't worth it, the alternative is to soften the two comments to say plainly that this pins the arithmetic contract and not the call site — a modest test that says what it is beats an ambitious one that doesn't do what its header claims.

[aethersdr-agent]

Worth considering as a follow-on to the clamp: once readLen is capped, a parser that returns > 64 doesn't crash — it quietly receives truncated reports on every poll, forever, and its parse() will just keep returning {} from its own length guard. That's a much better failure than a stack smash, but it's still silent, and the whole point of this PR is that the constraint is currently undeclared at the override sites.

A one-time qCWarning(lcDevices) when m_parser->reportSize() > sizeof(m_buf) (naming the parser) would make the mismatch audible the first time someone writes that parser, which is exactly when they can still fix it. Cheap, and it turns the clamp from a silent backstop into a diagnostic.

[nigelfenton]

Closing this in favour of a comment at the reportSize() declaration instead.

On reflection the code change was speculative hardening, not a fix: no parser on the tree exceeds 64, nothing overflows today, and the failure needs a future parser to return a larger value. The StreamDeck+ evidence I leaned on actually cuts the other way — that parser already correctly returns 14 despite its device advertising 512, so it shows the tree getting this right rather than getting it wrong.

What is genuinely worth keeping is the fact that reportSize() bounds a write into a fixed 64-byte buffer, which the virtual's declaration does not say anywhere. That is a documentation gap, and a comment fixes it without asking a reviewer to take on a runtime change for a hazard nobody has hit. Follow-up note coming against HidDeviceParser.h.

Thanks for the flag regardless — the scanner's stated mechanism was wrong (hid_read's return is checked), but it pointed at a file worth reading.