[v0.1/P0] Bound receiver allocations from protobuf scalar claims #49

Closed
opened 2026-09-03 16:43:17 +09:00 by natsukium · 1 comment
Owner

Problem

The frame-body ceiling bounds bytes received, but not memory a small protobuf frame can make the client allocate.

At review snapshot 5077d74b:

  • TryFrom<v1::GridDims> for GridDims only narrows each axis to u16. It accepts 65535 × 65535, although GridDims is documented as already admitted and the supported row/column maximum is 2048. GridMsg::Size then resizes the client shadow from that tiny frame.
  • ImageMsg::Header.total_bytes is a u64 copied directly into vec![0; capacity] in ImageShadow without the daemon's 64 MiB per-image cap.
  • ImageShadow::apply_frame_header fills every missing slot up to the wire-supplied u32 frame index, so Some(u32::MAX) can consume the process from a small frame.
  • The client mirrors up to the daemon's 256 MiB per-session image store but does not independently enforce that aggregate while applying headers/replacements.

This contradicts the protocol anti-corruption-layer claim that schema-inexpressible invariants are validated once on ingress. It also makes #16's frame cap insufficient as a memory-safety boundary.

Required change

Define receiver-side semantic admission for every wire value that controls allocation or indexed growth. The receiver must reject the claim before mutating state or allocating proportional memory; producer-side checks are not a substitute.

Acceptance criteria

  • Announced/reported GridDims accept only admitted row/column bounds and valid pixel sentinels/ranges. Requested geometry remains the separate RequestedDims path because create refusal and resize clamping have different semantics.
  • Image dimensions, decoded byte count, frame index/count, and aggregate retained image bytes are checked against protocol-visible limits before allocation or vector growth.
  • Shared limits live below daemon/client policy code so both encoder and decoder use the same values; the client does not depend on a daemon-private MAX_DECODED_BYTES comment.
  • An audit covers every other scalar that controls Vec/table growth after protobuf decode, not only the examples above.
  • Hand-built tiny protobuf frames with maximal scalar values return typed connection-local errors without large allocation, panic, long loop, or partial shadow mutation.
  • Boundary tests cover exactly-at-limit and one-past-limit values.
  • docs/reference/ipc.md names these semantic limits and distinguishes them from the frame-body limit.
  • #14 owns aggregate daemon admission and handshake limits.
  • #16 owns outbound frame and operation-specific payload limits.
  • This issue owns receiver-side claims whose encoded frame can remain small.
## Problem The frame-body ceiling bounds bytes received, but not memory a small protobuf frame can make the client allocate. At review snapshot `5077d74b`: - `TryFrom<v1::GridDims> for GridDims` only narrows each axis to `u16`. It accepts `65535 × 65535`, although `GridDims` is documented as already admitted and the supported row/column maximum is 2048. `GridMsg::Size` then resizes the client shadow from that tiny frame. - `ImageMsg::Header.total_bytes` is a `u64` copied directly into `vec![0; capacity]` in `ImageShadow` without the daemon's 64 MiB per-image cap. - `ImageShadow::apply_frame_header` fills every missing slot up to the wire-supplied `u32` frame index, so `Some(u32::MAX)` can consume the process from a small frame. - The client mirrors up to the daemon's 256 MiB per-session image store but does not independently enforce that aggregate while applying headers/replacements. This contradicts the protocol anti-corruption-layer claim that schema-inexpressible invariants are validated once on ingress. It also makes #16's frame cap insufficient as a memory-safety boundary. ## Required change Define receiver-side semantic admission for every wire value that controls allocation or indexed growth. The receiver must reject the claim before mutating state or allocating proportional memory; producer-side checks are not a substitute. ## Acceptance criteria - Announced/reported `GridDims` accept only admitted row/column bounds and valid pixel sentinels/ranges. Requested geometry remains the separate `RequestedDims` path because create refusal and resize clamping have different semantics. - Image dimensions, decoded byte count, frame index/count, and aggregate retained image bytes are checked against protocol-visible limits before allocation or vector growth. - Shared limits live below daemon/client policy code so both encoder and decoder use the same values; the client does not depend on a daemon-private `MAX_DECODED_BYTES` comment. - An audit covers every other scalar that controls `Vec`/table growth after protobuf decode, not only the examples above. - Hand-built tiny protobuf frames with maximal scalar values return typed connection-local errors without large allocation, panic, long loop, or partial shadow mutation. - Boundary tests cover exactly-at-limit and one-past-limit values. - `docs/reference/ipc.md` names these semantic limits and distinguishes them from the frame-body limit. ## Related work - #14 owns aggregate daemon admission and handshake limits. - #16 owns outbound frame and operation-specific payload limits. - This issue owns receiver-side claims whose encoded frame can remain small.
Author
Owner

