Skip to content

Image decoder hardening

Prompt

Before responding to questions or discussion points in this document, explore the ryll codebase thoroughly. Read relevant source files, understand existing patterns (SPICE protocol handling, channel architecture, async task model, image decompression, egui rendering), and ground your answers in what the code actually does today. Do not speculate about the codebase when you could read it instead. Where a question touches on external concepts (SPICE protocol, QEMU, QXL, LZ/GLZ/QUIC compression), research as needed to give a confident answer. Flag any uncertainty explicitly rather than guessing.

Consult ARCHITECTURE.md for the system architecture overview, docs/development.md for build commands, and AGENTS.md and STYLEGUIDE.md for project conventions. The canonical QUIC decoder is /srv/src-reference/spice/spice-common/common/quic.c and quic_tmpl.c; LZ and GLZ live alongside them in spice-common/common/.

Situation

In July 2026 a security review of the client filed twelve issues (#171 to #182). Each one lets a malicious SPICE server, or anyone in the middle of a non-TLS connection, crash ryll with a panic or make it allocate gigabytes. PR #437 fixes the three in shakenfist-spice-protocol and the channel read loops (#179,

180, #181), and #178 was already fixed by 80ff6e8. PR #437 says

outright that "the compression-crate half of the cluster (#171-#177) is a separate PR". This plan is that PR, plus #182, which is in the renderer.

All eight issues were re-checked against develop at 78c7238 on 2026-10-03, and every one still applies. Line numbers have drifted since the issues were filed; the ones below are current.

Issue Kind Where (at 78c7238)
#171 panic quic.rs:923, :1001, :1312, :1379 compute run_end = i + run unclamped; quic.rs:646-648 reads J[32] on a 32-entry array
#172 alloc bomb quic.rs:624/:629 take width and height off the wire; reset_channels (:586) resizes four rows at :451 before the cap at :1574 runs
#173 panic display.rs:2179-2187 copies row_bytes = width * 4 per row but only checked stride * height against the buffer
#174 panic display.rs:2304 FromCache arm pairs cached pixels with descriptor dimensions; crop and clip copies at :2511 and :2545 slice past the buffer
#175 alloc bomb lz.rs:67, glz.rs:276, lz4.rs:55 size the output from wire dimensions with checked_mul only
#176 alloc bomb display.rs:2268 read_to_end on a zlib stream with no output limit; _glz_size at :2262 is read and ignored
#177 alloc bomb cursor.rs:532 allocates from u16 cursor dimensions before the pixel-data length checks
#182 debug panic surface.rs:81 dst_stride - left * 4 underflows when left >= width; blit_chroma (:280) and blit_alpha (:342) already guard this

The root cause is the convention, not the call sites

Six of the eight bugs come from one gap. There is no shared limit on how large a decoded image may be. Each decoder either picks its own limit or has none. Today there are four:

  • MAX_DECODED_JPEG_DIMENSION = 16384 per side, in shakenfist-spice-compression/src/jpeg.rs:60, reused by video.rs:52.
  • A bare 16384 literal in quic.rs:1574.
  • MAX_SURFACE_DIMENSION = 16_384 in shakenfist-spice-renderer/src/display/surface.rs:16.
  • MAX_PIXMAP_PIXELS = 64 Mi pixels (8192 x 8192, 256 MiB of RGBA), local to one match arm at display.rs:2140. PR #437 promotes it to module scope and derives MAX_MESSAGE_BODY from it.

LZ, GLZ, LZ4, zlib-GLZ, the cursor decoder and QUIC's per-channel rows have no limit at all. STYLEGUIDE.md's "Dimension safety" section tells authors to use checked_mul on width * height * 4, and nothing more. That is the advice these decoders followed. It prevents overflow, but a 65535 x 65535 image passes it and asks for 17 GiB.

Patching each call site would leave the next decoder to make the same mistake. The fix is one shared limit that every decoder calls, plus a style guide that says to use it.

Mission and problem statement

Make every image decoder in shakenfist-spice-compression and shakenfist-spice-renderer refuse hostile input without panicking or allocating more than one bounded image buffer, and leave a single shared helper and style rule behind so new decoders get this by default.

Specifically:

  1. One pair of limits, a per-side dimension cap and a total pixel cap, defined once in shakenfist-spice-compression and used by every decoder in both crates.
  2. One helper that turns (width, height) into a byte length, or refuses. It replaces the checked-multiply idiom the style guide currently recommends.
  3. DecompressedImage cannot be constructed with a pixel buffer whose length disagrees with its dimensions. This closes #174 at the type rather than at one consumer.
  4. The QUIC run decoder matches upstream spice-common's bounds.
  5. One commit per issue, each with a regression test that fails (panics, or would allocate past the cap) against the old code.

Out of scope: per-channel message size caps (#436), a declarative bounded parser (#136), and fuzz targets for the compression crate (#135). These are recorded under Future work.

Open questions

Questions 1 to 3 were resolved with the operator on 2026-10-03, and each records its decision. Question 4 is resolved by the Step 1a/1b split.

  1. What is the cap? Decided: 16384 per side and 64 Mi pixels, both applied everywhere. The recommendation was to keep both existing values and apply both everywhere. 16384 per side is already used by JPEG, video, QUIC and surfaces. 64 Mi pixels is already used by pixmaps and, after #437, sets MAX_MESSAGE_BODY. With both applied, a 16384 x 4096 image passes and a 16384 x 16384 image (1 GiB of RGBA) does not. The largest realistic surface is an 8K display, 7680 x 4320 = 33 Mi pixels, so this leaves 2x headroom. The behaviour change is that JPEG and video frames above 64 Mi pixels, which are allowed today, will be refused. No SPICE server sends those, so this should be accepted, but it is a change and the PR description must say so.

  2. Should DecompressedImage::new become fallible? Decided: yes, new and new_glz return Option. The compression crate is published on crates.io at 0.x, so a breaking change costs a minor version bump and nothing else. Recommendation: new and new_glz return Option<DecompressedImage>, returning None when pixels.len() != width * height * 4. Callers in the renderer already handle a None image as a decode failure. The alternative, a debug_assert! plus a check in the FromCache arm, fixes #174 and leaves the invariant to be broken again by the next constructor call.

  3. Cursor cap. Decided with question 1: cursors use the shared limits. #177 suggests 16384 to match the other decoders. Real cursors are 32 to 256 pixels on a side, so a much tighter cap (512, say) is defensible. Recommendation: use the shared helper and its limits, for consistency. The cursor then costs at most 256 MiB, which is the same worst case as any other image. Do not add a cursor-specific constant unless review asks for one.

  4. Sequencing with PR #437. #437 moves MAX_PIXMAP_PIXELS to module scope in display.rs and builds MAX_MESSAGE_BODY from it. Re-pointing that constant at the shared limit has to wait for #437. Resolved by splitting Step 1. Step 1a (the limits module, the DecompressedImage change, JPEG and QUIC) does not touch what #437 changes, so it lands first and Steps 2 and 3 build on it. Step 1b re-points the renderer's MAX_PIXMAP_PIXELS and MAX_SURFACE_DIMENSION after #437 merges and this branch is rebased onto it. Neither PR blocks the other's review, but this one should merge second.

Execution

Phase Plan Status Merged
1. Decoder hardening This file, steps below Complete a167670
2. Push audit PLAN-decoder-hardening-phase-02-push-audit.md Complete 7d4abf5

Phase 1: Decoder hardening

This phase is a single pull request with one commit per step below, except Steps 2 to 4, which commit once per issue. Every commit builds and passes make test on its own.

Step Effort Model Isolation Status Brief for sub-agent
1a high opus none Complete Shared limit, helper and DecompressedImage invariant. See brief 1, except its renderer-constant bullet.
1b low sonnet none Complete After #437 merges and the branch is rebased: the renderer-constant bullet of brief 1.
2 high opus none Complete QUIC: #171 then #172, as two commits. See brief 2.
3 medium sonnet none Complete LZ, GLZ, LZ4 and zlib-GLZ: #175 then #176, as two commits. See brief 3.
4 medium opus none Complete Renderer: #177, #173, #174, #182, as four commits. See brief 4.
5 medium sonnet none Complete Documentation. See brief 5.

All six steps landed on the decoder-hardening branch, merged as PR #443 (a167670) on 2026-10-04; Step 1b ran after the branch was rebased onto #437.

Because DecompressedImage::new/new_glz now return Option and MAX_DECODED_RGBA_BYTES dropped from 1 GiB to 256 MiB, the change shipped in a minor release, 0.2.0, rather than a patch; see Choosing the version. The kerbside proxy needs no bump: it depends only on shakenfist-spice-protocol, at a pinned git revision, and the only change to that crate on this branch is an added constant (IMAGE_FLAGS_CACHE_REPLACE_ME, from #457).

Brief 1: shared limit and helper (Step 1a and Step 1b, one commit each).

  • Add shakenfist-spice-compression/src/limits.rs and re-export it from lib.rs. It holds pub const MAX_IMAGE_DIMENSION: u32 = 16384, pub const MAX_IMAGE_PIXELS: usize = 64 * 1024 * 1024, and pub fn rgba_len(width: usize, height: usize) -> Option<usize>. rgba_len returns None when either side is zero, either side exceeds the dimension cap, or the pixel count exceeds the pixel cap. Otherwise it returns width * height * 4, which cannot overflow once both caps hold.
  • Point MAX_DECODED_JPEG_DIMENSION at MAX_IMAGE_DIMENSION, and route its byte ceiling through rgba_len. Replace the literal at quic.rs:1574.
  • Step 1b only: the renderer's MAX_PIXMAP_PIXELS (module scope in display.rs once #437 has landed) is deleted, and MAX_MESSAGE_BODY and the Pixmap arm use MAX_IMAGE_PIXELS and rgba_len directly. MAX_SURFACE_DIMENSION in surface.rs:16 becomes MAX_IMAGE_DIMENSION.
  • Make DecompressedImage::new and new_glz return Option, refusing a pixel buffer whose length is not rgba_len(width, height). Update every caller (grep -rn 'DecompressedImage::new'). The FromCache arm at display.rs:2304 is left for Step 4, which adds a warning there. In this commit it only adapts to the new signature.
  • Rewrite STYLEGUIDE.md's "Dimension safety" section: decoders size buffers with rgba_len and do not hand-roll the checked multiply. Keep the reason, which is attacker-controlled dimensions.
  • Tests in limits.rs cover zero, exactly at each cap, one over each cap, and u32::MAX sides.

Brief 2: QUIC (two commits).

  • 171: in all four run loops (quic.rs:923, :1001, :1312,

    :1379), refuse the run when run > end - i, and return false the way the surrounding decode errors do. Do not silently clamp. This mirrors quic_tmpl.c:567, where run_end > (end - i) is a decode error. In decode_run (quic.rs:646) and its counterpart for the RGB state, change the guard to melcstate < J.len() - 1. Upstream uses MELCSTATES - 1 (quic.c:514, :551). Add tests: one with a crafted stream whose run overshoots the row, and one that drives melcstate to 31. Both panic on the old code. The existing error-handling tests from 8839e63 show how to build streams.

  • 172: in quic_decode_begin (quic.rs:595), check width and

    height with limits::rgba_len straight after they are read at :624 and :629, before reset_channels. Return false on refusal. The later check at :1574 then becomes redundant; delete it rather than leaving two. The test is a header declaring width = 0xFFFF_FFFF, which must return None without allocating.

Brief 3: LZ, GLZ, LZ4 and zlib-GLZ (two commits).

  • 175: replace the checked-multiply blocks before vec![0u8; ...]

    in lz.rs:63-67, glz.rs (around :272-276) and lz4.rs:52-55 with limits::rgba_len. LZ and GLZ return the existing anyhow error with the dimensions in the message. LZ4 returns None. LZ4 still needs its row_bytes checked multiply, because bpp varies. Each decoder gets a test with a 65535 x 65535 header and a tiny payload, which must fail fast. lz4.rs:421 already has an "absurd dimensions" test; check what it covers and extend it rather than duplicating it.

  • 176: in the zlib-GLZ arm (display.rs:2255-2275), bound the

    inflate with decoder.by_ref().take(limit + 1).read_to_end(...) and refuse when more than limit bytes come out. Derive limit from rgba_len(img_desc.width, img_desc.height); a GLZ stream can be slightly larger than its RGBA output (33-byte header, a control byte per 32 literals, and the alpha pass for RGBA), so the implementation bounds the inflate at rgba_len + rgba_len/4 + 64 (glz_stream_limit in display.rs). Validate the declared _glz_size against the same limit and against the inflated length, and rename it now that it is used. The test is a small DEFLATE stream of zeros that inflates past the limit.

Brief 4: renderer (four commits).

  • 177: in decode_cursor_pixels (cursor.rs:522), size rgba

    with limits::rgba_len, and move the pixel-data length checks (around :570, :591 and :612 per the issue, so re-locate them) ahead of the allocation. The test is a 65535 x 65535 cursor header with four bytes of pixel data.

  • 173: in the Pixmap arm (display.rs:2116-2190), refuse unless

    width * 4 <= stride, next to the existing needed_bytes check at :2159. Use checked_mul, because width is from the wire. Follow the arm's existing warn_once! pattern. The test is the issue's case: width = 1_000_000, height = 1, stride = 4, with four bytes of data.

  • 174: Step 1 made the mismatch unconstructible. Here, give the

    FromCache arm a warn_once! when the cached buffer does not fit the descriptor's dimensions, matching the cache-miss warning beside it. The test is the issue's two-message sequence: cache a 2 x 2 pixmap as id 42, then draw FromCache id 42 at 10000 x 10000. Drive it through the same path the display channel's existing draw tests use.

  • 182: give DisplaySurface::blit (surface.rs:66) the same

    left >= self.width || top >= self.height early return as blit_chroma and blit_alpha, and compute copy_width with saturating arithmetic. At left == width the old code copied nothing but marked the surface dirty; the debug underflow needs left > width, or a left large enough to overflow left * 4. The test uses such a left.

Brief 5: documentation (one commit).

  • docs/spice-protocol.md: after "All decompressors output RGBA pixels" (around line 297), add a short "Decode limits" paragraph saying that every decoder refuses images over the shared caps, why, and that a refused image is dropped and not painted, in the same way as the truncated-LZ4 paragraph above it. Name the constants. Do not quote their values, so the prose cannot go stale.
  • shakenfist-spice-compression/README.md: mention the DecompressedImage constructor change and the limits module, if the README documents the API.
  • AGENTS.md item 5, which covers untrusted wire input, gains one sentence pointing at limits::rgba_len. This is a convention change, so it belongs there.

Phase 2: Push audit

Run PUSH-AUDIT.md over Phase 1's merge commit, a167670; the phase plan is PLAN-decoder-hardening-phase-02-push-audit.md. Phase 1's Merged cell is recorded by the first commit of this phase's branch. Findings land as their own pull request, and that pull request closes this plan out. If the audit finds nothing that needs a code change, this phase sets the plan Complete in its own pull request.

Administration and logistics

Success criteria

  • All eight issues are closed by the Phase 1 pull request body.
  • grep -rn 'vec!\[0u8' shakenfist-spice-compression/src shakenfist-spice-renderer/src shows no non-test allocation sized from wire dimensions without going through rgba_len.
  • Each fix has a regression test that fails against develop at 78c7238. Exceptions: #177's 65535x65535 short-data test does not fail on a host with lazy overcommit (the 16 GiB zeroed allocation is never touched), so its regression proof is the over-cap test instead; #172's and #175's oversized tests are proven by reasoning, not by running the old code, which would attempt multi-GiB allocations.
  • pre-commit run --all-files and make test pass on every commit.
  • The style guide, docs/spice-protocol.md and AGENTS.md describe the shared limit, and no prose quotes its value.

Items deferred from the push audit

Phase 2 audited a167670^1..a167670 (22 files, 1 889 insertions) and produced 30 findings once duplicates were merged. The triage table, with each finding located in current develop, is in PLAN-decoder-hardening-phase-02-push-audit.md. The security review found all nine issues closed. Nothing the audit found reopens one.

To fix in the findings pull request (16):

  • Tests:
  • T1 (blocking): the #173 test's width is refused by the per-side cap before the stride guard runs. Use 16384 x 1 at stride 4, which the old code also panics on.
  • T2: add an in-cap FromCache mismatch, a 3x3 descriptor over a cached 2x2.
  • T3: drive a ZlibGlzRgb image through handle_message.
  • T5: add QUIC headers at the pixel cap and one over it.
  • T8: add a cursor over the pixel cap with in-cap sides.
  • QUIC run guard:
  • Q1 and T4: fold the four run-overshoot guards into one helper, so a single test covers all four loops, the tail loops included.
  • Comments:
  • Q3 and Q4: cut the repeated and over-long comments to their reasons.
  • Q6: the LZ4 pixel-cap test uses sides that are both legal (8192 x 8193).
  • Messages:
  • W1: the pixmap refusal prints {}x{}.
  • Docs:
  • D1 and D2: trim the "Decode limits" paragraph to the contract, and make the docs and jpeg.rs give one account of how the platform JPEG backends are bounded.
  • D3: drop the module-history sentence from limits.rs.
  • Security:
  • S3: bound the zlib-GLZ inflate by the validated declared_glz_size, not by the outer limit.
  • S4: check the pure-Rust JPEG backend's dimensions from read_info() before decode().

The findings pull request fixed all 16, one commit per finding. Two of the new or changed tests prove less than intended. T2's in-cap mismatch is also refused by DecompressedImage::new, so removing the display channel's own pre-clone check fails no test; the test shows the mismatch is refused, not which layer refuses it. Q6's corrected sides make the LZ4 case say what it means, but a payload that small is refused whether or not the pixel cap applies, which is T7's declined point. The QUIC tail run loops (T4) are covered through the shared helper's unit test, not through the decoder: they only run on images over 2048 pixels, which cannot be hand-packed.

Filed as issues, outside this plan (5): problems that predate it, which the audit found in files it read.

  • S1, #626 (high): the LZ/GLZ back-reference one pixel before the start panics.
  • Q8/S2, #627 (medium): surfaces skip the pixel cap and have no count limit. Q5, the stale "1 GiB" comment, goes with that fix.
  • W3/S5, #628 (low): decode refusals log at message rate. The compression crate has no warn_once!, so this is a design question rather than an edit.
  • Q9, #629: display.rs is about 6 000 lines.

Declined (8):

  • W2: the pixmap:too_large key's message names both limits, so the log is accurate, and renaming a key breaks anyone grepping for it.
  • Q2: one shared refusal type across nine sites, whose error types differ (anyhow, Option, warn_once!), would cost more churn than the uneven wording does. W1 fixes the one real inconsistency.
  • T6: hand-packed QUIC bitstreams are the only practical way to reach those states, and the tests document the model state they rely on.
  • T7: an allocation that was never made cannot be observed without an allocator hook. The LZ and GLZ tests do tell old code from new. The LZ4 case is covered by rgba_len's unit tests.
  • D4: the "Breaking changes" heading is a reviewer convention. A pull request template is a repository-wide change, not this plan's.
  • D5: the guest-driver claims about QXL_IMAGE_CACHE come from the driver sources, which are not in this repository. Checking them belongs to a docs audit.
  • S7: runlen overflow needs a 32-bit usize, ryll ships only 64-bit targets, and the run guard makes it harmless in release.
  • Q10/S6: DecompressedImage's pub fields. Nothing mutates them, and the security review found no path by which server data could. Making them private is a breaking change, and 0.2.0 and 0.3.0 have both shipped since the plan's own break, so it would need a release of its own.

No action (1): Q7, already fixed by df2f06b.

Future work

  • decode_image_and_emit (display.rs, the no_image_data guard if image_data_start >= payload.len()) refuses an image whose descriptor ends exactly at the end of the payload. A FromCache SpiceImage has no bytes after its descriptor, so if it is the last pointed-to data in a DRAW_COPY (no mask), every such draw would be dropped. Found during Step 4 and tracked as #442; fixed by #457, which merged into this branch.
  • Fuzz targets for the compression crate's decoders (#135). The helper makes "never allocates past the cap" a property a fuzzer can assert. The LZ4 decoder has one since 9364569 (fuzz_lz4_decode); QUIC, LZ, GLZ and the cursor decoder do not.
  • Per-channel message size caps (#436).
  • The four issues the push audit filed: #626, #627, #628 and #629 (see above).

Bugs fixed during this work

Issues #171, #172, #173, #174, #175, #176, #177 and #182, plus #442 (found here, fixed by #457, which merged into this branch).

Back brief

Before executing any step of this plan, please back brief the operator as to your understanding of the plan and how the work you intend to do aligns with that plan.

📝 Report an issue with this page