[v0.1/P0] Bound receiver allocations from protobuf scalar claims #49
Labels
No labels
priority/P0
priority/P1
priority/P2
release/v0.1.0
status/blocked
status/planned
type/bug
type/design
type/test-gap
type/tracker
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
natsukium/felis#49
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 GridDimsonly narrows each axis tou16. It accepts65535 × 65535, althoughGridDimsis documented as already admitted and the supported row/column maximum is 2048.GridMsg::Sizethen resizes the client shadow from that tiny frame.ImageMsg::Header.total_bytesis au64copied directly intovec![0; capacity]inImageShadowwithout the daemon's 64 MiB per-image cap.ImageShadow::apply_frame_headerfills every missing slot up to the wire-suppliedu32frame index, soSome(u32::MAX)can consume the process from a small frame.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
GridDimsaccept only admitted row/column bounds and valid pixel sentinels/ranges. Requested geometry remains the separateRequestedDimspath because create refusal and resize clamping have different semantics.MAX_DECODED_BYTEScomment.Vec/table growth after protobuf decode, not only the examples above.docs/reference/ipc.mdnames these semantic limits and distinguishes them from the frame-body limit.Related work
Triage plan (2026-09-03)
Source-grounded triage against
mainat69076d42, reviewed through seven rounds of an independent reviewer (pisol/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.
GridDimsnarrows tou16only.crates/felis-protocol/src/convert.rs:204-214: fournarrow(...)calls, nothing againstMAX_GRID_ROWS/MAX_GRID_COLS(2048,messages.rs:77-84) orMAX_GRID_PIXELS(32 768,:92). A 20-byteGridMsg::Size { dims: 65535×65535 }reachesShadowScreen::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 sameTryFromfeedsSessionInfo.dimson attach, which sizesShadowScreen::new(attached_dims.rows, attached_dims.cols)(felis-client/src/main.rs:~893), so a hostileReadyframe has the same effect.RequestedDims(messages.rs:165-233) is correctly separate:admitrefuses,clampclamps, both over the fullu32domain.u64.client-core/src/image_shadow.rs:299-322:vec![0u8; usize::try_from(total_bytes).unwrap_or(0)], with the comment at:307-309explicitly declining to re-cap ("re-capping here adds nothing"). Frame headers do the same at:219-230.Vecgrowth.image_shadow.rs:231-241:while entry.frames.len() < index { push(ClientFrame { pixels: Vec::new(), complete: false }) };indexis the wireu32(convert/image.rs:127-133copiesh.framethrough).u32::MAXpushes of a 32-byte struct is ~128 GiB. The daemon has no frame-count cap either (felis-grid/src/images.rs:293maps a byte-capacity error only), so there is no daemon constant to mirror yet.ImageShadowhas no byte gauge; the daemon'sDEFAULT_IMAGE_BYTE_CAP(256 MiB,pool.rs:38) andMAX_DECODED_BYTES(64 MiB,graphics/image_decode.rs:18) are daemon-private.docs/reference/ipc.md:477names 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, andsecurity-model.mdhas 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):
GridDimsrows/cols/pixelsScreen::resize,ShadowScreen::newImageMsg::Header.total_bytesvec![0; n]ImageMsg::Header.frameindexframesgrowthImageMsg::Header.width/heighttotal_bytesImageMsg::Chunk.offset:254-262, bounds-checked)ImageMsg::ShowFrame.index:290)PlacementsShifted.linesPlacement/VirtualPlacement cols/rowsnarrowto u16, no allocGridMsg::Hyperlink.uri/anchorLinkText::CAPshadow.rs:321-334)GridMsg::RowDeltarow indexshadow.rs:825-861)SessionInfo.dims(attachReady,List)TryFrom)InputMsg::Resize/Viewport,SpawnArgs.dimsRegionMsg::Requestrow ranges,SearchOptionsmax_hitsVerdict
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-309comment'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(daemonMAX_DECODED_BYTESbecomes an alias with the existingconst _: () = assert!pattern atimage_decode.rs:21),MAX_SESSION_IMAGE_BYTES = 256 MiB(pool::DEFAULT_IMAGE_BYTE_CAPaliases it), and a newMAX_IMAGE_FRAMES = 4096enforced on both sides: the daemon'sfelis-grid::imagesframe insert refuses past it (a Kittya=fbeyond 4096 frames answers the protocol'sENOSPC-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 in1..=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), pixels0 | 1..=MAX_GRID_PIXELS; failure is the existingWireError::OutOfRange { field, value }.RequestedDimsstays untouched.TryFrom<v1::ImageHeader>: rejecttotal_bytes > MAX_IMAGE_BYTES,frame >= MAX_IMAGE_FRAMES, and forRgba32requiretotal_bytes == width × height × 4(checked withu64arithmetic, no overflow);WireError::OverLimitfrom #16.felis-client-core
ImageShadow::apply→Result<(), ImageShadowError>(matchingShadowScreen::apply's shape). Before any mutation:retained + total_bytes − bytes_of(existing id being replaced) ≤ MAX_SESSION_IMAGE_BYTES, elseErr(AggregateExceeded); frame header likewise counts against the aggregate. Maintainretained: usizeupdated on insert/replace/delete/frame insert. Validate everything, then mutate, so a refused header leaves no partial entry.felis-clientforward_frame(main.rs:1770-1800) already closes the connection on a refused frame (the A-7 rule); routeImageShadowErrorand theWireErrordecode failures through it, logging kind + limit + value, never the payload.:307-309comment; 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:GridDims2048 ok / 2049 err / 65535 err / 0 rows err / pixel 0 ok / 32 768 ok / 32 769 err, over theu32domain (u32::MAX).image_shadow.rs:total_bytes = MAX_IMAGE_BYTESaccepted,+1refused withimagesunchanged; frame indexMAX_IMAGE_FRAMES − 1accepted,MAX_IMAGE_FRAMESandu32::MAXrefused withframes.len()unchanged; aggregate exactly atMAX_SESSION_IMAGE_BYTESaccepted, one past refused; replacement of the same id does not double-count; delete releases.v1::GridMsg/v1::ImageMsgwith maximal scalars via prost, runcodec::decode+apply, assert a typed error and, under atokio::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-strategyskill's proptest layer can then fuzz random scalars against "never allocates proportional to the claim").GridDimsvalidation is a pure bounded-integer function; a harness that everyu32quadruple 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"'sGridDimsdescription 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.mdstatus 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 inTryFrom<v1::*>, and the rejected alternative (trust the daemon, the:307stance) 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
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'sHeader → Chunk* → Completeenforcement 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×4check must match exactly whatgraphics.rs:156/203emits for every stored format (onlyRgba32reaches the wire today perimage_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.