# 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** `am.rs:662-671` — on API 28-30 (no `registerUidObserverForUids`), 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`.
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 — suspected** `handoff.rs:63-83, 580-607` — handoff wire layout is hand-verified
byte-exact constants (56/56/100) matching android-14.0.0_r1 only; a framework change corrupts
the server-side parse silently.
Fix: byte-count self-check before transact.
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).