(master) test/pyxtest: cover unbounded DEVICE_RESOLUTION SwapLongs

[metux]

Send a swapped ChangeDeviceControl/DEVICE_RESOLUTION with
num_valuators=255 and no valuator payload. Without the SProc length
check this overruns the request buffer; with it the server returns
BadLength.

Co-authored-by: Cursor cursoragent@cursor.com
Signed-off-by: Olivier Fourdan ofourdan@redhat.com
Part-of: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2285
(cherry picked from commit ddf3edc)
(cherry picked from commit 3ed09c52d4a9cfe85b5f9d2f64beac022d79b221)
(cherry picked from commit 3f8dfe0)

---

Backports

Release	Backport PR	Review
release/25.0	#3792	merged

Release merges are manual, by the maintainer.

[metux]

🤖 Automated review — generated by Starfleet ship Voyager (model: heavy-model) on behalf of @metux. Not a human review.

Review: passed with two advisories

First, a process note against my own review: this PR already carried a
bot-review-passed label from me with no review comment attached. That was wrong —
I labelled it in a batch without writing the review. The code is fine; the two advisories
below are quality items, not defects.

What this test actually protects

Regression test for the DEVICE_RESOLUTION SwapLongs bound — the same defect that
dix/ had to bound on the swapped-client path. The request declares num_valuators=255
(also the largest a CARD8 can hold, so the server has no way to reject it on that field
alone) while carrying zero valuator entries in the body. Before the bound, the server
would SwapLongs across the end of the request and into whatever followed.

The canary is the good part of this test and worth calling out: appending an
InternAtom request behind the malformed one and asserting its reply is intact catches
memory corruption that would otherwise pass silently. A "server still alive and
returned BadLength" assertion alone would not have — the corrupted bytes could just as
well have landed in the canary's own reply data. Asserting
struct.unpack_from(f"{bo}I", resp_canary.data, 8)[0] != 0 checks a field the attacker
does not control, so it is a real tripwire rather than a tautology.

Assertions verified as reachable: struct is already imported (test_xi.py:5), as are
pytest, proto.x11, proto.xi, xclient.Extension, X11Error and X11Reply. The
InternAtomReply atom is read at offset 8 — past the 4-byte type and 4-byte sequence —
with the client's byte order, which is right for a byte-swapped client.

Advisory — the test will error instead of skipping when XInput is absent

        opcode = conn.query_extension(Extension.XI).opcode

query_extension() returns None when the extension is missing, so this raises
AttributeError rather than skipping. The test directly above it — same class, same
extension, ten lines up — does handle this:

        ext = conn.query_extension(Extension.XI)
        if not ext:
            pytest.skip("XInput extension not available")

That guard exists for a reason, and this file uses query_extension(Extension.XI).opcode
in five places. It would be worth checking whether the other four are also unguarded, but
in this new test the skip should be added to match its sibling — otherwise an
-Dxinput=false build turns a test into an error.

Advisory — conn.seq += 1 is a no-op and has no precedent

        conn.send_request(bad.to_bytes(bo) + canary.to_bytes(bo))
        conn.seq += 1

Two things here. First, recv_response() never validates the sequence number — I read
it (xclient.py:273-310) and there is no seq check in the response path — so this
increment cannot affect the outcome. Second, send_request() already does
self.seq += 1 (xclient.py:268), and this manual bump is the only such line in the
whole pyxtest directory apart from that one.

The intent is presumably clear (two requests went out in one call, so bump twice), and
it is harmless today. But as written it reads as a meaningful protocol step that a future
reader will either puzzle over or remove — and if recv_response ever grows sequence
validation, the line silently becomes a bug rather than documentation. Either drop it or
replace it with a comment saying why it is there.

Backport

Test-only. No runtime code is touched, so there is nothing to backport and nothing that
can regress a release line.