From 412373e39e905a49bf85416d05ac545e5d60f3d5 Mon Sep 17 00:00:00 2001 From: yanhenrique-dev Date: Wed, 30 Sep 2026 00:38:11 -0300 Subject: [PATCH 1/3] fix(tauri): stop host binaries inheriting the AppImage library path, and close the pty descriptors on a failed attach Four defects from a read-only audit of the Rust backend, each with a test that fails against the old code. The audit is in docs/notes/audit-backend-rust.md and its reasoning is kept next to the fix, including the hypotheses that were disproved. git_cmd() now clears LD_LIBRARY_PATH, and diff_numstat goes through it. An AppImage build exports a library path pointing at the bundled libraries, which shadow the host ones a child git links against (libcurl, libssl, libz, libpcre2). The child dies with a symbol lookup error, and for `git diff --numstat` that is indistinguishable from "this file did not change", so session changes silently showed 0 additions and 0 deletions. The removal lives in the shared constructor rather than at the call site, so every git spawn inherits it. diff_numstat was the only production spawn outside the documented one-git-constructor rule, and it was also skipping Flatpak routing and the GIT_TERMINAL_PROMPT/GIT_OPTIONAL_LOCKS pair. Two tests in fs/git.rs. The second runs a stub git that exits 127 when it sees the variable, with the variable set on the process rather than with .env(), because a later .env() lands after the removal and wins. clipboard.rs gets the same treatment through a helper_cmd, for the four wl-paste, wl-copy and xclip spawns. The helper loop already reads a spawn failure as "this helper yielded", so on an AppImage the user was told no clipboard helper was installed while wl-clipboard worked in a terminal. pty.rs closes master and slave when the stdio attach fails. open_pty returns bare i32s with no owner and its own error paths have already run, so a dup failure leaked two descriptors per attempt for the life of the process. The test asserts through fcntl rather than by counting /proc/self/fd, which the parallel test harness makes unreliable. pty.rs process_label now goes through host::command, matching the shell spawn on the same path. Latent, since Flatpak was removed in 0.3.5-alpha. The audit also corrects two counts in its own portrait: 169 commands rather than 126, because 43 use #[tauri::command(async)], and zero production unwraps rather than one, since the fs/path.rs hit is inside a test. 427 lib tests pass, up from 423. cargo fmt and clippy -D warnings are clean. --- docs/notes/audit-backend-rust.md | 414 +++++++++++++++++++++++++++++++ src-tauri/src/checkpoint.rs | 6 +- src-tauri/src/clipboard.rs | 25 +- src-tauri/src/fs/git.rs | 81 +++++- src-tauri/src/pty.rs | 89 ++++++- 5 files changed, 601 insertions(+), 14 deletions(-) create mode 100644 docs/notes/audit-backend-rust.md diff --git a/docs/notes/audit-backend-rust.md b/docs/notes/audit-backend-rust.md new file mode 100644 index 00000000..8fb4485d --- /dev/null +++ b/docs/notes/audit-backend-rust.md @@ -0,0 +1,414 @@ +# Rust backend audit + +Audit of `src-tauri/src` (41 files, 30,017 lines) against the TypeScript side +that calls it. Every finding has a reproduction path. Things I looked for and +did not find are listed at the end, because a report that only lists hits does +not tell you where the reading stopped. + +All four findings are fixed on this branch, each with a test that fails against +the old code. The findings are written as they were found, so the reasoning and +the disproofs stay readable next to the fix. + +Dates and numbers are from 2026-09-30 against `origin/main` at `f849f20b`. + +## What the portrait gets right, and what it gets wrong + +The measurements in the task brief hold up, with two corrections that matter +because they change where you would look. + +| Measure | Brief | Measured | Note | +| --- | --- | --- | --- | +| LOC in `src-tauri/src` | 30,017 | 30,017 | exact | +| `fs/tests.rs` | 2,857 | 2,857 | exact | +| `session_store.rs` | 2,787 | 2,787 | exact | +| `harness.rs` | 2,564 | 2,564 | exact | +| `fs/github.rs` | 2,388 | 2,388 | exact | +| `#[tauri::command]` | 126 | 169 | brief counted only the bare attribute and missed `#[tauri::command(async)]`, which appears 43 times | +| `async fn` commands | 0 | 0 | confirmed, no command is an `async fn` | +| `generate_handler!` entries | 171 / 169 unique | 171 / 169 unique | exact, including the two duplicated clipboard commands | +| `unwrap()` in production | 1 (`fs/path.rs`) | 0 | the one in `fs/path.rs:138` is inside `#[test] fn canonicalize_missing_accepts_relative_paths` | +| `expect()` in production | 5 | 5 | exact: four in `contract.rs` on a compile-time `include_str!` artifact, one in `lib.rs` on `tauri::generate_context!` | +| `panic!` / `unreachable!` | 0 | 0 | confirmed in production (one `panic!` in `fs/tests.rs`) | +| `clippy.toml` / `[lints]` | absent | absent | confirmed | + +Two things the portrait does not show. + +The 126 bare commands are not the whole list, and the distinction is not +cosmetic. `#[tauri::command]` runs the body on the calling thread. +`#[tauri::command(async)]` moves it to a worker. Of the 169 commands, 126 are +bare and 43 opt into the worker pool. So "126 commands, all sync" understates +the surface by 43 and hides which third of the API is which. + +`unsafe` appears 27 times in production code and the portrait does not count +it: `pty.rs` 19, `lib.rs` 4, `harness.rs` 3, `fs/github.rs` 1. Nothing in CI +runs `miri` or a sanitizer over any of it. + +## Findings + +### Medium (fixed): `checkpoint.rs` spawns `git` outside the one-constructor rule + +`src-tauri/src/checkpoint.rs:920` + +```rust +fn diff_numstat(before: &Path, after: &Path) -> Option<(i64, i64)> { + let mut cmd = Command::new("git"); +``` + +`docs/notes/tauri-boundary.md#one-git-constructor` states that all git goes +through one sandboxed constructor, and `fs/git.rs:1360` implements it as +`git_cmd()`. Every other git spawn in `checkpoint.rs` uses `git_checked` or a +`git_stdout` wrapper. This one calls `Command::new("git")` directly, so it +skips two things: Flatpak host routing, and the `GIT_TERMINAL_PROMPT=0` / +`GIT_OPTIONAL_LOCKS=0` pair that `git_checked` sets. + +Reproduction path: + +1. `src/lib/checkpoint.ts:118` calls `session_checkpoint_capture`. +2. `checkpoint.rs:591` `session_checkpoint_capture` reaches + `store.capture()` at line 604. +3. `capture()` calls `calculate_session_stats()` at line 177. +4. `calculate_session_stats` calls `diff_numstat()` at line 906. +5. `diff_numstat` spawns bare `git diff --no-index --no-ext-diff --numstat`. + +On an AppImage build the process carries an `LD_LIBRARY_PATH` that points at +the bundled libraries. `src-tauri/src/external_url.rs:1-7` documents the +consequence for a host binary that inherits it: the bundled libraries shadow +the host ones and the helper dies with a symbol lookup error. That note is +why `external_url.rs:45` and `fs/write.rs:542,553` call +`.env_remove("LD_LIBRARY_PATH")`. `git` links libcurl, libssl, libz and +libpcre2, so it is exposed to the same shadowing, and this call site has no +such removal. + +I confirmed the inheritance mechanism rather than assuming it. A stub binary +placed on `PATH` that aborts when it sees the app's marker variable behaves +like this: + +``` +$ APPIMAGE_BUNDLED_LIBS=1 git --version +git: symbol lookup error: undefined symbol +$ env -u APPIMAGE_BUNDLED_LIBS git --version +real-git-ran +``` + +Damage when it fires: silent. `diff_numstat` returns `None` on a non-zero +exit, `calculate_session_stats` propagates that, and the two call sites drop +the entry rather than surfacing an error. Line 177 skips the insert; lines +229 to 231 call `manifest.stats.remove(&relative)`. The user sees the file +listed in session changes with 0 additions and 0 deletions, and nothing in the +UI says the number is missing. + +I could not run an AppImage build in this environment, so the last step, +observing the real `git` fail on a real AppImage, is reasoned from the +documented `xdg-open` failure and the stub above rather than measured. The +code-path asymmetry is verified and unconditional. + +Fix applied: the removal went into `git_cmd()` rather than the call site, so +every git spawn inherits it, and `diff_numstat` now goes through the +constructor. Two tests in `fs/git.rs`, one of which runs a stub git that exits +127 when it sees the variable. + +### Medium (fixed): clipboard helpers inherit the bundled library path + +`src-tauri/src/clipboard.rs:114`, `:121`, `:176`, `:181` + +Four spawns build `Command::new(path)` directly, where `path` is a discovered +`wl-paste`, `wl-copy` or `xclip` binary. None of them removes +`LD_LIBRARY_PATH`, and none routes through `host::command`. + +These are the same class of host binary as the `xdg-open` case that +`external_url.rs` already fixed and documented. `wl-copy` links +libwayland-client, and `xclip` links libX11; both can collide with bundled +copies in an AppImage. + +Reproduction path: `src/lib/clipboard.ts` calls `copy_file_to_clipboard` and +`clipboard_file_paths`, which reach `clipboard_file_paths_sync` and +`copy_file_to_clipboard_sync`. Both loop over helpers in session order. When a +helper dies on a symbol error, the loop treats it as a helper that yielded and +tries the next one, per the documented `stale-helper-yields` rule. If none +survives, the user gets "No clipboard helper found" while wl-clipboard is +installed and working in a terminal. + +The five clipboard entries in `docs/notes/tauri-boundary.md` cover helper +ordering and URI parsing. None mentions the environment, so this is not a +documented tradeoff. + +### Low (fixed): `pty.rs` leaks two descriptors when `dup_stdio` fails + +`src-tauri/src/pty.rs:229-231` + +```rust +let (master, slave) = open_pty(cols, rows)?; +let mut cmd = crate::host::command(&shell); +cmd.args(&args) + .current_dir(&workdir) + .stdin(dup_stdio(slave)?) + .stdout(dup_stdio(slave)?) + .stderr(dup_stdio(slave)?) +``` + +`open_pty` is careful: it closes `master` on the grant/unlock failure at line +433, on the `ptsname` failure at line 437, on the slave-open failure at line +441, and on the resize failure at line 445. `spawn_unix` bails through `?` on +lines 229 to 231 without closing either descriptor, because at that point +`master` and `slave` are bare `i32` values with no owner. + +Reproduction: exhaust the process descriptor limit (EMFILE), then open a +terminal. I confirmed the leak with a standalone program that mirrors lines +223 to 231 and counts `/proc/self/fd`: + +``` +success path: before=4 after=4 delta=0 +failure path: is_err=true before=4 at_bail=6 delta=2 +``` + +Both descriptors stay open. Each failed spawn costs two, and the session +holds them until the process exits. + +Severity is low because it needs descriptor exhaustion. The default limit +here is 1,048,576, and terminals are user-initiated rather than spawned in a +loop, so the realistic path is a long-lived session that has already leaked +descriptors from somewhere else. + +Fix applied: `attach_and_close_on_failure` owns both descriptors on the error +path, mirroring what `open_pty` already does. The test asserts through +`fcntl` on the descriptor rather than by counting `/proc/self/fd`, which the +parallel test harness makes unreliable. + +### Low (fixed): `pty.rs` reaches for `ps` without host routing + +`src-tauri/src/pty.rs:567` + +`process_label` calls `Command::new("ps")` directly. Inside a Flatpak sandbox +the sandbox's own `ps` cannot see host processes, so the label lookup returns +`None` and `command_label` never runs. + +Flatpak packaging was removed in 0.3.5-alpha, so this path is currently +unreachable. It is still a violation of the routing rule the rest of the +backend follows, and it would come back the moment host execution returns. +`pty.rs` is otherwise consistent: `crate::host::command(&shell)` at line 226. + +### Resolved during the audit: the `AGENTS.md` command count was wrong + +`AGENTS.md`, Tauri boundary section + +For most of this audit the file stated 179 entries and 177 unique. The real +numbers are 171 and 169, and commit `5ff7861b` corrects it to exactly those +while I was working. I am recording it because the measurement is the useful +part and the fix is already in. + +I counted two ways that agree. Bracket-matched extraction of the +`generate_handler!` body gives 171 entries, with `clipboard_file_paths` and +`copy_file_to_clipboard` each appearing twice. Separately, there are exactly +169 `#[tauri::command]` attributes in the tree, so every command is registered +once and every registration has a command. + +Worth keeping in mind: the file carried 179 for part of this audit and 171 +earlier the same week, with no corresponding change in `lib.rs`. The count is +hand-maintained, and it is the number a reviewer checks when adding a command. +A check that compares the attribute count against the registration count would +keep it honest without anyone remembering to update it. + +## Process findings + +These are not code defects. Each is a gap in what CI can catch. + +**No dependency vulnerability scan.** `cargo audit` and `cargo deny` are +absent. A crate with a published advisory enters the tree and nothing in the +pipeline objects. Cost: one `cargo deny` config and a CI step, plus a +lockfile policy decision about which advisory sources to treat as blocking. +Cheapest useful version is `cargo audit` alone. + +**No coverage number.** `cargo test` runs and roughly 430 tests pass, but +nothing records what fraction of the 30,017 lines they reach. A change that +drops coverage passes. Cost: `cargo llvm-cov` with a report step, and a +chosen floor. The floor is the arguable part; the report alone is cheap and +makes regressions visible in review. + +**No `miri` or sanitizer run over the `unsafe` code.** 27 `unsafe` sites +exist, 19 of them in `pty.rs` handling raw descriptors, `setsid`, +`TIOCSWINSZ` and `ioctl`. `miri` cannot run the libc calls, so it would not +cover most of them, but it would cover the pointer arithmetic in `lib.rs` and +`harness.rs`. An ASan or MSan build under CI is the tool that fits the pty +code, and it is the one that would have caught the descriptor leak above +without a hand-written proof. + +**No release build in CI.** CI runs `check`, `test`, `fmt` and `clippy`. The +AppImage is built only on tag. The defect in finding 1 depends on the AppImage +bundling bundled libraries, so no CI job would ever exercise the conditions +that produce it. + +## Where the Rust could be doing less + +Four candidates. I did not implement any of them. + +**Stream parsing in `harness.rs`.** At 2,564 lines this is the largest backend +file, and a meaningful part of it frames SSE and normalises event names and +payload keys. That is string work, and it sits behind a 30,000-line rebuild +and a PR that crosses the TS/Rust boundary for what is usually a one-line +change to a regex or a key name. The V2 migration is the evidence: the +`docs/CONTRACTTS.md` note records that it took six pull requests partly +because the field-extraction kit is duplicated across eight `*Protocol.ts` +files. The counter-argument is real and I want it on the record: framing has to +happen where the bytes arrive, and moving the parse to TS would move it behind +an IPC hop per chunk. The split worth considering is framing in Rust, naming +in TS. + +**`fs/git.rs` shells out for reads.** 1,812 lines calling `git` for +`rev-parse`, `cat-file`, `diff`, `log` and `branch`. Output parsing of this +kind is fragile across git versions, locales and configuration, and this +project already carries notes about each of those. `gix` would give typed +reads with no subprocess. The counter-argument is the documented +`one-git-constructor` decision: git on the host keeps hooks, ssh and user +config working, and a library implementation would not honour a user's +`core.sshCommand` or credential helper. The defensible split is reads through +`gix`, writes and anything needing hooks through the CLI. This is a large +change and I am not claiming it is cheap. + +**Untyped state crossing the boundary.** `control.rs` passes orchestration +state as `serde_json::Value` through `mpsc::Sender`, and +`session_store.rs` holds `model_settings: Value` and `blocks: Value`. The +`model_settings` case is at least guarded: `session_store.rs:264` rejects a +non-object before writing, and `src/lib/sessionStore.ts:920` normalizes on the +way back out. The orchestration state has no equivalent guard, because there +is no type to check against. Both sides can construct it and it changes +independently, which is the condition `docs/CONTRACTS.md` names as the +justification for a contract artifact. Whether it clears the bar depends on +whether the orchestration state has already drifted once. I did not find +evidence that it has, so I am listing it as a question rather than a finding. + +**Serialisation acting as an undocumented API.** 110 structs carry +`#[serde(rename_all = "camelCase")]`. Five boundaries are declared in +`contracts/rust-ipc.json`; the other 105 are convention. This is the +documented failure mode of this repository, and the reason the contract +artifact exists. The cost of a per-struct artifact would be high and the +documented criterion (both sides building independently, plus a shape that has +actually drifted) is not met by most of them. The cheaper mitigation is a +generated TypeScript type from the Rust structs, so a rename in Rust breaks +`tsc` rather than producing `undefined` at runtime. That is a tooling +proposal, not a contract proposal. + +## What I looked at and did not find + +Each of these was a live hypothesis with a plausible mechanism. Reporting the +disproofs matters as much as the hits, because four of them would have been +wrong findings. + +**The `pty.rs:265-266` descriptor leak on a failed second `dup`.** The brief +predicted this. `File::from_raw_fd(dup_fd(master)?)` looks like a leak: if the +second `dup` fails, the `File` built on line 265 is dropped by the early +return. It is not a leak. Measured with the same code shape: + +``` +both-succeed: before=5 during=7 after=5 delta=0 +second-fails: is_err=true before=5 after=5 delta=0 +``` + +`File` owns the descriptor and `Drop` closes it. The real leak in `pty.rs` is +the one at lines 229 to 231, where the descriptors are still bare `i32` at +the point of failure. Same subsystem, opposite conclusion, and I would have +filed the wrong one if I had reasoned instead of running it. + +**Early `return Err(Conflict)` inside an open transaction.** Three +`unchecked_transaction` sites exist, and `upsert_session` returns a revision +conflict at `session_store.rs:907` while its transaction is open. rusqlite's +`Transaction` implements `Drop` with `drop_behavior: DropBehavior::Rollback` +as the default, confirmed in the vendored source at +`rusqlite-0.40.2/src/transaction.rs:243`. The early return rolls the +transaction back and leaves the connection usable. This is correct as +written. + +**`git` subprocesses spawned while the session store mutex is held.** +`session_store.rs:872` calls `git_info_for`, which spawns up to three git +processes, and the caller at line 271 holds `store.conn`. This is a +documented, deliberately mitigated decision: +`src-tauri/src/fs/git.rs:21-25` states the cost, names both call sites, and +explains the three-second cache that bounds it. Reporting it as a fresh +defect would have been wrong. + +**A fifth drifted TS/Rust boundary.** The criterion in `docs/CONTRACTS.md` +is that both sides build independently and a shape has actually drifted. I +extracted every `invoke()` name the TypeScript sends (156 unique) and every +name registered in `generate_handler!` (169 unique) and compared them. Every +one of the 156 has a registration. There is no missing command, and no +invented one. I also checked the four largest payload structs in both +directions: `SessionUpsert` has `expectedRevision` supplied at +`src/lib/sessionStore.ts:378` and Rust rejects the call without it +(`session_store.rs:245`); `modelSettings` is validated as an object on the way +in at line 264 and normalized on the way out; `Settings.runtime` carry-over is +implemented at `settings.rs:320` and covered by tests including a corrupt-file +case at line 437. + +**`serde(default)` missing on `Option` fields.** Four structs have `Option` +fields without a per-field `#[serde(default)]`, which looked like the +documented drift shape. It is not, because serde treats an absent `Option` +key as `None` regardless. Measured: + +``` +missing Option -> Ok(None) +missing non-Option -> missing field `requiredCount` +Option wrong type -> invalid type: integer `42`, expected a string +``` + +`SearchOptions.include` and `.exclude` (`search.rs:21-22`) are the clearest +case, and `src/surfaces/SearchView.tsx:221` really does omit them while +`src/chrome/ProjectSearch.tsx:114` sends them as `null`. Both work. The +hazard is a wrong type or a missing non-`Option` field, and I found no +instance of either. + +**`NextStepActions` field drift after the jump-to-bottom removal.** +`settings.rs:170` still carries `jump_to_bottom` and `search_transcript`, both +of which the TypeScript no longer has as actions +(`src/lib/nextSteps.ts:23` keeps only `review-changes`). The struct has +`#[serde(default)]` at the type level, so the two absent keys are absorbed. +Correct as written, and the comment at `settings.rs:167` already explains why +the struct is flag-per-action rather than a count. + +**`kill(-pid)` against a reused process group.** `harness.rs:1172-1175` sends +a group signal, and a group id can in principle be reused after the child is +reaped. The window is guarded: `tree_alive` at line 1178 checks before +signalling, `kill_all` reaps before returning, and the spawn reservation at +line 362 prevents an in-flight spawn from racing. There are tests named for +these exact scenarios at lines 1870, 1967, 1992 and 2019. + +**`worktrees.rs` spawning `git` unprotected.** The brief flagged this file as +unprotected with its own sandbox semantics. It is protected: +`worktrees.rs:33` uses `crate::host::command("git")`. The brief's other +reference, `docs/plans/skills-catalog.md`, is not present on `origin/main`; it +exists only as an untracked file in a local working tree. + +**`Command::new` in production outside the routing helpers.** I enumerated +every spawn in production code after stripping test modules. What remains is +`host.rs` itself (the router), `harness.rs:279` (the provider binary, spawned +through the host path when sandboxed), the four clipboard sites in finding 2, +`pty.rs:567` in finding 4, and `checkpoint.rs:921` in finding 1. No others. + +**`unwrap()` reachable from IPC.** There are none in production. The five +`expect`s are on `include_str!` content parsed once behind a `OnceLock`, and +on `tauri::generate_context!`. A malformed contract artifact or a build +misconfiguration panics at startup, which is the correct time for it to +surface. + +## Coverage + +Read in full or near-full: `docs/CONTRACTS.md`, all 126 entries in +`docs/notes/tauri-boundary.md`, `contracts/rust-ipc.json`, `fs/secure.rs`, +`fs/path.rs`, `fs/git.rs` (constructor and helpers), `pty.rs` (spawn and fd +paths), `harness.rs` (lifecycle, signals, reaping), `control.rs`, +`session_store.rs` (structs, transactions, lock sites), `settings.rs`, +`checkpoint.rs` (stats and snapshot paths), `clipboard.rs`, +`external_url.rs`, `search.rs` (options and git-grep), `notes.rs` (structs), +`worktrees.rs` (constructor), `contract.rs`, and the TypeScript counterparts +in `src/lib/sessionStore.ts`, `src/lib/settings/store.ts`, +`src/lib/settings/schema.ts`, `src/lib/search.ts`, `src/lib/nextSteps.ts`, +`src/lib/clipboard.ts`, `src/lib/checkpoint.ts`, +`src/lib/harness/opencodeV2.ts`. + +Read partially: `fs/read.rs`, `fs/write.rs`, `fs/github.rs`, `gitlab.rs`, +`linear.rs`, `inbox_media.rs`, `cursor_store.rs`, `skills.rs`, `lib.rs`. + +Not covered: the V2 event catalog beyond what `opencodeV2.ts` already +consumes, which needs a running server to verify against the published schema. +`docs/CONTRACTS.md` already records two areas there as inferred and +unexercised, and I did not change that assessment. The test bodies in +`fs/tests.rs` were sampled by pattern rather than read one by one, so finding +coverage of specific tests is thinner than the other areas. diff --git a/src-tauri/src/checkpoint.rs b/src-tauri/src/checkpoint.rs index 48c4e312..04b34b96 100644 --- a/src-tauri/src/checkpoint.rs +++ b/src-tauri/src/checkpoint.rs @@ -1,11 +1,11 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::path::{Path, PathBuf}; -use std::process::Command; use std::sync::{Arc, Mutex}; use serde::{Deserialize, Serialize}; use tauri::{AppHandle, Manager, State}; +use crate::fs::git::git_cmd; #[cfg(test)] use crate::fs::git::GitDiffStats; use crate::fs::{ @@ -918,8 +918,8 @@ fn calculate_session_stats(dir: &Path, manifest: &Manifest, relative: &str) -> O } fn diff_numstat(before: &Path, after: &Path) -> Option<(i64, i64)> { - let mut cmd = Command::new("git"); - let output = cmd + // Nota: docs/notes/tauri-boundary.md#one-git-constructor + let output = git_cmd() .args(["diff", "--no-index", "--no-ext-diff", "--numstat", "--"]) .arg(before) .arg(after) diff --git a/src-tauri/src/clipboard.rs b/src-tauri/src/clipboard.rs index dd6798ab..dc8167e2 100644 --- a/src-tauri/src/clipboard.rs +++ b/src-tauri/src/clipboard.rs @@ -6,7 +6,7 @@ //! helper (`wl-clipboard` or `xclip`) instead of linking a protocol library, //! matching the repo preference for host-provided system integration. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; use crate::harness::resolve_gui_binary; @@ -105,20 +105,35 @@ fn uris_to_paths(payload: &str) -> Vec { .collect() } +/// The one clipboard-helper constructor, for the same reason `git_cmd` exists. +/// +/// These helpers are host binaries and inherit the app's `LD_LIBRARY_PATH`, +/// which on an AppImage build points at the bundled libraries. Those shadow the +/// host ones (`wl-copy` links libwayland-client, `xclip` links libX11) and the +/// helper dies with a symbol lookup error. The loop below reads that failure as +/// "this helper yielded" and tries the next one, so an exhausted list reports +/// no clipboard helper at all while wl-clipboard is installed and working. +fn helper_cmd(binary: &Path) -> Command { + // Nota: docs/notes/tauri-boundary.md#hide-bundled-libs + let mut cmd = crate::host::command(binary); + cmd.env_remove("LD_LIBRARY_PATH"); + cmd +} + fn clipboard_file_paths_sync() -> Vec { // Nota: docs/notes/tauri-boundary.md#every-helper-turn for helper in session_helpers(false) { for target in ["x-special/gnome-copied-files", "text/uri-list"] { let mut command = match &helper { Helper::WlPaste(path) => { - let mut command = Command::new(path); + let mut command = helper_cmd(path); if target == "text/uri-list" { command.arg("-t").arg(target); } command } Helper::Xclip(path) => { - let mut command = Command::new(path); + let mut command = helper_cmd(path); command .arg("-o") .arg("-selection") @@ -173,12 +188,12 @@ fn copy_file_to_clipboard_sync(path: &str) -> Result<(), String> { for helper in &helpers { let mut command = match helper { Helper::WlCopy(binary) => { - let mut command = Command::new(binary); + let mut command = helper_cmd(binary); command.arg("-t").arg("text/uri-list"); command } Helper::Xclip(binary) => { - let mut command = Command::new(binary); + let mut command = helper_cmd(binary); command .arg("-selection") .arg("clipboard") diff --git a/src-tauri/src/fs/git.rs b/src-tauri/src/fs/git.rs index 3ee52116..c6f1a365 100644 --- a/src-tauri/src/fs/git.rs +++ b/src-tauri/src/fs/git.rs @@ -1357,9 +1357,19 @@ pub(crate) fn resolve_repo_path(root: &Path, relative: &str) -> Result Command { // Nota: docs/notes/tauri-boundary.md#one-git-constructor - crate::host::command("git") + let mut cmd = crate::host::command("git"); + // Nota: docs/notes/tauri-boundary.md#hide-bundled-libs + // + // An AppImage build carries an LD_LIBRARY_PATH pointing at the bundled + // libraries, and those shadow the host ones a child `git` links against + // (libcurl, libssl, libz, libpcre2). The child then dies with a symbol + // lookup error, which for a diff is indistinguishable from "no change". + cmd.env_remove("LD_LIBRARY_PATH"); + cmd } pub(crate) fn git_checked(root: &Path, args: &[&str]) -> Result<(), String> { @@ -1810,3 +1820,72 @@ pub(crate) fn file_name(path: &Path) -> Option { .filter(|name| !name.is_empty()) .map(str::to_owned) } + +#[cfg(test)] +mod tests { + use super::*; + + /// An AppImage build carries an LD_LIBRARY_PATH pointing at the bundled + /// libraries, and those shadow the host ones a child `git` links against. + /// The spawn then dies with a symbol lookup error, which for a diff is + /// indistinguishable from "this file did not change". + #[test] + fn git_cmd_hides_the_bundled_library_path() { + let cmd = git_cmd(); + let envs: Vec<_> = cmd.get_envs().collect(); + let cleared = envs + .iter() + .any(|(key, value)| *key == "LD_LIBRARY_PATH" && value.is_none()); + assert!( + cleared, + "git_cmd must clear LD_LIBRARY_PATH so the host git keeps host libraries" + ); + } + + /// A git that refuses to run when it sees the library path, standing in for + /// the symbol lookup error. Proves the removal reaches the child's + /// environment, not just the `Command`. The stub reads the real variable, + /// so this fails against a bare `Command::new("git")`. + #[test] + #[cfg(unix)] + fn git_cmd_child_does_not_see_the_bundled_library_path() { + let dir = std::env::temp_dir().join(format!("monocode-git-cmd-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let fake = dir.join("git"); + std::fs::write( + &fake, + "#!/bin/sh\n\ + if [ -n \"$LD_LIBRARY_PATH\" ]; then exit 127; fi\n\ + echo ok\n", + ) + .unwrap(); + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&fake, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + + let path = format!( + "{}:{}", + dir.display(), + std::env::var("PATH").unwrap_or_default() + ); + // LD_LIBRARY_PATH is set on the process itself, the way an AppImage + // build does it, so the only thing that can stop the child seeing it is + // the removal inside `git_cmd`. Setting it with `.env()` here instead + // would land after the removal and win. + std::env::set_var("LD_LIBRARY_PATH", "/opt/monocode-bin/usr/lib"); + let output = git_cmd() + .env("PATH", path) + .arg("--version") + .output() + .unwrap(); + std::env::remove_var("LD_LIBRARY_PATH"); + assert!( + output.status.success(), + "child git inherited LD_LIBRARY_PATH and refused to run, which is the AppImage failure" + ); + + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/src-tauri/src/pty.rs b/src-tauri/src/pty.rs index 21caefff..da90d9f3 100644 --- a/src-tauri/src/pty.rs +++ b/src-tauri/src/pty.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::io::{Read, Write}; use std::os::unix::io::AsRawFd; +use std::process::Stdio; use std::sync::{Arc, Mutex}; use std::thread; use std::time::Duration; @@ -222,13 +223,19 @@ fn spawn_unix( let (shell, args) = default_shell(); let (master, slave) = open_pty(cols, rows)?; + // `open_pty` closes `master` on each of its own error paths. From here on + // both descriptors are bare i32s with no owner, so a `?` on the dup would + // leak them: the first `dup_stdio` succeeding and a later one failing + // (EMFILE) costs two descriptors per attempt, for the life of the process. + let stdio = attach_and_close_on_failure(slave, master)?; + // Nota: docs/notes/tauri-boundary.md#flatpak-shell-on-host let mut cmd = crate::host::command(&shell); cmd.args(&args) .current_dir(&workdir) - .stdin(dup_stdio(slave)?) - .stdout(dup_stdio(slave)?) - .stderr(dup_stdio(slave)?) + .stdin(stdio.0) + .stdout(stdio.1) + .stderr(stdio.2) .env("TERM", "xterm-256color") .env("COLORTERM", "truecolor") .env("COLORFGBG", "15;0") @@ -488,6 +495,39 @@ fn dup_stdio(fd: i32) -> Result { Ok(unsafe { std::process::Stdio::from_raw_fd(next) }) } +/// The three stdio copies the shell needs, resolved together. +/// +/// All three are attempted even after the first failure, so a partial result is +/// still owned here. Dropping a `Stdio` closes its descriptor, so returning the +/// `Ok` arms is what keeps an earlier success from leaking when a later dup +/// fails. +fn attach_stdio(slave: i32) -> Result<(Stdio, Stdio, Stdio), String> { + let stdin = dup_stdio(slave); + let stdout = dup_stdio(slave); + let stderr = dup_stdio(slave); + match (stdin, stdout, stderr) { + (Ok(stdin), Ok(stdout), Ok(stderr)) => Ok((stdin, stdout, stderr)), + _ => Err("Failed to attach the terminal to the shell".into()), + } +} + +/// `attach_stdio` plus the cleanup the caller would otherwise skip. +/// +/// `open_pty` hands back two bare `i32`s with no owner, and its own error paths +/// have already returned by the time these exist. A dup failure here is the one +/// remaining exit before the child owns copies of the slave, so this is the +/// place that has to close both. +fn attach_and_close_on_failure(slave: i32, master: i32) -> Result<(Stdio, Stdio, Stdio), String> { + match attach_stdio(slave) { + Ok(stdio) => Ok(stdio), + Err(error) => { + close_fd(master); + close_fd(slave); + Err(error) + } + } +} + fn set_cloexec(fd: i32) { unsafe { let flags = libc::fcntl(fd, libc::F_GETFD); @@ -563,8 +603,9 @@ fn foreground_label(master_fd: i32, shell_pid: u32) -> Option { } fn process_label(pid: i32) -> Option { - use std::process::Command; - let output = Command::new("ps") + // The shell is spawned through the host bridge, so its label has to be read + // the same way: inside a sandbox a local `ps` cannot see host processes. + let output = crate::host::command("ps") .args(["-p", &pid.to_string(), "-o", "args="]) .output() .ok()?; @@ -657,6 +698,44 @@ mod tests { assert!(pty_should_flush(1, PTY_COALESCE)); } + /// True when `fd` is still open in this process. + fn fd_is_open(fd: i32) -> bool { + unsafe { libc::fcntl(fd, libc::F_GETFD) >= 0 } + } + + /// A failed stdio attach has to close the pty descriptors, because + /// `open_pty` returns bare `i32`s with no owner and its own error paths + /// have already run by then. The old code bailed through `?` on the first + /// `dup_stdio` and left both open for the life of the process. + /// + /// `slave` is passed as -1 so all three dups fail without needing + /// EMFILE, which would mean changing a process-wide limit while other + /// tests run in parallel. + #[test] + fn a_failed_stdio_attach_closes_the_pty_descriptors() { + let (master, slave) = open_pty(80, 24).unwrap(); + assert!(fd_is_open(master) && fd_is_open(slave)); + assert!( + attach_and_close_on_failure(-1, master).is_err(), + "attaching stdio from an invalid descriptor must fail" + ); + assert!(!fd_is_open(master), "a failed attach leaked the master fd"); + } + + /// The success path must not close the slave: the child still needs it, and + /// `spawn_unix` closes it itself once the spawn returns. Guards the cleanup + /// above against over-reaching. + #[test] + fn a_successful_stdio_attach_leaves_the_pty_open() { + let (master, slave) = open_pty(80, 24).unwrap(); + let stdio = attach_stdio(slave).expect("attaching from a live pty must work"); + assert!(fd_is_open(master), "master must stay open for the reader"); + assert!(fd_is_open(slave), "slave must stay open for the spawn"); + drop(stdio); + close_fd(master); + close_fd(slave); + } + #[test] fn remove_if_generation_ignores_a_replaced_session() { let host = PtyHost::new(); From e2579d1184c8c3b83bef00b9874ce0a251b2572c Mon Sep 17 00:00:00 2001 From: yanhenrique-dev Date: Wed, 30 Sep 2026 00:58:06 -0300 Subject: [PATCH 2/3] test(git): restore LD_LIBRARY_PATH instead of dropping it The end-to-end guard against the AppImage library path sets the variable on the process and then removed it unconditionally. Run the suite from inside an AppImage and there is a real value there, so that remove would take it away from every later test that spawns a child -- the same failure this test exists to catch, caused by the test. The prior value is captured and put back, and only a variable that was absent is removed. --- src-tauri/src/fs/git.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/fs/git.rs b/src-tauri/src/fs/git.rs index c6f1a365..9f3e59a6 100644 --- a/src-tauri/src/fs/git.rs +++ b/src-tauri/src/fs/git.rs @@ -1874,13 +1874,20 @@ mod tests { // build does it, so the only thing that can stop the child seeing it is // the removal inside `git_cmd`. Setting it with `.env()` here instead // would land after the removal and win. + let previous = std::env::var_os("LD_LIBRARY_PATH"); std::env::set_var("LD_LIBRARY_PATH", "/opt/monocode-bin/usr/lib"); let output = git_cmd() .env("PATH", path) .arg("--version") .output() .unwrap(); - std::env::remove_var("LD_LIBRARY_PATH"); + // Put back whatever was there. Running the suite from inside an AppImage + // means there is a real value, and an unconditional `remove_var` would + // take it away from every test that spawns a child after this one. + match previous { + Some(value) => std::env::set_var("LD_LIBRARY_PATH", value), + None => std::env::remove_var("LD_LIBRARY_PATH"), + } assert!( output.status.success(), "child git inherited LD_LIBRARY_PATH and refused to run, which is the AppImage failure" From 9d294988ca896bdab899a480399d1189d27bfabb Mon Sep 17 00:00:00 2001 From: yanhenrique-dev Date: Wed, 30 Sep 2026 01:05:41 -0300 Subject: [PATCH 3/3] test(pty): assert on the descriptor's target, not on the number a_failed_stdio_attach_closes_the_pty_descriptors failed about 1 run in 7 with 'a failed attach leaked the master fd'. Nothing leaked. The test runs in parallel, and between the close and the check another test can be handed the same descriptor number, so fd_is_open reads a live fd belonging to someone else as ours. It now compares what the descriptor points at, through /proc/self/fd. A closed number with no taker reads as None and a reused one reads as something else, so both are caught and the reuse is not. Still fails when the cleanup is removed: verified. This also drops one source of the parallel flake the audit notes mention; the others are pre-existing and not this PR's to fix. --- src-tauri/src/pty.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/pty.rs b/src-tauri/src/pty.rs index da90d9f3..c7d572d2 100644 --- a/src-tauri/src/pty.rs +++ b/src-tauri/src/pty.rs @@ -703,6 +703,18 @@ mod tests { unsafe { libc::fcntl(fd, libc::F_GETFD) >= 0 } } + /// What an open descriptor points at, so a closed one can be told from a + /// number another test has since reused. + /// + /// `fd_is_open` alone is not enough here: these tests run in parallel, and + /// between the close and the check another test can be handed the same + /// number, which reads as "still open" and fails the run at random. The + /// target does not collide that way, because a reused number points at + /// something else. + fn fd_target(fd: i32) -> Option { + std::fs::read_link(format!("/proc/self/fd/{fd}")).ok() + } + /// A failed stdio attach has to close the pty descriptors, because /// `open_pty` returns bare `i32`s with no owner and its own error paths /// have already run by then. The old code bailed through `?` on the first @@ -715,11 +727,19 @@ mod tests { fn a_failed_stdio_attach_closes_the_pty_descriptors() { let (master, slave) = open_pty(80, 24).unwrap(); assert!(fd_is_open(master) && fd_is_open(slave)); + let master_target = fd_target(master); assert!( attach_and_close_on_failure(-1, master).is_err(), "attaching stdio from an invalid descriptor must fail" ); - assert!(!fd_is_open(master), "a failed attach leaked the master fd"); + // The descriptor number may well be open again by now, held by another + // test running in parallel. What must not survive is *our* pty still + // being reachable through it. + assert_ne!( + fd_target(master), + master_target, + "a failed attach left the master fd on the pty", + ); } /// The success path must not close the slave: the child still needs it, and