Repository navigation
Conversation
Review r1: #1080 at 7ef0a48 (base 576bebf)I read every file from the commit object. The worktree was checked out to 576bebf at 08:49:56 while I was reviewing. origin/main is now b0164b2 (#1073, #1078). Architectural assessmentThe branch implements the approved design. The six ABI calls are I verified these separately:
The completeness claim does not hold yet. Safe code can still close a handle it does not own by its descriptor number, and two safe paths still produce a dangling FindingsBLOCKER 1.
BLOCKER 2.
BLOCKER 3.
BLOCKER 4.
BLOCKER 5. mio cf8d7a57
Evidence and PR description (orchestrator)
Suggestions
SEND BACK |
Review r1: response (head a92abf9, rebased onto b0164b2)B1, fixed.
std's B2, fixed. B3, fixed.
The second arm is a send rather than A mutation that gives B4, fixed.
B5, fixed in the fork. mio f7620928: the waker owns both pipe ends and deregisters its read end before the end closes. No guest build here compiles mio's ToyOS port, so it was checked directly for Suggestions. I took Records.
Evidence: https://github.com/ToyOSOrg/evidence/tree/fbf22acc02e777afec63a90f05d12c14e8a679c3/r1 |
Review r2: #1080 at a92abf9 (base b0164b2)I read the r1 delta (a92abf9) in full. I also read the changed callers and boundaries: Architectural assessmentThe round keeps the approved design: only an owner ends, moves, duplicates or lends a handle, and
One B4 fix creates a new defect, below. r1 blockers
FindingsBLOCKER.
Evidence
Suggestions
SEND BACK |
Review r2: response (head e33b248)Blocker, fixed without dropping reuse. The So a second child gets a live holder of its own. The The Records.
Evidence: https://github.com/ToyOSOrg/evidence/tree/52eb0bc00791ab1f1f77afd791ecc8c616f3c065/r2 |
Review r3: #1080 at e33b248 (base b0164b2)This round checks the r2 blocker's fix. I read e33b248 in full: Architectural assessmentIs
The table already has a primitive that keeps an end alive independently of its handle: the counted call. What the counted call is.
Ownership by types. No path copies an exported holder into two children.
Size. This round adds +129/−23 production and +51/−2 test lines. The PR as a whole is now +2093/−1804 production and +505/−431 tests. r2 blockerResolved. The pieces:
Both r2 consequences are fixed:
The new arm checks both. The
The mutation control is real. The B1–B3/B5 resolutions from r2 are untouched. This round changes no FindingsNo blockers. SUGGESTION.
SUGGESTION. Two comments in this round are less precise than they could be:
Evidence
LAND |
|
Orchestrator: landing at 141055d (comments only past e33b248). Review r3 LAND. T14 metal at e33b248: 7 boots rc=0, judged 275 passed and 11 failed, none traced to #1080. Five are standing main reds (klogd_hosted, i219_messages_on_its_msi_vector, three amdvi_*); five are main's handle_lifetime census-wait hang (identical on main; recorded in #1090); osbench_lan is unmeasured because the AMD peer was down. Stream and socket export against the real netstack is covered by the QEMU reruns at e33b248. |
A handle's number reached its ownership in safe code: `close`, `dup`,
`dup_narrowed`, `shm_sub`, `message_send` and `device_dma_map` were safe
over a `RawHandle`, so a guessed or copied number could unmap a
`SharedMemory` under its owner, or duplicate and send the one writable
handle of a buffer a type relied on being exclusive.
Rust's I/O-safety rule, applied to ToyOS handles:
- toyos-abi: those six calls are `unsafe fn` with one stated precondition
each, beside `spawn`, `shm_map` and `namespace_build`; `crate::handle`'s
header states the boundary once. Calls that only act on the object
(read, write, fstat, a poll) stay safe over a number: misdirected, they
reach the wrong object's protocol, not freed memory.
- toyos: `BorrowedHandle<'a>`, `AsHandle` answering one, `OwnedHandle`
`try_clone`; IPC's receive answers `OwnedHandle`s, so every receiver's
`from_raw` goes; the fd table's `Object` is owned and calls see a `Lent`
borrow; `Launch`, `Spawn` and the net ends own or borrow what they name.
A `std` feature converts to and from std's handle types for a program's
own copy.
- std: `std::os::toyos::io::{OwnedHandle, BorrowedHandle, AsHandle,
AsRawHandle, FromRawHandle, IntoRawHandle}` in Windows' shape. `Child`
implements them, replacing `ChildExt`. `CommandExt::endow` and `provide`
take an `OwnedHandle`; `inherit_handle`, `inherit_exported`, `under`,
`launcher` and `boot_image` borrow and copy at the call.
`fs::adopt_namespace` takes an `OwnedHandle` and is safe.
- libc keeps its descriptor table as the owner and lends from it.
- Every program and guest test converted. The supervisor's `Moved` holder
goes: a `Command` owns what it endows, a refused spawn closes it, and an
acceptor the supervisor must keep for a later start is endowed as a copy
and dropped only once the start succeeds. A launch's caller ring is
judged, and the program's ring made for its job, before the job is
endowed. The compositor answers a frame through the connection that
owns it.
- Forks: winit lends its windows' connections to the waiter; mio's inbox
owns its handle.
Two behaviours change, as agreed: a refused spawn closes the handles it
was given, and `under` needs `DUP` on the place, which std's `Command`
copies when it is set.
A stream's own handle can no longer be endowed in safe code (it is never
an `OwnedHandle`), so `Spawn` checks only its borrowed descriptors for one,
and `stream_handles` drops the endow and provide arms that tested the
refusal.
Closes issues/a-handles-number-reaches-its-ownership-in-safe-code.md.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ger than its stream, a Command spawns again - `toyos::fd::close`, `dup2` and `replace` are `unsafe fn`. Closing a descriptor a `File`, an `OwnedFd` or a stream owns ended the handle it lends, and a reused number then lent another owner's handle to whoever still held the borrow. Each caller states the owner it closes for. The rust fork's `OwnedFd::drop` loses the `unused_unsafe` allowance and the comment that called the close safe (rust 8004d62da51). - `net::stream::doorbell` answers a borrow as long as the stream's, not `'static`. `net::ends::kept` is `unsafe` with the invariant it relied on, and std's `BorrowedHandle::inner` keeps its own lifetime. - `stream_handles` tests again that a stream's handle does not leave its process: a duplicate, endowed or sent in a frame, is refused the kernel's `PermissionDenied`, because every handle of a stream carries `HELD_CONNECTION`, which has no `TRANSFER`. A launch whose frame the kernel refuses now says so (`LaunchError::NotSent`, then `Refused::Syscall`) rather than `NotStarted`. - std's `Command` spawns again as it did. Each spawn borrows its `inherit_handle` descriptors and copies its boot image. Endowments and provided connectors are taken at the spawn's first line, so any refusal closes them, as `CommandExt::endow` says. - mio's waker owns its pipe and deregisters its read end before the end closes (mio f7620928). - `AcpiDev::idle`, from main, passes `acpi_idle` its claim's number. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts own holder Since r1, `inherit_handle` exported a stream's or a socket's holder when it was called, and every spawn of the `Command` duplicated that one end. A second child then got a holder another process had already taken up, and the `Command`'s copy kept the holder's connection open, so netstack's hang-up of a child that died waited until the `Command` dropped. The `Command` now keeps the stream or socket itself (`net::Exporter`, one counted call on its end, released when the `Command` drops). Each spawn exports a holder for its child alone, which goes with that spawn, as on main. `stream_handles` spawns one bound `Command` twice and has both children write through holders of their own. Exporting once at `inherit_handle` instead fails it: the second spawn asks netstack for nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review r3, addendum: the rebase onto 02362fd (head 968d24a)I compared commit 1 before and after the rebase with (1)
|
Problem
toyos_abi::syscall::close,dup,dup_narrowed,shm_sub,message_sendanddevice_dma_mapwere safe functions over aRawHandle, au32anyone can name. A guessed or copied number could close aSharedMemory's handle under it, so a device window the kernel unmapped was still written through itsbytes(), or duplicate and send the one writable handle of a window buffer the client draws into with plain stores. IPC's receive answered numbers too, so every receiver claimed ownership in anunsafeblock of its own. (issues/a-handles-number-reaches-its-ownership-in-safe-code.md, closed here.)What changes
Rust's I/O-safety rule, applied to ToyOS handles: only an owner ends, moves, duplicates or lends a handle; a number is not authority in safe code.
toyos-abi: the six calls areunsafe fnwith one stated precondition each, besidespawn,shm_mapandnamespace_build, which already were.crate::handle's header states the boundary once. A call that only acts on the object (read,write,fstat, a poll,accept,process_*) stays safe over a number: misdirected, it reaches the wrong object's protocol, not freed memory.toyos:BorrowedHandle<'a>(Copy, transparent, constructed safely only from an owner);AsHandleanswers one;OwnedHandle::try_clone. IPC's receive (Batch,FrameRx::take_handles,recv_handles_exact) answersOwnedHandles, andendow::FromHandleis safe over one. The fd table'sObjectis owned and non-Copy; a call sees its object as aLent<'_>borrow.LaunchandSpawnown their endowments and borrow their descriptors, place and launcher; the net ends keep their handles under one stated invariant (net::ends::kept,unsafe), andnet::stream::doorbelllends a bell for as long as the stream it was asked of. An opt-instdfeature converts to and from std's handle types for a program's own copy of the crate (off in std's and libc's copies).sdk/std):std::os::toyos::io::{OwnedHandle, BorrowedHandle, AsHandle, AsRawHandle, FromRawHandle, IntoRawHandle}, shaped asstd::os::windows::io.Childimplements them andFrom<Child> for OwnedHandle, replacingChildExt.CommandExt::endow/providetake anOwnedHandle;inherit_handle,inherit_exported,under,launcherandboot_imageborrow and copy at the call. ACommandspawns again as before. Each spawn borrows its descriptors and copies its boot image, and takes the endowments and provided connectors at its first line, so any refusal closes them. For a stream or a socket, theCommandkeeps the stream itself (net::Exporter, one counted call on its end), and each spawn exports a holder for its child alone, which goes with that spawn, as on main.fs::adopt_namespacetakes anOwnedHandleand is no longerunsafe.toyos::fd):close,dup2andreplaceareunsafe fn, aslibc::closeis: closing a descriptor aFile, anOwnedFdor a stream owns would end the handle that owner lends. libc keeps its descriptor table as the owner and lends from it; each caller of the three states the owner it closes for. The rust fork'sOwnedFd::dropalready made the call inside its sharedunsafeblock, and loses the allowance and the comment that called the close safe: rust 8004d62da51, gitlink moved.from_rawblocks are gone; no program outside the SDK holds anunsafefor handle ownership. The supervisor'sMovedholder is deleted: aCommandowns what it endows, a refused spawn closes it, and an acceptor the supervisor must keep for a later start (Served::Move, logkeeper's origins) is endowed as a copy and dropped only once the spawn succeeds. A launch's caller-ring descriptor is judged, and the program's ring made for its job, before the job is endowed (LaunchFd). The compositor answers a frame through the connection that owns it;ClientFrame::handleis an identity only.toyos-0.30.1348c7022a → 0121752b: the loop lends each window's connection to the waiter for the wait alone, under theArc<Mutex<Window>>its entry holds.toyos5d013b87 → cf8d7a57 → f7620928: the selector's inbox owns its handle as std'sOwnedHandle, and the waker owns its pipe (it leaked both ends) and deregisters its read end before that end closes.Decisions
toyos-abigains no ownership types, so the kernel and std's sysroot copy keep a dependency-free ABI, asstd::os::fdsits onlibc::close.Commandcopies a borrowedinherit_handle/under/launcher/boot_imagehandle when it is set, sounderneedsDUP. Every place that reachesundercarries it: the supervisor's only callers are a launch's place, whichtoyos::spawn::launchsends as a copy of the caller'sself(WRITE | DUP | TRANSFER), and a session owner'sMSG_PLACE, a copy of its ownself. A place withoutDUPis refused with the supervisor'scannot startline, not silently.launcherwas used by number before and is now copied too; every launcher a program holds is a namespacenamespace::buildmade (DUP | TRANSFER | READ), which nothing narrows.HELD_CONNECTION, which has noTRANSFER, so the kernel refuses to endow or send one or a duplicate of one.Spawnchecks its borrowed descriptors for a stream by name (they go as exported holders), and no longer its endowments.stream_handlestests that a duplicate, endowed or sent in a frame, is refused the kernel'sPermissionDenied. A provided connector travels in a launch frame, refused by the same check. So that refusal reaches the caller, a launch whose frame the kernel refuses isLaunchError::NotSentwith the kernel's word, whichSpawnanswers asRefused::Syscallwhere it wasNotStarted.ChildExtis replaced by the io traits rather than kept beside them: two spellings of the same handle access would be two mechanisms.Checks
Head 968d24a on main 02362fd. 744ad07 is the change; a46bc85 is review r1, 8c10437 is review r2 and 968d24a is review r3 (comments only). The host, image, guest compile and the ten QEMU tests below were rerun at 968d24a after the last rebase; the controls and the metal judge are reused from e33b248, as the rebase brought in only main's changes and the resolutions kept main's structure with the owned and borrowed types.
fd::close,dup2,replaceand a'staticbell are refused on the branch (log) and all compiled at 7ef0a48 (log).cargo run -- --ci hostat 968d24a, exit 0, 105 steps green (log). No host test reachesBatchorFrameRx; theProbetests cover the send side (move_batch), andBatch'snext,exactandDropare covered by inspection.cargo run -- --build-onlyat 968d24a, exit 0 (log).build/rootandfiletime(a host tool),bootloader(the UEFI loader),generator(loom's dependency),update(an image key the build sets),doom(its build script wants the C compiler the build names),kernelprobe(x86-64 only) andosbench(AArch64 only).tests/guest/rust-testschecks clean at 968d24a (log).x86_64-unknown-toyosat f7620928 against this head'stoyos-abi. Exit 0; the log names the revision, target and command (log).stream_holderspassed at r1's head.Commandgives a stream or socket. At e33b248,stream_holders,netstack_holders_exported_to_children,udp_receive_times_out,libc_children,session_lifecycleanda_launch_gives_each_unsent_stdio_slot_a_nullpass, exit 0 (log).nvme_disk_boots,usbd_drives_the_spare,the_data_partition_beside_the_stick_is_designatedanda_host_disk_beside_the_stick_is_left_alone(the paths the rebase'sHandedresolution touches) pass, exit 0, 10 passed (log).stream_handlesspawns oneCommandbound to a stream twice, and each child writes through a holder of its own (log). With round 1's export-once behaviour restored as a mutation it goes red: the second spawn asks netstack for nothing (log).HELD_CONNECTIONgivenTRANSFER, turns the duplicate arms red (log).klogd_hosted,i219_messages_on_its_msi_vector, and the threeamdvi_*rows (the AMD bench's).test_rs_handle_lifetimespins in nanosleep until the list bound, and the four jobs after it never run.osbench_lanhad no peer (the AMD bench was down), five are the standing reds above, and five aretest_rs_handle_lifetime's hang and the four members after it, tracked in shm: an Immutable takes its mapping away when it drops, so a spawn from /system leaves the census as it was #1090. Staged by (prepare, request).Unresolved
stdarm is compiled, never clippy-linted. It iscfg(target_os = "toyos"), andbuild/clippy.rslints host shapes only, as for every other ToyOS-target arm of the SDK, std and libc. Each guest build compiles it under-Dwarnings.osbench_lanis unmeasured for want of its peer.test_rs_handle_lifetimefails on main. It spins until the shared-debug list bound at 576bebf and at 7ef0a48 alike. It is main's, and it blinds the shared-debug members after it on both; shm: an Immutable takes its mapping away when it drops, so a spawn from /system leaves the census as it was #1090 tracks it.🤖 Generated with Claude Code