Triage plan (2026-09-03)

Source-grounded triage against main at 69076d42, reviewed through seven rounds of an independent reviewer (pi sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12.

Claim check

Accurate against HEAD, and slightly worse than stated for animation frames.

  • GridDims narrows to u16 only. crates/felis-protocol/src/convert.rs:204-214: four narrow(...) calls, nothing against MAX_GRID_ROWS/MAX_GRID_COLS (2048, messages.rs:77-84) or MAX_GRID_PIXELS (32 768, :92). A 20-byte GridMsg::Size { dims: 65535×65535 } reaches ShadowScreen::apply (client-core/src/shadow.rs:395-404) → Screen::resize (felis-grid/src/screen.rs:1024-1053) → install_ring(rows, cols, …) allocating ~4.3 G cells. The same TryFrom feeds SessionInfo.dims on attach, which sizes ShadowScreen::new(attached_dims.rows, attached_dims.cols) (felis-client/src/main.rs:~893), so a hostile Ready frame has the same effect. RequestedDims (messages.rs:165-233) is correctly separate: admit refuses, clamp clamps, both over the full u32 domain.
  • Image header allocates on the wire's u64. client-core/src/image_shadow.rs:299-322: vec![0u8; usize::try_from(total_bytes).unwrap_or(0)], with the comment at :307-309 explicitly declining to re-cap ("re-capping here adds nothing"). Frame headers do the same at :219-230.
  • Frame index drives unbounded Vec growth. image_shadow.rs:231-241: while entry.frames.len() < index { push(ClientFrame { pixels: Vec::new(), complete: false }) }; index is the wire u32 (convert/image.rs:127-133 copies h.frame through). u32::MAX pushes of a 32-byte struct is ~128 GiB. The daemon has no frame-count cap either (felis-grid/src/images.rs:293 maps a byte-capacity error only), so there is no daemon constant to mirror yet.
  • No aggregate on the client. ImageShadow has no byte gauge; the daemon's DEFAULT_IMAGE_BYTE_CAP (256 MiB, pool.rs:38) and MAX_DECODED_BYTES (64 MiB, graphics/image_decode.rs:18) are daemon-private.
  • The doc claim it contradicts. docs/reference/ipc.md:477 names the anti-corruption layer; docs/reference/spec.md:157 (REQ-605a) says "The daemon normalizes once at admission"; nothing says the client validates announced dims, and security-model.md has no client-side admission bullet. The issue's reading is fair: the design says ingress validates schema-inexpressible invariants once, and the client's ingress does not.

Audit of other post-decode scalars that drive growth (what I could verify; the implementation should complete it):

Scalar Consumer Bounded today?
GridDims rows/cols/pixels Screen::resize, ShadowScreen::new No (this issue)
ImageMsg::Header.total_bytes vec![0; n] No
ImageMsg::Header.frame index frames growth No
ImageMsg::Header.width/height stored only; renderer atlas upload Indirectly by total_bytes
ImageMsg::Chunk.offset slice write Yes (:254-262, bounds-checked)
ImageMsg::ShowFrame.index index Yes (:290)
PlacementsShifted.lines saturating sub Yes
Placement/VirtualPlacement cols/rows narrow to u16, no alloc Yes
GridMsg::Hyperlink.uri/anchor LinkText::CAP Yes (shadow.rs:321-334)
GridMsg::RowDelta row index out-of-range dropped Yes (tests at shadow.rs:825-861)
SessionInfo.dims (attach Ready, List) shadow sizing No (same TryFrom)
Daemon side: InputMsg::Resize/Viewport, SpawnArgs.dims clamp / admit Yes
Daemon side: RegionMsg::Request row ranges, SearchOptions bounded by buffer size; verify max_hits Verify

Verdict

accept. Principle 3 is untouched (the client still trusts the daemon for content; it stops trusting it for allocation claims), and the "validated once at ingress" promise is the design's own. The :307-309 comment's argument ("a buggy daemon would still ship the bytes") is wrong for the reason the issue gives: the bytes never have to arrive; the claim allocates.

Approach

felis-protocol

  • messages/limits.rs (created by #16) gains the receiver-facing constants, moved out of daemon-private code so encoder and decoder share them: MAX_IMAGE_BYTES = 64 MiB (daemon MAX_DECODED_BYTES becomes an alias with the existing const _: () = assert! pattern at image_decode.rs:21), MAX_SESSION_IMAGE_BYTES = 256 MiB (pool::DEFAULT_IMAGE_BYTE_CAP aliases it), and a new MAX_IMAGE_FRAMES = 4096 enforced on both sides: the daemon's felis-grid::images frame insert refuses past it (a Kitty a=f beyond 4096 frames answers the protocol's ENOSPC-class error, consistent with the per-image byte cap), and the client rejects a header index ≥ it. Record why a count cap is needed beside the byte cap (a 4-byte frame makes the byte cap admit 16 M frames).
  • TryFrom<v1::GridDims> for GridDims (convert.rs:204) validates at wire width: rows/cols in 1..=MAX_GRID_ROWS/COLS (announced geometry is effective geometry, REQ-605a "stores and reports only the effective geometry", so zero is not a sentinel here), pixels 0 | 1..=MAX_GRID_PIXELS; failure is the existing WireError::OutOfRange { field, value }. RequestedDims stays untouched.
  • TryFrom<v1::ImageHeader>: reject total_bytes > MAX_IMAGE_BYTES, frame >= MAX_IMAGE_FRAMES, and for Rgba32 require total_bytes == width × height × 4 (checked with u64 arithmetic, no overflow); WireError::OverLimit from #16.

felis-client-core

  • ImageShadow::applyResult<(), ImageShadowError> (matching ShadowScreen::apply's shape). Before any mutation: retained + total_bytes − bytes_of(existing id being replaced) ≤ MAX_SESSION_IMAGE_BYTES, else Err(AggregateExceeded); frame header likewise counts against the aggregate. Maintain retained: usize updated on insert/replace/delete/frame insert. Validate everything, then mutate, so a refused header leaves no partial entry.
  • felis-client forward_frame (main.rs:1770-1800) already closes the connection on a refused frame (the A-7 rule); route ImageShadowError and the WireError decode failures through it, logging kind + limit + value, never the payload.
  • Delete the :307-309 comment; the new test names the reason.

felis-daemon: alias the constants; add the frame-count refusal; nothing else changes on the wire.

Tests

  • convert.rs: GridDims 2048 ok / 2049 err / 65535 err / 0 rows err / pixel 0 ok / 32 768 ok / 32 769 err, over the u32 domain (u32::MAX).
  • image_shadow.rs: total_bytes = MAX_IMAGE_BYTES accepted, +1 refused with images unchanged; frame index MAX_IMAGE_FRAMES − 1 accepted, MAX_IMAGE_FRAMES and u32::MAX refused with frames.len() unchanged; aggregate exactly at MAX_SESSION_IMAGE_BYTES accepted, one past refused; replacement of the same id does not double-count; delete releases.
  • Hand-built frames: encode v1::GridMsg/v1::ImageMsg with maximal scalars via prost, run codec::decode + apply, assert a typed error and, under a tokio::time::timeout(1 s) or a custom global-allocator counter in the test crate, no allocation over 1 MiB (the allocator-counter is the precise check; test-strategy skill's proptest layer can then fuzz random scalars against "never allocates proportional to the claim").
  • Kani candidate (per the skill's heuristic): the GridDims validation is a pure bounded-integer function; a harness that every u32 quadruple either fails or yields values within the bounds is cheap.

Docs cascade

  • docs/reference/ipc.md: the "Semantic limits" table from #16 gains a "checked by receiver" column and the image/geometry rows; a short paragraph distinguishing frame-body bytes from claimed allocation; "Session"'s GridDims description says announced dims are validated on receipt.
  • docs/reference/spec.md: REQ-605a gains "a receiver validates announced geometry against the same bounds before sizing any grid"; REQ-1008 says the two image caps are protocol-visible constants and adds the frame-count cap; docs/reference/protocols/kitty-graphics.md status table gains the frame-count limit.
  • docs/explanation/architecture/ipc.md: the anti-corruption paragraph states the rule explicitly: every scalar that controls allocation or indexed growth is admitted in TryFrom<v1::*>, and the rejected alternative (trust the daemon, the :307 stance) with why.
  • docs/explanation/security-model.md "Daemon IPC": a "Client-side admission" bullet.
  • docs/explanation/protocols/kitty-graphics.md: the frame-count decision.
  • CHANGELOG.md: Kitty animations over 4096 frames are refused (user-affecting); client closes on out-of-bound announced geometry/image claims.
  • skills/felis: none.

Dependencies

  • #16 first (shared limits.rs, WireError::OverLimit). #51 (canonical image transfer state machine) should land beside this per #52's order 3, since the header-validation code is where #51's Header → Chunk* → Complete enforcement will sit; do #49 first so #51 builds on validated headers. #30 after. Independent of #13/#14/#15.

Risk/effort

M. Main risk: the frame-count cap is a new daemon behavior on a real producer path (Kitty animations); 4096 is far above anything surveyed (kitty's own tests use tens of frames) but the number must be recorded as a decision with a revisit trigger. Second risk: the total_bytes == w×h×4 check must match exactly what graphics.rs:156/203 emits for every stored format (only Rgba32 reaches the wire today per image_shadow.rs:39-41; assert that in the test).

Labels

Keep priority/P0, release/v0.1.0. A 20-byte frame from a same-UID peer, or a compromised remote daemon over the SSH relay, that takes down the GUI client is exactly the class the freeze must close, and the fix is small once #16's module exists.

## Triage plan (2026-09-03) Source-grounded triage against `main` at `69076d42`, reviewed through seven rounds of an independent reviewer (`pi` sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. ## Claim check Accurate against HEAD, and slightly worse than stated for animation frames. - **`GridDims` narrows to `u16` only.** `crates/felis-protocol/src/convert.rs:204-214`: four `narrow(...)` calls, nothing against `MAX_GRID_ROWS`/`MAX_GRID_COLS` (2048, `messages.rs:77-84`) or `MAX_GRID_PIXELS` (32 768, `:92`). A 20-byte `GridMsg::Size { dims: 65535×65535 }` reaches `ShadowScreen::apply` (`client-core/src/shadow.rs:395-404`) → `Screen::resize` (`felis-grid/src/screen.rs:1024-1053`) → `install_ring(rows, cols, …)` allocating ~4.3 G cells. The same `TryFrom` feeds `SessionInfo.dims` on attach, which sizes `ShadowScreen::new(attached_dims.rows, attached_dims.cols)` (`felis-client/src/main.rs:~893`), so a hostile `Ready` frame has the same effect. `RequestedDims` (`messages.rs:165-233`) is correctly separate: `admit` refuses, `clamp` clamps, both over the full `u32` domain. - **Image header allocates on the wire's `u64`.** `client-core/src/image_shadow.rs:299-322`: `vec![0u8; usize::try_from(total_bytes).unwrap_or(0)]`, with the comment at `:307-309` explicitly declining to re-cap ("re-capping here adds nothing"). Frame headers do the same at `:219-230`. - **Frame index drives unbounded `Vec` growth.** `image_shadow.rs:231-241`: `while entry.frames.len() < index { push(ClientFrame { pixels: Vec::new(), complete: false }) }`; `index` is the wire `u32` (`convert/image.rs:127-133` copies `h.frame` through). `u32::MAX` pushes of a 32-byte struct is ~128 GiB. The daemon has no frame-*count* cap either (`felis-grid/src/images.rs:293` maps a byte-capacity error only), so there is no daemon constant to mirror yet. - **No aggregate on the client.** `ImageShadow` has no byte gauge; the daemon's `DEFAULT_IMAGE_BYTE_CAP` (256 MiB, `pool.rs:38`) and `MAX_DECODED_BYTES` (64 MiB, `graphics/image_decode.rs:18`) are daemon-private. - **The doc claim it contradicts.** `docs/reference/ipc.md:477` names the anti-corruption layer; `docs/reference/spec.md:157` (REQ-605a) says "The daemon normalizes once at admission"; nothing says the client validates announced dims, and `security-model.md` has no client-side admission bullet. The issue's reading is fair: the design says ingress validates schema-inexpressible invariants once, and the client's ingress does not. **Audit of other post-decode scalars that drive growth** (what I could verify; the implementation should complete it): | Scalar | Consumer | Bounded today? | |---|---|---| | `GridDims` rows/cols/pixels | `Screen::resize`, `ShadowScreen::new` | No (this issue) | | `ImageMsg::Header.total_bytes` | `vec![0; n]` | No | | `ImageMsg::Header.frame` index | `frames` growth | No | | `ImageMsg::Header.width/height` | stored only; renderer atlas upload | Indirectly by `total_bytes` | | `ImageMsg::Chunk.offset` | slice write | Yes (`:254-262`, bounds-checked) | | `ImageMsg::ShowFrame.index` | index | Yes (`:290`) | | `PlacementsShifted.lines` | saturating sub | Yes | | `Placement/VirtualPlacement cols/rows` | `narrow` to u16, no alloc | Yes | | `GridMsg::Hyperlink.uri/anchor` | `LinkText::CAP` | Yes (`shadow.rs:321-334`) | | `GridMsg::RowDelta` row index | out-of-range dropped | Yes (tests at `shadow.rs:825-861`) | | `SessionInfo.dims` (attach `Ready`, `List`) | shadow sizing | No (same `TryFrom`) | | Daemon side: `InputMsg::Resize/Viewport`, `SpawnArgs.dims` | clamp / admit | Yes | | Daemon side: `RegionMsg::Request` row ranges, `SearchOptions` | bounded by buffer size; verify `max_hits` | Verify | ## Verdict **accept.** Principle 3 is untouched (the client still trusts the daemon for *content*; it stops trusting it for *allocation claims*), and the "validated once at ingress" promise is the design's own. The `:307-309` comment's argument ("a buggy daemon would still ship the bytes") is wrong for the reason the issue gives: the bytes never have to arrive; the *claim* allocates. ## Approach **felis-protocol** - `messages/limits.rs` (created by #16) gains the receiver-facing constants, moved out of daemon-private code so encoder and decoder share them: `MAX_IMAGE_BYTES = 64 MiB` (daemon `MAX_DECODED_BYTES` becomes an alias with the existing `const _: () = assert!` pattern at `image_decode.rs:21`), `MAX_SESSION_IMAGE_BYTES = 256 MiB` (`pool::DEFAULT_IMAGE_BYTE_CAP` aliases it), and a new `MAX_IMAGE_FRAMES = 4096` enforced on **both** sides: the daemon's `felis-grid::images` frame insert refuses past it (a Kitty `a=f` beyond 4096 frames answers the protocol's `ENOSPC`-class error, consistent with the per-image byte cap), and the client rejects a header index ≥ it. Record why a count cap is needed beside the byte cap (a 4-byte frame makes the byte cap admit 16 M frames). - `TryFrom<v1::GridDims> for GridDims` (`convert.rs:204`) validates at wire width: rows/cols in `1..=MAX_GRID_ROWS/COLS` (announced geometry is effective geometry, REQ-605a "stores and reports only the effective geometry", so zero is not a sentinel here), pixels `0 | 1..=MAX_GRID_PIXELS`; failure is the existing `WireError::OutOfRange { field, value }`. `RequestedDims` stays untouched. - `TryFrom<v1::ImageHeader>`: reject `total_bytes > MAX_IMAGE_BYTES`, `frame >= MAX_IMAGE_FRAMES`, and for `Rgba32` require `total_bytes == width × height × 4` (checked with `u64` arithmetic, no overflow); `WireError::OverLimit` from #16. **felis-client-core** - `ImageShadow::apply` → `Result<(), ImageShadowError>` (matching `ShadowScreen::apply`'s shape). Before any mutation: `retained + total_bytes − bytes_of(existing id being replaced) ≤ MAX_SESSION_IMAGE_BYTES`, else `Err(AggregateExceeded)`; frame header likewise counts against the aggregate. Maintain `retained: usize` updated on insert/replace/delete/frame insert. Validate everything, then mutate, so a refused header leaves no partial entry. - `felis-client` `forward_frame` (`main.rs:1770-1800`) already closes the connection on a refused frame (the A-7 rule); route `ImageShadowError` and the `WireError` decode failures through it, logging kind + limit + value, never the payload. - Delete the `:307-309` comment; the new test names the reason. **felis-daemon**: alias the constants; add the frame-count refusal; nothing else changes on the wire. **Tests** - `convert.rs`: `GridDims` 2048 ok / 2049 err / 65535 err / 0 rows err / pixel 0 ok / 32 768 ok / 32 769 err, over the `u32` domain (`u32::MAX`). - `image_shadow.rs`: `total_bytes = MAX_IMAGE_BYTES` accepted, `+1` refused with `images` unchanged; frame index `MAX_IMAGE_FRAMES − 1` accepted, `MAX_IMAGE_FRAMES` and `u32::MAX` refused with `frames.len()` unchanged; aggregate exactly at `MAX_SESSION_IMAGE_BYTES` accepted, one past refused; replacement of the same id does not double-count; delete releases. - Hand-built frames: encode `v1::GridMsg`/`v1::ImageMsg` with maximal scalars via prost, run `codec::decode` + `apply`, assert a typed error and, under a `tokio::time::timeout(1 s)` or a custom global-allocator counter in the test crate, no allocation over 1 MiB (the allocator-counter is the precise check; `test-strategy` skill's proptest layer can then fuzz random scalars against "never allocates proportional to the claim"). - Kani candidate (per the skill's heuristic): the `GridDims` validation is a pure bounded-integer function; a harness that every `u32` quadruple either fails or yields values within the bounds is cheap. **Docs cascade** - `docs/reference/ipc.md`: the "Semantic limits" table from #16 gains a "checked by receiver" column and the image/geometry rows; a short paragraph distinguishing frame-body bytes from claimed allocation; "Session"'s `GridDims` description says announced dims are validated on receipt. - `docs/reference/spec.md`: REQ-605a gains "a receiver validates announced geometry against the same bounds before sizing any grid"; REQ-1008 says the two image caps are protocol-visible constants and adds the frame-count cap; `docs/reference/protocols/kitty-graphics.md` status table gains the frame-count limit. - `docs/explanation/architecture/ipc.md`: the anti-corruption paragraph states the rule explicitly: every scalar that controls allocation or indexed growth is admitted in `TryFrom<v1::*>`, and the rejected alternative (trust the daemon, the `:307` stance) with why. - `docs/explanation/security-model.md` "Daemon IPC": a "Client-side admission" bullet. - `docs/explanation/protocols/kitty-graphics.md`: the frame-count decision. - `CHANGELOG.md`: Kitty animations over 4096 frames are refused (user-affecting); client closes on out-of-bound announced geometry/image claims. - `skills/felis`: none. ## Dependencies - **#16 first** (shared `limits.rs`, `WireError::OverLimit`). **#51** (canonical image transfer state machine) should land beside this per #52's order 3, since the header-validation code is where #51's `Header → Chunk* → Complete` enforcement will sit; do #49 first so #51 builds on validated headers. **#30** after. Independent of #13/#14/#15. ## Risk/effort **M.** Main risk: the frame-count cap is a new *daemon* behavior on a real producer path (Kitty animations); 4096 is far above anything surveyed (kitty's own tests use tens of frames) but the number must be recorded as a decision with a revisit trigger. Second risk: the `total_bytes == w×h×4` check must match exactly what `graphics.rs:156/203` emits for every stored format (only `Rgba32` reaches the wire today per `image_shadow.rs:39-41`; assert that in the test). ## Labels Keep `priority/P0`, `release/v0.1.0`. A 20-byte frame from a same-UID peer, or a compromised remote daemon over the SSH relay, that takes down the GUI client is exactly the class the freeze must close, and the fix is small once #16's module exists.
Sign in to join this conversation.
No description provided.