A truncated access unit stops aborting the decoder, and a pad's OS identity stops being the client's own numbering #418

Merged

enricobuehler merged 2 commits from worktree-avi-high-fixes into main 2026-08-27 16:56:45 +00:00

Conversation 0 Commits 2 Files Changed 7

+695 -4

enricobuehler commented 2026-08-27 16:41:23 +00:00

Owner

Copy Link

Copy Source

The two HIGH findings from the 2026-08-27 audio/video/input sweep. The other 71 findings are a register in punktfunk-planning design/core-planes-sweep-2026-08-27.md (bfa794f); nothing else from it is touched here.

Both are the same shape: a number that arrived from outside was trusted as if it described something we owned.

---

1. An over-declared OBU is a parse error, not a decode-thread panic

obu_size is a leb128 read straight out of the stream — bounded only by u32::MAX, and tied to nothing about how many bytes are actually present. read_obu then built the OBU with an unchecked &data[start_offset..start_offset + obu_size], so any access unit whose last OBU declared more payload than remained panicked with range end index .. out of range. That is a bounds check rather than arithmetic, so it panics in release too, and it aborts whichever thread is decoding.

It reaches every native AV1 rung — pf-vkdecode, pf-dxvadec and pf-vaadec are all re-exports of pf_bitstream::av1::Av1Planner, whose plan_au hands raw access-unit bytes straight to this function. PUNKTFUNK_AU_FAULT=truncate produces the shape (its FaultMode::Truncate docs reason only about Annex-B, where a NALU carries no length — AV1 OBUs do), and so does any AU delivered short over the wire.

This was a hole in an otherwise consistent posture rather than a missing idea: plan_au degrades every other malformation to TruncatedAu/Parse, and pf-vkdecode already re-validates obu.end > au.len() one layer up. So the guard goes in the one place all three rungs route through, with checked_add plus a length compare, and the checked end is reused for bytes_used so the slice and the advance can no longer disagree.

Recorded as PROVENANCE.md deviation 14 — the same class as deviations 7, 9, 10, 11, 12 and 13. Not filed upstream.

2. A pad's OS identity is host-wide, not the client's wire index

Every OS-level name a virtual pad needs is derived from a pad index and nothing else: the Global\pfxusb-boot-<i> / Global\pfds-boot-<i> mailboxes, the SwDeviceCreate instance ids (pf_xusb_<i>, pf_pad_<i>, pf_ds4_<i>, pf_xbox_<i>), and on Linux the DualSense pairing MAC, the Deck serial and the Switch MAC — the last three documented as needing to be unique per pad, because hid-playstation adopts the MAC as the HID uniq and SDL/Steam dedup controllers by that serial.

The host serves up to DEFAULT_MAX_CONCURRENT sessions of the same desktop, each with its own input thread and its own router, and every client numbers its first controller wire pad 0. So two paired clients each holding a controller collide on all of them:

• Windows — the second session's Shm::create_named sees ERROR_ALREADY_EXISTS on all five retries and never gets a pad for the whole session. Worse, the create-failure hint tells the operator to Restart-Service PunktfunkHost, which would kill both sessions, and no other process is involved at all.
• Linux — nothing errors. Both mint the same DualSense MAC, hid-playstation writes it into HID_UNIQ for both, and SDL/Steam merge the two pads into one controller.

The wire index is a session's own numbering and cannot be an OS identity. New pf_inject::pad_pool makes the OS slot host-wide — claimed on a pad's first present frame, released when it goes away, freed wholesale when the session drops — and Pads translates once on the way in. Because only the number changes and not the name format, the drivers (which read the index back out of pszDeviceLocation) need no change.

Slots are claimed lazily rather than as fixed per-session windows, so a single session still reaches all MAX_PADS pads; two sessions share the range. An exhausted host now declines with an honest line instead of retrying against a name it can never win.

Feedback reverses the same map, which the finding did not call out and is the part worth reviewing: a backend tags rumble and rich HID output with the OS slot it created the device under, so Pads::pump maps it back to the client's wire index. Without that, this fix would have delivered one client's rumble to another client's pad — a subtler bug than the one being fixed. HidOutput::pad/with_pad keeps that translation in one place so a seventh variant cannot silently forget it.

