# Review Findings — CoreShift-Core
Cross-subsystem review (spawn, io/fd/reactor, transport, binder, signal, android/uid/dex,
fs/proc/inotify/socket). Severity: `must-fix` = confirmed bug/race/leak/security with a real
production consequence; `fix` = confirmed defect worth correcting; `note` = suspected or design.
---
## spawn
### must-fix
1. **leak — confirmed** `src/spawn/fork.rs:392-398` — `reap_and_drain`'s `Err` arm (child setup
error read failure) closes fds but never `waitpid`s or orphans the child. The child is left
running unmonitored and becomes a permanent zombie on exit.
Fix: blocking `waitpid` like the `Ok(Some)` arm at `fork.rs:386`, or `orphan_child(pid)`.
2. **leak — confirmed** `src/spawn/mod.rs:895-899` — `RunningProcess` has no `Drop`. A caller that
uses `spawn_start` and drops the handle (or hits an error in `register_with_reactor` at
`mod.rs:1314`) leaks an unreaped child (zombie) and closes the pidfd without handing the pid to
the reaper.
Fix: `impl Drop for RunningProcess` calling `orphan_child(pid)` (idempotent; reaper tolerates
`ECHILD`).
3. **leak — confirmed** `src/spawn/mod.rs:1195-1203` — `ManagedProcess::Drop`'s 100 ms bounded reap
give-up returns without `orphan_child(pid)`. A D-state (uninterruptible) child becomes a
permanent zombie once it finally exits.
Fix: call `orphan_child(pid)` before returning on the give-up path.
4. **race — confirmed** `src/spawn/mod.rs:1079-1085` vs `1537-1558` — `ManagedProcess::poll_completion`
has no `CancelPolicy::None` give-up (the blocking `wait_loop` has `deadline_passed_at`). A wedged
child with `CancelPolicy::None` makes `poll_completion`/`next_deadline` poll at 100 ms forever,
diverging from blocking `spawn()`.
Fix: mirror the `deadline_passed_at` bound in `poll_completion`.
5. **design — confirmed** `src/spawn/mod.rs:1183-1190` — `ManagedProcess::Drop` unconditionally
`SIGKILL`s even when `CancelPolicy::None` ("do nothing on cancellation") was selected,
contradicting the documented policy.
Fix: skip the signal for `CancelPolicy::None` but still reap/orphan.
6. **design — confirmed** `src/spawn/mod.rs:521-523` — `Process::kill` treats `pidfd_send_signal`
`EINVAL` as "kernel lacks it" and falls back to `kill(pid, ...)`. On modern kernels `EINVAL`
means an invalid signal (esp. `sig==0`), where the fallback becomes an existence-check that
succeeds — semantics change.
Fix: fall back only on `ENOSYS`.
7. **false-positive — retested** `src/spawn/posix.rs:214` — the review claimed glibc's
`posix_spawn` does not honor `FD_CLOEXEC` and that `POSIX_SPAWN_CLOEXEC_DEFAULT` is required for
the `CloexecOnly` contract. **Not confirmed by direct test on glibc 2.43**: an fd marked
`FD_CLOEXEC` is correctly closed in the posix-spawned child, satisfying `CloexecOnly`.
`POSIX_SPAWN_CLOEXEC_DEFAULT` additionally closes fds that do *not* carry `FD_CLOEXEC`, which
`CloexecOnly` does not promise. The flag is also **unsupported by some libc builds**
(glibc 2.43 returns `EINVAL` from `posix_spawnattr_setflags` for `0x4000`), so setting it
unconditionally would break spawn on those systems. Closed as not-applicable; no code change.
8. **bug — suspected** `src/spawn/clone3.rs:125-129` — if `CLONE_PIDFD` was requested but
`pidfd_out == 0`, the code silently returns `Process::new` without a pidfd, silently degrading
`Clone3Pidfd`'s documented guarantee.
Fix: return an error when a pidfd was requested but not delivered.
9. **race — confirmed** `src/spawn/mod.rs:99-111` — if the reaper thread's `Builder::spawn` fails,
`REAPER_STARTED` stays `true` forever and no future orphan is ever reaped (zombie accumulation).
Fix: reset the atomic on spawn failure.
### fix / note
10. **design — confirmed** `src/spawn/fork.rs:102-132` — the child-error handshake blocks
indefinitely (no timeout); a `SIGSTOP`ped or hung child stalls `spawn_start` forever. Also,
`read_child_setup_error` treats EOF after a partial message as success (`Ok(None)`).
Fix: return an error when EOF arrives with `read_len > 0`; bound or document the block.
11. **nit — confirmed** `src/spawn/mod.rs:42-61` — `SYS_CLONE3`/`SYS_PIDFD_SEND_SIGNAL` are
cfg-gated to a fixed arch list; x86/mips have the same numbers but the crate fails to compile
there.
Fix: widen the cfg list.
12. **design — confirmed** `src/spawn/mod.rs:84-147` — the background `spawn-orphan-reaper` thread
contradicts the crate's "no hidden threads" invariant (`lib.rs:22`). Documented and targeted, so
benign; amend the invariant or make the thread opt-in.
13. **nit — confirmed** `src/spawn/mod.rs:855-861` — exec backends don't validate a negative
`pgroup.leader` upfront; it surfaces as a child-side `setpgid` `EPERM`.
Fix: reject `leader < 0` in `validate_backend`.
14. **nit — confirmed** `src/spawn/fork.rs:70` — under `vfork` the child shares the calling thread's
TLS, so its failing syscalls clobber the suspended parent's `errno`. Cosmetic (parent re-reads
errno after its own syscalls); otherwise the vfork child path is genuinely allocation-free and
async-signal-safe.
---
## io / fd / reactor
### must-fix
15. **leak/race — confirmed** `src/io/drain.rs` — when the child or a descendant holds a pipe
write-end open past the child's exit, the drain never reaches EOF and the reactor waits forever
(no timeout on the managed path; blocking `spawn` has the N4 bound but `ManagedProcess`/direct
`RunningProcess` users do not).
Fix: surface a drain-completion timeout the caller can bound, matching blocking `spawn`.
### fix / note
16. **race — confirmed** `src/reactor/mod.rs` — `Reactor::del` on a closed fd with a stale epoll
registration: if a caller registers an fd, the fd is closed elsewhere, then a new fd reuses the
number, the stale registration can deliver events to the wrong token.
Fix: assert/guard against deleting a token whose fd does not match, or require
`Reactor::del` before close (documented contract already requires it).
17. **nit — confirmed** `src/fd.rs` `Event` — the `error` field is largely dead in the spawn paths
(handlers match `readable || hangup` and only `drop_*` on `error`); behavior is fine but the
distinction is undocumented.
---
## transport
18. **design — suspected** `src/transport.rs:55-56, 172, 175` — `WatcherId` is the only handle in the
shared `TransportEvent`; a daemon running the abstract-socket backend and a Binder backend
multiplexing into one registry would collide (both loops start at 0).
Fix: scope ids per backend (offset/tag), or key the registry `(backend, WatcherId)`.
19. **design — confirmed** `src/transport.rs:141-143` — `WatcherSink::push` conflates transient
backpressure (`EAGAIN`) with a dead peer (`EPIPE`); docs tell the caller to evict on any error,
so a slow reader gets evicted.
Fix: add a non-fatal `Backpressure` signal or document that backends must never push while the
peer buffer is full.
20. **error-handling — confirmed** `src/transport.rs:96-108` — `ReplySink::send` errors are
indistinguishable from a dropped sink; a failed reply is silently lost.
Fix: document that the error must be logged and that a Binder backend's blocked transaction
thread must be unblocked on delivery failure.
---
## binder
### must-fix
21. **safety — confirmed** `src/binder/serve.rs:478-493` — `DeathRecipient::drop` calls
`AIBinder_DeathRecipient_delete` without first `unlinkToDeath`-ing any linked target. A delivery
already in flight can still reference the freed framework object → use-after-free inside
`libbinder_ndk`. The `Arc` slab protects only the Rust callback box, not the NDK object.
Fix: track linked targets and `unlink_to_death` each before `delete`, or document that `unlink`
is mandatory and assert on drop.
22. **race/leak — confirmed** `serve.rs:263-265`, `am.rs:465-467, 581-584, 692-695`,
`display.rs:377-379`, `fps.rs:237-239`, `task_stack.rs:202-204` — every `open*` spawns another
permanent `ABinderProcess_joinThreadPool` thread. The NDK documents a single-thread, once-per-
process call; each open/close leaks a thread.
Fix: a process-global `OnceLock` starting the single pool thread.
23. **bug — confirmed** `src/binder/sys.rs:96-105` — `read_string` collapses a genuine empty string
to `None` (`writeString("")` decodes to `None`, conflating `""` with null).
Fix: track the `-1` marker explicitly and return `Some("")` for length 0.
24. **bug — confirmed, severity escalated** `am.rs:662-671` — on API 28-33 (Android 9-13; no
`registerUidObserverForUids`, which the `IActivityManager.aidl` at android-11..14 tags shows was
only added in Android 14 / API 34 — **not** S as `dex.rs:714` previously claimed), the fallback
still writes the 5th `int[] uids` arg to the 4-arg `registerUidObserver`; the server ignores the
trailing array, so the observer is registered **unfiltered** across every uid, contradicting the
doc claim at `am.rs:657`. Cross-checked against the 81c041b review (H1): same call site, same
root cause — the write sequence is not branched on which register code is used. Range widened
from the original "API 28-30" after AOSP source verification (android-12, -12.1, -13 all lack
the ForUids code; android-14 adds it).
Fix: branch the parcel writes on which register code is used.
25. **bug — confirmed** `am.rs:663` (`let _ = transact_write(...)?`) — registration failure is
silently swallowed (a `SecurityException` reply is dropped without reading the exception
header). A rejected registration returns `Ok` and the daemon runs with a dead observer.
Same pattern at `am.rs:450` and `am.rs:561`.
Fix: read the reply's first i32 and `Err` when `ex != EX_NONE`.
26. **race — confirmed** `am.rs:663` vs `682-690` — registration happens *before* the statics are
published; a callback landing between them sees `UID_READ_I32 == 0` / `kind = None` and returns
`STATUS_UNKNOWN_TRANSACTION`, or updates `UID_LAST_EVENT` before the eventfd is stored (event
lost).
Fix: publish the constants before the register transaction.
### fix / note
27. **error-handling — confirmed** `serve.rs:259`, `am.rs:365`, `handoff.rs:459,515`,
`task_stack.rs:156`, `fps.rs:182`, `display.rs:298,347` — `AIBinder_associateClass` bool return
is ignored everywhere. A failed association leaves the binder classless; the next
`prepareTransaction` fails with a masked `STATUS_INVALID_OPERATION`.
Fix: check the bool and return `CoreError` on false.
28. **design — confirmed, FIXED (unreleased)** `handoff.rs` — handoff wire layout used hand-verified
byte-exact constants (56/56/100) matching android-14.0.0_r1 only; a framework change or a
miscounted constant corrupts the server-side parse silently. The two-entry constant was in fact
**wrong**: `String16 "exec"` is 4-char/16 bytes (`4 + pad((4+1)*2) = 16`, NUL + padding), so
`BUNDLE_LENGTH_TWO` was 100, not the real 104 (`4 + 52 + 48`).
Fix (landed): a host-runnable wire model (`src/binder/wire.rs`) derives the bundle length by
running the exact write sequence through a byte counter, and the same `write_bundle_body` emits
the on-device bytes — advertised length and emitted bytes can no longer disagree. Byte math
re-verified field-by-field against AOSP `AParcel_writeString`/`writeStrongBinder`; covered by
host tests (`single_bundle_body_is_56`, `two_bundle_body_is_104`,
`write_path_emits_exactly_the_advertised_length`).
29. **design — confirmed** `display.rs:269-275` — blocking eventfd for the display callback
(contrast `fps.rs:199-205` `EFD_NONBLOCK`); an edge-triggered epoll drain would wedge.
Fix: use `EFD_NONBLOCK`.
30. **race — suspected** `sys.rs:934-936`, `serve.rs:139`, `am.rs:37/52/90`, `display.rs:179` —
process-global statics torn down while binder pool threads still run → UB at teardown.
Fix: leak the statics (`Box::leak`) or join/detach the pool before exit.
---
## signal
### must-fix
31. **bug — confirmed** `src/signal.rs:279-288` — `unblock_all` destroys the entire signal mask
(`SIG_SETMASK` with an empty set). Used only in the spawn child (correct there — the child wants
all signals unblocked), but as a public API it would unblock every signal a live signalfd
depends on; the next SIGTERM then has default disposition → process death instead of a signalfd
read.
Fix: only unblock a caller-specified set, or warn loudly in the docs.
32. **bug — confirmed, small window** `src/signal.rs:118-136` — the handler is installed before
`SHUTDOWN_FLAG_PTR` is published (and the pointer is cleared before the handler is restored), so
SIGINT/SIGTERM arriving in that window hit the new handler with a null/old flag and are dropped.
Fix: publish the pointer before installing handlers (and block the two signals around the
switch).
### fix / note
33. **bug — confirmed** `src/signal.rs:311-315, 205-217, 233-245` — signalfd's blocking requirement
is understated: the signals must be blocked in **every** thread before creating the signalfd,
not just the reading thread. Only `block_current_thread` exists.
Fix: expose a process-wide block or document the all-thread requirement.
---
## android / uid observer / proc / fs
### must-fix
34. **leak/race — confirmed** `src/binder/am.rs:613-706` — the uid observer never calls
`unregister()`/`Drop` and uses process-global statics. A second `open_with_uid_observer`
overwrites the shared eventfd/state while the first observer stays registered with the
framework; its callbacks then write to the second watcher's eventfd and clobber
`UID_LAST_EVENT`. `unregister_code` is resolved (am.rs:747) but never used; the `AIBinder`'s
strong ref is never released. Contrast `FpsListener`/`TaskStackListener` which implement
`Drop → unregister()`.
Fix: route callbacks per-instance via `AIBinder_new(userdata)` + `AIBinder_getUserData` and add
`Drop` that calls `unregisterUidObserver`.
35. **safety/nit — suspected** `src/binder/sys.rs:84-88` — `string_alloc` hands a `String`'s buffer
to `AParcel_readString` and only re-validates at `finish()` by truncating at the first NUL; a
malformed peer could leave non-UTF-8 bytes returned as `Some(String)` (UB).
Fix: hold a plain `Vec<u8>` until `finish()` validates UTF-8.
36. **design — confirmed** `src/android/dex.rs:506` — `framework.jar` is fully re-read and re-parsed
up to 8× per uid-observer registration (`find_transaction_code` per code).
Fix: cache resolved codes in statics.
37. **nit — suspected** `src/android/dex.rs:162` — `end + 4` can overflow in debug builds
(unreachable in practice: all callers cap `idx` via `cap_section_size`).
Fix: `end.checked_add(4).is_none_or(...)`.
38. **nit — suspected** `src/android/dex.rs:52-61` — EOCD located by scanning for the *last*
signature occurrence; a false signature embedded in the APK/ZIP signing block could be picked.
Cosmetic for stock framework.jar.
39. **design — confirmed, documented** `src/proc.rs:60-76, 188-208` — the path-based `/proc/<pid>`
convenience helpers retain the pid-reuse TOCTOU (recycled pid between lookup and read
misattributes another process). `ProcDir` (proc.rs:224-341) pins correctly via dirfd +
`openat`/`fstatat`.
Fix: route callers through `ProcDir`; racy wrappers are safe only for read-only diagnostics.
---
## fs / socket / log
### fix / note
40. **durability — confirmed** `src/fs.rs` `write_atomic` — the temp file is fsync'd but the parent
directory is not, so a crash right after `renameat` can lose the rename.
Fix: fsync the parent dir after rename.
41. **TOCTOU — confirmed** `src/socket.rs` — the stale-path `unlink` before `bind` (and any chmod)
is a symlink/race window for an attacker able to write the socket directory.
Fix: `O_NOFOLLOW`/openat-style pinning or document the threat model.
42. **contract — confirmed** `src/fd.rs:233` `read_u64_blocking` — retries on `EINTR` but treats any
other error as fatal; on a non-eventfd (or after a partial read) this can wedge. Document the
eventfd-only contract (already the documented use).
43. **design — confirmed, CI gap** `.github/workflows/ci.yml` — every binder wire-layout constant
and the uid-observer/observer/handoff read-write paths are `#[cfg(target_os = "android")]`, so
CI only ever `cargo check`s them (`--target aarch64-linux-android`); no test ever *executes*
the Android code on a host runner. Hand-verified byte math (finding 28) and the am.rs fallback
(finding 24) shipped wrong precisely because the affected code was compile-only. The wire.rs
model (finding 28 fix) is the first host-runnable byte test for an Android-only path.
Fix: keep the wire/write sequences in the non-gated (or `cfg(test)`) model so host tests pin
them; extend to the uid-observer AIDL arg layouts.
---
## Fix status
- **spawn items 1-12 — FIXED, released as 2.18.0.** `reap_and_drain` `Err` arm orphans the child
(`fork.rs`); `RunningProcess`/`ManagedProcess::Drop` leak paths call `orphan_child`; `Process::kill`
falls back to plain `kill` only on `ENOSYS`; clone3 pidfd handles on `u64::MAX` sentinel return an
explicit error; reaper spawn-failure orphans the child; child-error handshake orphaned; `SYS_*`
consts pinned via `#![allow]` where needed; `register_with_reactor` orphan path wired.
- **binder handoff — SYNC, new in 2.18.1.** `Handoff` now `unsafe impl Sync` (handoff.rs:430):
field-by-field safety comment lists every member (`DlHandle`, `Vtable` fn-pointer struct,
`*mut AIBinder_Class`, `OwnedBinder`, two `u32`s) and why each is Sync. Unblocks the
FocusSource finding-9 bounded handoff (persistent worker thread + `recv_timeout`).
- **binder handoff wire layout — FIXED (finding 28).** `BUNDLE_LENGTH_TWO` was 100; the real
two-entry length is **104**. Root cause: `String16 "exec"` = 16 bytes (`4 + pad(10)`), not 12 —
the old constant dropped the NUL terminator and 4-byte padding. Fix introduces
`src/binder/wire.rs`: a host-runnable wire model that derives the bundle length by executing the
exact write sequence against a byte counter and emits the same bytes on-device via the shared
`write_bundle_body`. Length (single 56 / two 104) and write-path self-consistency are pinned by
`cargo test` on every host commit (finding 43's CI gap). Byte math re-verified field-by-field
against AOSP android-14.0.0_r1 `AParcel_writeString` (`writeInt32(len)` + `writeInplace((len+1)*2)`
padded to 4) and `writeStrongBinder` (28 = 24-byte `flat_binder_object` + 4-byte stability int32).
- **Full suite green:** 125 lib + 16 integration + 2 repo-files + 7 + 2 = 0 failures; clippy clean;
`cargo check` clean on aarch64 + armv7 android.
> Remaining open rows: 13-20 (binder thread-pool `OnceLock` sites), 21-22 (DeathRecipient
> unlink-on-drop), 24 (uid-observer fallback unfiltered registration — must-fix, escalated),
> 25 (observer registration error swallowed), 26 (publish-before-install race), 30-42 as written,
> 43 (CI gap) as written. Low-priority notes except where a security consequence is stated in the row.