Skip to content

Commit c763956

Browse files
committed
fix(upgrade): remove child-process mmap probe, use direct try/catch
Compiled Bun binaries don't support the -e flag, so spawning process.execPath -e '...' re-runs the main program instead of evaluating the expression. This made probeMmapSafe() always return false in production, negating the mmap optimization. The probe was unnecessary: mmap on a temp copy is safe because the copy is a regular file (separate inode), not a running binary. ETXTBSY (Linux) and AMFI SIGKILL (macOS) only affect the running binary's inode. Simplify to direct copy → mmap with try/catch fallback to arrayBuffer(). Addresses Seer review comment on PR #344.
1 parent d6a3076 commit c763956

File tree

2 files changed

+18
-69
lines changed

2 files changed

+18
-69
lines changed

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -666,7 +666,7 @@ mock.module("./some-module", () => ({
666666
* **Install script: BSD sed and awk JSON parsing breaks OCI digest extraction**: The install script parses OCI manifests with awk (no jq). Key trap: BSD sed \`\n\` is literal, not newline. Fix: single awk pass tracking last-seen \`"digest"\`, printing when \`"org.opencontainers.image.title"\` matches target. The config digest (\`sha256:44136fa...\`) is a 2-byte \`{}\` blob — downloading it instead of the real binary causes \`gunzip: unexpected end of file\`.
667667
668668
<!-- lore:019cb963-cb63-722d-9365-b34336f4766d -->
669-
* **macOS SIGKILL on MAP\_SHARED mmap of signed Mach-O binaries**: \`Bun.mmap()\` always opens files with \`PROT\_WRITE\`/\`O\_RDWR\` regardless of the \`shared\` flag. This fails on the running binary: macOS sends uncatchable SIGKILL (AMFI rejects writable mappings on signed Mach-O), Linux returns ETXTBSY (kernel blocks opening running executables for write). The platform-conditional fix (PR #340: darwin→arrayBuffer, else→mmap) was insufficient — mmap fails on BOTH platforms. Fixed by using \`new Uint8Array(await Bun.file(oldPath).arrayBuffer())\` unconditionally in bspatch.ts. Costs ~100 MB heap for the old binary but is the only approach that works cross-platform.
669+
* **macOS SIGKILL on MAP\_SHARED mmap of signed Mach-O binaries**: \`Bun.mmap()\` always opens files with \`PROT\_WRITE\`/\`O\_RDWR\` regardless of the \`shared\` flag. This fails on the running binary: macOS sends uncatchable SIGKILL (AMFI rejects writable mappings on signed Mach-O), Linux returns ETXTBSY (kernel blocks opening running executables for write). Fix: copy-then-mmap — copy the running binary to a temp file, then mmap the copy (separate inode, no AMFI/ETXTBSY restrictions). Falls back to \`arrayBuffer()\` if copy/mmap fails. Do NOT use child-process probe (\`process.execPath -e ...\`) — compiled Bun binaries don't support \`-e\`, causing the child to rerun the main program.
670670
671671
<!-- lore:019c969a-1c90-7041-88a8-4e4d9a51ebed -->
672672
* **Multiple mockFetch calls replace each other — use unified mocks for multi-endpoint tests**: Bun test mocking gotchas: (1) \`mockFetch()\` replaces \`globalThis.fetch\` — calling it twice replaces the first mock. Use a single unified fetch mock dispatching by URL pattern. (2) \`mock.module()\` pollutes the module registry for ALL subsequent test files. Tests using it must live in \`test/isolated/\` and run via \`test:isolated\`. (3) For \`Bun.spawn\`, use direct property assignment in \`beforeEach\`/\`afterEach\`.

src/lib/bspatch.ts

Lines changed: 17 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,7 @@
66
* minimal memory usage during CLI self-upgrades:
77
*
88
* - Old binary: copy-then-mmap for 0 JS heap (CoW on btrfs/xfs/APFS),
9-
* with child-process probe for mmap safety (macOS AMFI sends uncatchable
10-
* SIGKILL on signed Mach-O), falling back to `arrayBuffer()` if unsafe
9+
* falling back to `arrayBuffer()` if copy/mmap fails
1110
* - Diff/extra blocks: streamed via `DecompressionStream('zstd')`
1211
* - Output: written incrementally to disk via `Bun.file().writer()`
1312
* - Integrity: SHA-256 computed inline via `Bun.CryptoHasher`
@@ -228,88 +227,38 @@ type OldFileHandle = {
228227
cleanup: () => void;
229228
};
230229

231-
/**
232-
* Probe whether `Bun.mmap()` works on a file by spawning a child process.
233-
*
234-
* macOS AMFI sends uncatchable SIGKILL when a process creates a writable
235-
* memory mapping of a code-signed Mach-O binary — even a copy. Since
236-
* SIGKILL terminates the process inside the syscall (before any catch
237-
* block runs), we cannot detect the failure in-process.
238-
*
239-
* This probe spawns a minimal child that attempts `Bun.mmap()` on the
240-
* target file. If the child exits 0, mmap is safe. If it's killed
241-
* (SIGKILL) or exits non-zero, we fall back to `arrayBuffer()`.
242-
*
243-
* @param filePath - Path to the file to probe (should be the temp copy)
244-
* @returns true if mmap is safe on this file, false otherwise
245-
*/
246-
async function probeMmapSafe(filePath: string): Promise<boolean> {
247-
try {
248-
const child = Bun.spawn(
249-
[
250-
process.execPath,
251-
"-e",
252-
`Bun.mmap(${JSON.stringify(filePath)}, { shared: false })`,
253-
],
254-
{ stdout: "ignore", stderr: "ignore" }
255-
);
256-
const exitCode = await child.exited;
257-
return exitCode === 0;
258-
} catch {
259-
return false;
260-
}
261-
}
262-
263230
/**
264231
* Load the old binary for read access during patching.
265232
*
266-
* Strategy: copy to temp file, probe mmap safety via child process,
267-
* then mmap the copy if safe. This avoids `Bun.mmap()` on files that
268-
* would trigger uncatchable SIGKILL (macOS AMFI on signed Mach-O) while
269-
* keeping zero JS heap on platforms where mmap works. On CoW filesystems
270-
* (btrfs, xfs, APFS) the copy is a metadata-only reflink (near-instant).
233+
* Strategy: copy to temp file, then try mmap on the copy. The copy is a
234+
* regular file (no running process), so `Bun.mmap()` succeeds on both
235+
* Linux and macOS — ETXTBSY (Linux) and AMFI SIGKILL (macOS) only affect
236+
* the running binary's inode, not a copy. On CoW filesystems (btrfs, xfs,
237+
* APFS) the copy is a metadata-only reflink (near-instant).
271238
*
272-
* Falls back to `Bun.file().arrayBuffer()` (~100 MB heap) if the probe
273-
* fails or if copy/mmap errors for any reason.
239+
* Falls back to `Bun.file().arrayBuffer()` (~100 MB heap) if copy or
240+
* mmap fails for any reason.
274241
*/
275242
async function loadOldBinary(oldPath: string): Promise<OldFileHandle> {
276243
const tempCopy = join(tmpdir(), `sentry-patch-old-${process.pid}`);
277244
try {
278245
copyFileSync(oldPath, tempCopy);
279246

280-
// Probe mmap safety in a child process — macOS AMFI sends uncatchable
281-
// SIGKILL on writable mappings of signed Mach-O, even for copies.
282-
const mmapSafe = await probeMmapSafe(tempCopy);
283-
284-
if (mmapSafe) {
285-
const data = Bun.mmap(tempCopy, { shared: false });
286-
return {
287-
data,
288-
cleanup: () => {
289-
try {
290-
unlinkSync(tempCopy);
291-
} catch {
292-
// Best-effort cleanup — OS will reclaim on reboot
293-
}
294-
},
295-
};
296-
}
297-
298-
// mmap probe failed — read copy into JS heap, then clean up temp file
299-
const data = new Uint8Array(await Bun.file(tempCopy).arrayBuffer());
300-
try {
301-
unlinkSync(tempCopy);
302-
} catch {
303-
// Best-effort cleanup
304-
}
247+
// mmap the copy — safe because it's a separate inode, not the running
248+
// binary. MAP_PRIVATE avoids write-back to disk.
249+
const data = Bun.mmap(tempCopy, { shared: false });
305250
return {
306251
data,
307252
cleanup: () => {
308-
// Data is in JS heap — no temp file to clean up
253+
try {
254+
unlinkSync(tempCopy);
255+
} catch {
256+
// Best-effort cleanup — OS will reclaim on reboot
257+
}
309258
},
310259
};
311260
} catch {
312-
// Copy failed — fall back to reading original directly into JS heap
261+
// Copy or mmap failed — fall back to reading into JS heap
313262
try {
314263
unlinkSync(tempCopy);
315264
} catch {

0 commit comments

Comments
 (0)