One deliberate trade-off, commented at the site: a slot is released as soon as the removal reaches the backend, while the devnode itself lingers for pad_slots::SWEEP_GRACE (300 ms). A session claiming that slot inside the window can still lose the create race — one IndexOwnedElsewhere and the existing backoff, self-healing. Holding the slot until the sweep fired would trade that transient for a permanent leak on any session that unplugs a pad it never re-plugs.

---

Verification

pf-bitstream	100 tests. Two new AV1 tests, both non-vacuous — reverting the guard reproduces the original panic verbatim (range end index 10600 out of range for slice of length 5293)
pf-inject	40 tests (8 new: the collision itself, single-session reach, release, drop, exhaustion, mask translation, reverse map)
punktfunk-core	502 tests with --features quic — quic is not a default feature, so a plain cargo check -p punktfunk-core compiles none of datagram.rs
punktfunk-host	cargo check clean on linux/amd64 in punktfunk-rust-ci. punktfunk-host does not build on macOS at all, and scripts/xcheck.sh does not cover it — this is the only compile signal native/input.rs gets, and it was re-run after cargo fmt, against the committed tree
fmt	cargo fmt --check clean

Not covered

• Nothing here has been on glass. Both fixes are reasoned + tested, not field-reproduced. The pad fix in particular wants two concurrent sessions each with a controller, on Windows and on Linux — that is the case the whole change exists for, and no test here can stand in for it.
• The Windows half is compile-unverified. xcheck.sh does not reach punktfunk-host or pf-inject, so the Windows backends are covered only by the Linux compile of the shared Pads code plus review. The cfg(windows) manager bodies are untouched by this diff, which is what limits the risk.
• Pad-audio endpoint minting has a similar index-keyed shape but is a separate (medium) finding, deliberately left out of scope.

