Clipboard copied once into a region the compositor made; copy-once is a type, the compositor forbids unsafe code - #557
Conversation
…ndle is ever used
The compositor took MSG_CLIPBOARD_SET_SHM's region from the client: any handle
the client chose went to shm_map, and a pipe there is WrongType, which the
kernel answers by ending the caller (exit 139). And it validated the region as
UTF-8 in place and then copied it, while the client could still write it.
Now the compositor never receives a handle from a client. A copy past the
inline limit is MSG_COPY_BEGIN{len}, answered with MSG_COPY_REGION and a
region the compositor made; the client writes it and sends MSG_COPY_COMMIT;
the compositor reads every byte once through AtomicU8, validates that copy
with String::from_utf8, drops the region and closes the connection. A copy
never committed is dropped at HANDSHAKE_TIMEOUT by name. Message 10 is
retired and refused by name. The inline clipboard goes through the same
validation instead of from_utf8_lossy.
The compositor denies clippy::undocumented_unsafe_blocks. The key read is a
byte buffer decoded as KeyEvent through IpcPayload instead of a byte view of
an event array; the cursor upload writes the mapping's slice instead of a raw
pointer; the one unsafe block left in render.rs carries its SAFETY clause.
compositor_hostile_clipboard is the guest test: a pipe on the retired message,
a region rewritten while it is read, a region rewritten after its commit, and
a copy never committed. compositor_client_death's two region cases move to the
new protocol.
issues/isolation/a-received-handle-has-no-knowable-type.md loses the two
bullets that said a hostile client reaches only a closed instance; blockd is
one it reaches. Filed: the compositor ignores unknown message types, a
window's buffer is read while its client writes it, and no gate runs clippy
over the compositor.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review, round 1, of PR #557 at cc3e0c2 (merge base 1808fb8). Gate state: the Net: production +256/−113 (+143), tests +398/−27 (+371), issues +62/−12 (+50). BLOCKER
NOTE
REMOVE
Required guest runs (orchestrator, at the fixed head)
m2 is a compile failure once SEND BACK |
…ve closes its handle - copy_begin moves the region's handle with handle_send on its own and closes it when the move is refused, then sends MSG_COPY_REGION bare. A client that begins a copy and closes before the answer used to cost the compositor one handle slot and one region per try: the kernel restores a refused batch at its own numbers, and nothing closed it. A handle is never closed after a successful move. No guest can trigger this deterministically, since it is the client's close racing the send. The split is the fix. The same leak in deliver_with_handles (window creation, paste, resize) is filed. - CopyRegion (client.rs) wraps the compositor's copy region. Its one method, take(self), reads every byte once through SharedMemory::as_atomic. Pending connections and frames carry Option<CopyRegion>, and copy_out is gone. Reading the region in place or reading it a second time no longer compiles. - The copy state is decided in one match on (frame.copy, msg_type): a connection holding a region may send its commit and nothing else, and a commit comes only on such a connection. Deleting the refusal arm makes the match non-exhaustive. - MSG_COPY_BEGIN's payload must be exactly one ClipboardShmMsg. Trailing bytes drop the client as out of protocol. - MSG_RETIRED_CLIPBOARD_SET_SHM drops its client with its own reason, DropReason::Retired, so the host can tell that refusal from every other out-of-protocol drop. - The compositor is #![forbid(unsafe_code)]. The window blit reads SharedMemory::as_slice, and the copy reads SharedMemory::as_atomic, added in toyos/src/shm.rs. The clippy deny and its issue are deleted. - toyos-window: CopyError keeps only what is its own, TooLong and Compositor(CreateError). CreateError reports CompositorGone only for a closed port. It reports ConnectRefused for a kernel refusal, BrokenOff for a connection that failed mid-exchange (the compositor exited, or refused by closing), Protocol for an answer that came without its region, and Unmappable for a region that would not map. - terminal and editor report a refused copy instead of discarding it, and an editor cut whose copy was refused keeps its text. - compositor_hostile_clipboard: the rewritten-while-read case is replaced by a copy of 0xFF bytes, whose paste must be the clipboard from before and whose refusal the host requires by name. The after-commit case overwrites with text, so a read at paste would be pasted. New cases: a second MSG_COPY_BEGIN on a copy connection, and a begin with trailing bytes. Each must end in a hangup. - Filed: issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md. netd is added to a-received-handle-has-no-knowable-type.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review, round 2, of PR #557 at c5d09bb (merge base e5ffe95). Changes since round 1 are reviewed as cc3e0c2..28db806; c5d09bb brings in only main's toyos-net-tcp and toyos-net-wire. Gate state. This head is not yet ready for review under
Net: production +332/−139 (+193), tests +379/−27 (+352), issues +80/−12 (+68). Round-1 BLOCKERs
Rulings asked for
BLOCKERNone open. NOTE
REMOVE
LAND AFTER NAMED CHANGES |
…keeps the kernel's word The compositor: - A refused region move drops the client as `DropReason::Gone`, written directly rather than built from a send error. - `deliver_with_handles` loses the sentence saying its handles move whether or not the frame lands, which is false for a refused `handle_send`. `toyos-window`'s `CreateError`: - `ConnectRefused` and `Unmappable` become one `Kernel(SyscallError)`. Nothing distinguished them but the call that was refused, and an `IpcError::Syscall` now lands there too instead of being discarded as `BrokenOff`. - An answer without its handle is `NoHandle`, not `Protocol` of the type that was right. Its doc names both causes, since `recv_handles_exact` also answers `None` when this process has no room for the handle. - `IpcError::TooLarge` is unreachable: every frame this crate sends is within `MAX_FRAME_LEN`, and a const assertion holds `MAX_INLINE_PAYLOAD` to it. - `Protocol`'s message no longer says the type was wrong, because a payload on `MSG_COPY_REGION` is the right type in the wrong shape. Tests: - `compositor_hostile_clipboard` gains two cases. A commit with a payload over a region of `E`s must leave the clipboard as it was. A commit on a window's connection must lose the window its connection. - `window_refusal` gains readers for `BrokenOff` (the stand-in closes unanswered), `NoHandle` (`MSG_WINDOW_CREATED` with no buffer), and `Kernel` (a port queue filled until the kernel refuses the connection). Filed: - `issues/isolation/sharedmemory-slices-rest-on-a-size-nobody-checks.md` - `issues/design-debt/decode-payload-accepts-bytes-past-its-type.md` - `issues/design-debt/a-client-waits-on-the-compositors-answer-with-no-bound.md` Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in #561: the host hands out no guest slots and no build slots. It touches tests/toyos.rs apart from this branch's hunks, and the merge is clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mes it compositor_hostile_clipboard loses its 30 s ceiling, its 2 s paste re-mark and all three Instant deadlines. A hang-up is a blocking read that answers 0, the region and the probe are blocking header reads, a paste and a window's close are the window's blocking recv_event. The paste marker is printed once, so each GUI+V the host types is one paste and no stale-paste skip is needed. compositor_client_death's probe loses its 500 x 10 ms sleep-poll, and the window-closed-from-the-inside case its eight 2 s polls: it waits for Close with no timeout, then asks once more, which a latched window answers None at once and an unlatched one answers Close at once. Every wait prints what it waits for first, so an event that never comes is the harness's ceiling with the missing event on the guest's last line. window_refusal had no clock; both its sides now name their waits too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
The stand-in serves one request per case. A client that panicked at a wrong answer left the stand-in blocked in accept on the next case, so a decoding red was a harness ceiling instead of the client's assertion. Now every answer is collected first and judged after, so a wrong one reds as the client's panic and the server's reap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
|
Review, round 3, of PR #557 at a69ee8e (merge base 1ec6daa). Changes since round 2 (c5d09bb) are reviewed as c6613b4, 2a5c94c and a69ee8e. The merges 74435f1 and da3271f bring in only main. Gate state.
Net: production +197 (340/−143), tests +388 (491/−103), issues +132 (144/−12). The production growth is accepted: a message that let any GUI client end the desktop becomes a protocol in which the compositor makes the region, plus typed errors. Earlier BLOCKERs
What the brief asked
BLOCKER
NOTE
REMOVE
SEND BACK |
…itor that answers the commit instead of dropping the window reds instead of passing: a fresh MSG_GET_RESOLUTION on the same connection is answered only if the connection survives, and await_hangup tells that apart from a drop. File the compositor's own mapping of a committed region outliving the client's handle, with its owner and exit, and give the two prior new issues an owner line in the form the others use. Replace the atomic cast the test duplicated with SharedMemory::as_atomic, and give the second poll_event its waiting line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
cargo test --lib reds once on buildlock::tests::a_key_being_built_is_waited_for_and_another_key_is_not under a loaded host and passes alone straight after; unrelated to this branch's compositor work. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
…ly gate cargo run -- --build-only and cargo test --test toyos-build -- --list both fail after this branch's merge moved rust/'s pin, reproducing with ./x alone outside this repository's wrapper; a hand-run cargo build with the same command line and env succeeds every time, so the fault is in the stale build-dir x drives, not in the crates or this branch's sources. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
|
Review, round 4, of PR #557 at 70156f1 (merge base 51fe14c). Changes since round 3 (a69ee8e) are reviewed as 10265bb, 2591a1d and 70156f1. The merge e039fe6 is clean: its diff against its main-side parent is exactly the branch's own 20 paths. Gate state.
Net against Earlier BLOCKERs
Round 3 NOTEs and REMOVEs
What the brief asked
BLOCKERNone. NOTE
REMOVE
LAND AFTER NAMED CHANGES |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
…aky buildlock test, and cut a narrating comment The bootstrap build-cache failure was this worktree's environment, not the tree's, per the orchestrator's 17 successful `cargo test --test toyos-build` runs at this head; the file is deleted rather than kept as a false claim. The buildlock flake is the orchestrator's to disable, not this branch's, so it gets an owner line and loses the prescribed fix it has no standing to choose. `tests/toyos.rs`'s clipboard test comment loses the line that only restates the test's name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
origin/main brought #557 (clipboard fix), which registers metal_sim_hostile_clipboard at Fast beside the metal_sim_window_drag row this branch moved to Weekly. Kept #564's move and #557's own row and tier for the new test, per this branch's stated rule that a row main added keeps main's tier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6rME2DoqwjcYFStYHHY4j
The compositor no longer receives a handle from a client. The type system rules out reading a copy's region anywhere except once, at the commit, and the compositor crate forbids
unsafecode.What changed, and why
The compositor never uses a handle a client sent.
MSG_CLIPBOARD_SET_SHM(10) gave the compositor a handle the client chose.SharedMemory::adoptmapped it, and for a pipe the kernel answersWrongTypeby ending the caller (exit 139), so any GUI client could end the desktop. Message 10 is retired. A client that sends it is dropped with its own reason,DropReason::Retired, and the compositor callshandle_recvnowhere. A handle a client sends stays queued until its connection closes, and then goes back to the kernel unused.The compositor makes the region. A copy past
MAX_INLINE_PAYLOADopens its connection withMSG_COPY_BEGIN, whose payload is exactly oneClipboardShmMsg. Trailing bytes drop the client as out of protocol. The compositor moves the region's handle and then sends a bareMSG_COPY_REGION. The client writes the text and sends a bareMSG_COPY_COMMIT, and the compositor closes the connection. A commit with a payload is refused and the region's text is never read.window::copy_fitsis the one bound, and both ends read it.A refused move closes its handle.
copy_begincallssyscall::handle_sendon its own. When the kernel refuses the move, it restores the handle at its number, and the compositor closes it and drops the client asDropReason::Gone. The compositor never closes a handle after a successful move, because the slot has been reissued. No guest can trigger the refused move deterministically: it needs the client's close to race the compositor's send. The proof is the split itself.deliver_with_handleshas the same leak (window creation, paste, resize), which is filed.Copy-once is a type.
CopyRegioninclient.rshas a private field and one method,take(self) -> Vec<u8>, which reads every byte once throughSharedMemory::as_atomic. Pending connections and frames holdOption<CopyRegion>. Reading the region in place does not compile (E0616), and neither does reading it at a paste without consuming it (E0507).set_clipboardvalidates the copy withString::from_utf8and refuses non-UTF-8 by name. The inlineMSG_CLIPBOARD_SETgoes through the same function. The compositor's own mapping of a region it took outlives the client's own handle to it, which is filed with its owner and exit condition.The copy state is one match.
dispatchmatches on(frame.copy, msg_type). A connection holding a region may send its commit and nothing else, and a commit is only accepted from such a connection; on any other connection, a window's included, it drops the client. If the refusal arm is deleted, the match is no longer exhaustive (E0004).#![forbid(unsafe_code)]on the compositor. The window blit readsSharedMemory::as_slice(). The copy readsSharedMemory::as_atomic(), which is new intoyos/src/shm.rsnext toas_slice. The key read decodes each event throughipc::decode_payload, and a torn read asserts. The cursor upload writes through the mapping's slice. Both slices rest onadopt's unchecked size, which is filed.toyos-window.CopyErrorhas two variants:TooLong, andCompositor(CreateError)for everything the exchange shares with window creation.CreateErrorreportsCompositorGoneonly when the port is closed. The other failures each have their own variant:Kernel(e): the kernel refused a call of the exchange (the connection, a frame, or the region's map), with its error.BrokenOff: the compositor closed the connection, having exited or refused, or sent a frame this client cannot read.NoHandle: the answer came without its handle. The compositor sent none, or this process had no room to take it;recv_handles_exactanswersNonefor both.Protocol(t): an answer this exchange does not allow. Its message names the type without claiming the type was the wrong part.IpcError::TooLargeisunreachable!: every frame this crate sends is withinMAX_FRAME_LEN, and a const assertion holdsMAX_INLINE_PAYLOADto it.terminal and editor log a refused copy instead of discarding it. An editor cut whose copy was refused keeps its text.
issues/isolation/a-received-handle-has-no-knowable-type.mdloses its two bullets saying that a hostile client reaches only a closed instance. It gains two instances a client can reach: blockd'sRegion::adopt, and netd's piped socket and bind.Tests. No guest wait in this PR has a clock: no deadline, no liveness guard, no must-not window and no count over time. Each wait blocks on its event (the connection's hang-up, the reply, the paste, the window's close) and prints
waiting for <event>first. An event that never comes reds at the harness's ceiling, and the guest's last line names it. Where a case needs nothing to happen, a later ordered event proves it: the probe the compositor answers after the refusal, or the paste.compositor_hostile_clipboard(new) is registered asmetal_sim_hostile_clipboard, inTier::Fast. Its cases:0xFFbytes; the paste must be the clipboard from before it.Cs, overwritten withDs after its commit; the paste must be theCs.MSG_COPY_BEGINon a copy connection.Es; the paste must be the inline clipboard set before it.await_hangupreads the connection to EOF rather than watching for aCloseevent, so a compositor that answered the commit instead of dropping the window reds here instead of passing.Cases 5 and 6 must end in a hangup, and bytes where the hangup was due red at once. The guest prints its paste marker once per paste and the host types GUI+V once per marker, so each marker is exactly one paste. The host reds on any
handle fault:orexit: compositorrecord from the kernel, and it requires the named refusals of cases 1, 2 and 4.compositor_client_death's two region cases use the new protocol: a commit with no copy begun, and a copy ofu32::MAXbytes. The host requires the named refusal of the second. Its probe loses its 500 polls of 10 ms and blocks on the answer. The window closed from the inside loses its eight polls of 2 s: it waits forClosewith no timeout and then polls once more, which a latched window answersNoneat once and an unlatched one answersCloseat once.window_refusal's stand-in compositor gains three answers: a close with no answer (BrokenOff),MSG_WINDOW_CREATEDwith no buffer (NoHandle), and a port queue the client fills until the kernel refuses the connection (Kernelwith the kernel's own error). The client makes every request before it judges any, so a wrong answer reds as its assertion instead of leaving the stand-in blocked inaccepton the next case.Gates (host)
cargo test --libcargo test --workspace --exclude toyos-buildcargo run -- --clippycargo test --test toyos-build -- --listcargo run -- --build-onlyArms that must not compile
Measured at 74435f1. No file they touch has changed since (
git diff --stat 74435f1d 70156f1b -- userland/compositor userland/toyos-window toyos/src/shm.rsis empty). Each patch was applied as a checked patch. The tree was built withcargo run -- --build-onlyand then restored clean.region.0.as_slice())field 0 of struct CopyRegion is privatecannot move out of *r which is behind a shared reference(Some(_), 0_u32..=13_u32)and(Some(_), 15_u32..=u32::MAX)not coveredvariant Retired is never constructedGuest runs: run by the orchestrator
Every run applies its patch with
git apply, runs its command, and reverts withgit apply -R. Control v3 is the whole change reverted onto the base it merges,git diff HEAD 51fe14c1 --over its nine paths:tests/toyos-rust-tests/src/bin/compositor_client_death.rs,tests/toyos-rust-tests/src/bin/window_refusal.rs,userland/compositor/src/client.rs,userland/compositor/src/main.rs,userland/compositor/src/render.rs,userland/compositor/src/session.rs,userland/editor/src/main.rs,userland/terminal/src/main.rs,userland/toyos-window/src/lib.rs. It leaves outtoyos/src/shm.rs's pure addition — auseplusas_atomic, a new method that changes no existing item and that only the kept test calls on the reverted tree.Measured at 70156f1, from
orch-runs/557r5-*.log,557r6-fast.log,557r7-control.logandsummary.txt:359-375, 411-412, 485.cargo test --test toyos-build -- metal_sim_hostile_clipboardPASS metal_sim_hostile_clipboard (4s)cargo test --test toyos-build -- window_refusalPASS window_refusal (54ms)cargo test --test toyos-build -- --nightly metal_sim_client_deathPASS metal_sim_client_death (7s)metal_sim_hostile_clipboardthe kernel ended the compositor: [kernel 0.935 cpu0] handle fault: pid=4 tid=0 syscall=106 a PipeWrite where the call takes a SharedMemmetal_sim_hostile_clipboardthe client that sent a pipe where a region went was not refused by nameCopyRegionand takes it at the pastemetal_sim_hostile_clipboard[a region rewritten after its commit] the paste was 2097152 bytes, UTF-8: true, first byte that is not 'C' at Some(0)HandshakeTimeoutmetal_sim_hostile_clipboardthe client that held its region and never committed was not dropped by namecopy_fitsbound removed--nightly metal_sim_client_deatha copy longer than any clipboard was not refused by namemetal_sim_hostile_clipboard[a second begin on a copy] the peer answered where the compositor's refusal was dueset_clipboardmetal_sim_hostile_clipboard[a copy that is not UTF-8] the paste was 6291456 bytes, not the clipboard from beforemetal_sim_hostile_clipboard[a begin with bytes past its length] the peer answered where the compositor's refusal was duecopy_commit's bare-payload check deletedmetal_sim_hostile_clipboard[a commit with a payload] the paste was 2097152 bytes, not the clipboard from beforemetal_sim_hostile_clipboard[a commit on a window] the peer answered where the compositor closing the window was dueDisconnectedmapped toCompositorGonewindow_refusalreply Close decoded wrongly(leftCompositorGone, rightBrokenOff), thenthe client did not survive the refusalsProtocol(MSG_WINDOW_CREATED)window_refusalreply NoBuffer decoded wrongly(leftProtocol(1), rightNoHandle), thenthe client did not survive the refusalsEndowError::Refused(_) => Self::CompositorGonewindow_refusala full queue decoded wrongly(leftSome(CompositorGone)), thenthe client did not survive the refusalscargo test --test toyos-buildlog_ring_keeps_the_owners_slots, disabled onorigin/mainat #569, unrelated: this branch touches no logd or kernel log pathHigh risk: the two checks
handle fault:andexit: compositorrecords, whatever the compositor believed it did. The pipe's read end hangs up only once the refused connection's queue is gone.Filed
issues/isolation/a-refused-handle-move-leaves-the-compositor-holding-it.md: the same leak indeliver_with_handles.issues/isolation/the-compositor-ignores-a-message-it-does-not-know.md. Owner:Session::dispatch.issues/isolation/a-window-buffer-is-read-while-its-client-writes-it.md. Owner: the compositor's window blit.issues/isolation/sharedmemory-slices-rest-on-a-size-nobody-checks.md:adopttakes an unchecked size, andshareplusadoptgives two mappings in one process. Ownertoyos::shm.issues/isolation/the-compositor-keeps-a-committed-regions-mapping-for-as-long-as-the-client-does.md:CopyRegion::takedrops the compositor's own handle but not its mapping, which the kernel tears down only once every handle to the region is gone anywhere. Owner:kernel::object::shm's handle-driven mapping teardown.issues/design-debt/decode-payload-accepts-bytes-past-its-type.md: one trailing-bytes rule intoyos::ipc.issues/design-debt/a-client-waits-on-the-compositors-answer-with-no-bound.md:clipboard_setandWindow::create.issues/build/a-key-being-built-is-waited-for-and-another-key-is-not-reds-under-host-load.md: found running this round's gates, unrelated to this PR. Held by the orchestrator.🤖 Generated with Claude Code