The two HIGH findings from the 2026-08-27 audio/video/input sweep. The other 71 findings are a register in punktfunk-planning `design/core-planes-sweep-2026-08-27.md` (`bfa794f`); nothing else from it is touched here. Both are the same shape: a number that arrived from outside was trusted as if it described something we owned. --- ## 1. An over-declared OBU is a parse error, not a decode-thread panic `obu_size` is a leb128 read straight out of the stream — bounded only by `u32::MAX`, and tied to nothing about how many bytes are actually present. `read_obu` then built the OBU with an unchecked `&data[start_offset..start_offset + obu_size]`, so any access unit whose last OBU declared more payload than remained panicked with `range end index .. out of range`. That is a bounds check rather than arithmetic, so it panics in **release** too, and it aborts whichever thread is decoding. It reaches every native AV1 rung — `pf-vkdecode`, `pf-dxvadec` and `pf-vaadec` are all re-exports of `pf_bitstream::av1::Av1Planner`, whose `plan_au` hands raw access-unit bytes straight to this function. `PUNKTFUNK_AU_FAULT=truncate` produces the shape (its `FaultMode::Truncate` docs reason only about Annex-B, where a NALU carries no length — AV1 OBUs do), and so does any AU delivered short over the wire. This was a hole in an otherwise consistent posture rather than a missing idea: `plan_au` degrades every *other* malformation to `TruncatedAu`/`Parse`, and `pf-vkdecode` already re-validates `obu.end > au.len()` one layer up. So the guard goes in the one place all three rungs route through, with `checked_add` plus a length compare, and the checked end is reused for `bytes_used` so the slice and the advance can no longer disagree. Recorded as PROVENANCE.md deviation 14 — the same class as deviations 7, 9, 10, 11, 12 and 13. **Not filed upstream.** ## 2. A pad's OS identity is host-wide, not the client's wire index Every OS-level name a virtual pad needs is derived from a pad index and nothing else: the `Global\pfxusb-boot-<i>` / `Global\pfds-boot-<i>` mailboxes, the `SwDeviceCreate` instance ids (`pf_xusb_<i>`, `pf_pad_<i>`, `pf_ds4_<i>`, `pf_xbox_<i>`), and on Linux the DualSense pairing MAC, the Deck serial and the Switch MAC — the last three documented as needing to be unique per pad, because `hid-playstation` adopts the MAC as the HID `uniq` and SDL/Steam dedup controllers by that serial. The host serves up to `DEFAULT_MAX_CONCURRENT` sessions of the same desktop, each with its own input thread and its own router, and **every client numbers its first controller wire pad 0**. So two paired clients each holding a controller collide on all of them: * **Windows** — the second session's `Shm::create_named` sees `ERROR_ALREADY_EXISTS` on all five retries and never gets a pad for the whole session. Worse, the create-failure hint tells the operator to `Restart-Service PunktfunkHost`, which would kill *both* sessions, and no other process is involved at all. * **Linux** — nothing errors. Both mint the same DualSense MAC, `hid-playstation` writes it into `HID_UNIQ` for both, and SDL/Steam merge the two pads into one controller. The wire index is a session's own numbering and cannot be an OS identity. New `pf_inject::pad_pool` makes the OS slot host-wide — claimed on a pad's first present frame, released when it goes away, freed wholesale when the session drops — and `Pads` translates once on the way in. Because only the *number* changes and not the name *format*, the drivers (which read the index back out of `pszDeviceLocation`) need no change. Slots are claimed lazily rather than as fixed per-session windows, so a single session still reaches all `MAX_PADS` pads; two sessions share the range. An exhausted host now declines with an honest line instead of retrying against a name it can never win. **Feedback reverses the same map**, which the finding did not call out and is the part worth reviewing: a backend tags rumble and rich HID output with the OS slot it created the device under, so `Pads::pump` maps it back to the client's wire index. Without that, this fix would have delivered one client's rumble to another client's pad — a subtler bug than the one being fixed. `HidOutput::pad`/`with_pad` keeps that translation in one place so a seventh variant cannot silently forget it. One deliberate trade-off, commented at the site: a slot is released as soon as the removal reaches the backend, while the devnode itself lingers for `pad_slots::SWEEP_GRACE` (300 ms). A session claiming that slot inside the window can still lose the create race — one `IndexOwnedElsewhere` and the existing backoff, self-healing. Holding the slot until the sweep fired would trade that transient for a permanent leak on any session that unplugs a pad it never re-plugs. --- ## Verification | | | |---|---| | `pf-bitstream` | 100 tests. Two new AV1 tests, **both non-vacuous** — reverting the guard reproduces the original panic verbatim (`range end index 10600 out of range for slice of length 5293`) | | `pf-inject` | 40 tests (8 new: the collision itself, single-session reach, release, drop, exhaustion, mask translation, reverse map) | | `punktfunk-core` | 502 tests with `--features quic` — `quic` is **not** a default feature, so a plain `cargo check -p punktfunk-core` compiles none of `datagram.rs` | | `punktfunk-host` | `cargo check` clean on linux/amd64 in `punktfunk-rust-ci`. `punktfunk-host` does not build on macOS at all, and `scripts/xcheck.sh` does not cover it — this is the only compile signal `native/input.rs` gets, and it was re-run **after** `cargo fmt`, against the committed tree | | fmt | `cargo fmt --check` clean | ## Not covered * **Nothing here has been on glass.** Both fixes are reasoned + tested, not field-reproduced. The pad fix in particular wants two concurrent sessions each with a controller, on Windows and on Linux — that is the case the whole change exists for, and no test here can stand in for it. * **The Windows half is compile-unverified.** `xcheck.sh` does not reach `punktfunk-host` or `pf-inject`, so the Windows backends are covered only by the Linux compile of the shared `Pads` code plus review. The `cfg(windows)` manager bodies are untouched by this diff, which is what limits the risk. * Pad-audio endpoint minting has a similar index-keyed shape but is a separate (medium) finding, deliberately left out of scope.

enricobuehler added 2 commits 2026-08-27 16:41:27 +00:00

fix(bitstream/av1): an over-declared OBU is a parse error, not a decode-thread panic ... 5317c583ab

`obu_size` is a leb128 read straight out of the stream — bounded only by `u32::MAX`,
and tied to nothing about how many bytes are actually present. `read_obu` then built
the OBU with an unchecked `&data[start_offset..start_offset + obu_size]`, so any
access unit whose last OBU declared more payload than remained panicked with
`range end index .. out of range for slice of length ..`. That is a bounds check
rather than arithmetic, so it panics in release too, and it aborts whichever thread
is decoding.

It reaches every native AV1 rung: pf-vkdecode, pf-dxvadec and pf-vaadec are all
re-exports of `pf_bitstream::av1::Av1Planner`, whose `plan_au` hands raw access-unit
bytes straight to this function. `PUNKTFUNK_AU_FAULT=truncate` produces the shape,
and so does any AU delivered short over the wire.

This was a hole in an otherwise consistent posture, not a missing idea: `plan_au`
degrades every other malformation to `TruncatedAu`/`Parse`, and pf-vkdecode already
re-validates `obu.end > au.len()` a layer up. Bound it once, where all three rungs
route through. The checked end is reused for `bytes_used`, so the slice and the
advance can no longer disagree.

Both tests fail without the guard — the parser one reproduces the original panic
verbatim (`range end index 10600 out of range for slice of length 5293`).

Recorded as PROVENANCE.md deviation 14. Not filed upstream.

fix(host/input): a pad's OS identity is host-wide, not the client's wire index ... 86fbd8121b

Every OS-level name a virtual pad needs is derived from a pad index and nothing
else: the `Global\pfxusb-boot-<i>` / `Global\pfds-boot-<i>` bootstrap mailboxes, the
`SwDeviceCreate` instance ids (`pf_xusb_<i>`, `pf_pad_<i>`, `pf_ds4_<i>`,
`pf_xbox_<i>`), and on Linux the DualSense pairing MAC, the Deck serial and the
Switch MAC — the last three documented as needing to be unique per pad, because
hid-playstation adopts the MAC as the HID `uniq` and SDL/Steam dedup by that serial.

The host serves up to DEFAULT_MAX_CONCURRENT sessions of the same desktop, each with
its own input thread and its own router, and every client numbers its first
controller wire pad 0. So two paired clients each holding a controller collide on all
of them. On Windows the second session's `Shm::create_named` sees ERROR_ALREADY_EXISTS
on all five retries and never gets a pad for the whole session — and the create-failure
hint tells the operator to restart the service, which would kill both sessions, when no
other process is involved at all. On Linux nothing errors: both mint the same DualSense
MAC and SDL merges the two pads into one controller.

The wire index is a session's own numbering and cannot be an OS identity. New
`pf_inject::pad_pool` makes the OS slot host-wide — claimed on a pad's first present
frame, released when it goes away, freed wholesale when the session drops — and `Pads`
translates once on the way in. Because only the NUMBER changes and not the name format,
the drivers (which read the index back out of `pszDeviceLocation`) need no change.

Slots are claimed lazily rather than as fixed per-session windows, so a single session
still reaches all MAX_PADS pads; two sessions share the range. An exhausted host now
declines with an honest line instead of retrying against a name it can never win.

Feedback reverses the same map: a backend tags rumble and rich HID output with the OS
slot it created the device under, so `Pads::pump` maps it back to the client's wire
index — otherwise the fix would have delivered one client's rumble to another's pad.
`HidOutput::pad`/`with_pad` keep that translation in one place, so a seventh variant
cannot silently forget it.

Verified: pf-inject 40 tests (8 new, covering the collision, single-session reach,
release, drop, exhaustion, mask and reverse-map), punktfunk-core 502 with `--features
quic`, and `cargo check -p punktfunk-host` clean on linux/amd64 in punktfunk-rust-ci —
punktfunk-host does not build on macOS, so that check is the only compile signal for
this file and it was not skipped.

enricobuehler merged commit 7c0faeebc8 into main 2026-08-27 16:56:45 +00:00

enricobuehler deleted branch worktree-avi-high-fixes 2026-08-27 16:56:52 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

FernandoMX FlipperDan HitFrostbite bluelightspecial enricobuehler (Enrico Bühler) kszlim luxus nannku neoney

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unom/punktfunk#418