From 9f17f16c662d1a06131d0f358589a79df4a9f0a0 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 28 Jun 2026 01:43:03 -0700 Subject: [PATCH] fix(environments): use $BASHPID for atomic snapshot temp + harden failure path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The atomic mv approach (kyssta-exe's commit) narrows but does not close the #38249 race: the temp name used $$ (parent shell PID), which is identical across &-launched concurrent subshells. Two concurrent writers pick the same temp file, clobber each other mid-write, and mv then publishes a torn snapshot — a reader sourcing it absorbs declare-x/export fragments into PATH. - Use $BASHPID (actual per-subshell PID) so concurrent writers never collide. - Chain mv on export success (&&) and rm the temp on failure so a partial dump never replaces a good snapshot; apply the same to the init_session bootstrap. - shlex-quote the static temp-path portion (Windows/spaces), $BASHPID outside. - LocalEnvironment.cleanup sweeps orphaned snap.tmp.* temps. - Regression tests: string-shape + a behavioral concurrent writers/readers test that proves the snapshot never tears (would still tear with $$). --- tests/tools/test_base_environment.py | 160 +++++++++++++++++++++++++++ tools/environments/base.py | 51 ++++++--- tools/environments/local.py | 11 ++ 3 files changed, 207 insertions(+), 15 deletions(-) diff --git a/tests/tools/test_base_environment.py b/tests/tools/test_base_environment.py index 88fa6a7ea0f0c..bfa20fc5ac17c 100644 --- a/tests/tools/test_base_environment.py +++ b/tests/tools/test_base_environment.py @@ -90,6 +90,166 @@ class TestWrapCommand: assert "exit 126" in wrapped +class TestAtomicSnapshotWrite: + """Regression for #38249: concurrent terminal calls in one session both + source AND rewrite the shared env snapshot. A non-atomic ``export -p > + snap`` truncates-then-writes in place, so a concurrent ``source snap`` can + read a half-written file and embed ``declare -x``/``export`` fragments into + PATH, breaking ``ls``/``git``/``tr`` with command-not-found. The write must + assemble in a temp file and ``mv -f`` it into place (mv is atomic on POSIX + same-fs), so a reader sees the old-or-new complete file, never a torn one. + """ + + def test_wrap_command_uses_atomic_temp_then_mv(self): + env = _TestableEnv() + env._snapshot_ready = True + wrapped = env._wrap_command("echo hi", "/tmp") + # Env dump goes to a temp file, not directly over the live snapshot. + assert "export -p > " in wrapped + assert ".tmp." in wrapped + # Then an atomic rename onto the real snapshot path. + assert "mv -f " in wrapped + # The env-dump must NOT write the live snapshot in place (the bug). + snap = env._snapshot_path + assert f"export -p > {snap} " not in wrapped + assert f"export -p > '{snap}'" not in wrapped + + def test_temp_path_uses_bashpid_not_dollardollar(self): + """The temp name MUST use ``$BASHPID`` (the real subshell PID), not + ``$$``. In ``&``-launched concurrent subshells ``$$`` stays the parent + shell's PID, so two writers would pick the same temp name, clobber each + other mid-write, and mv would publish a torn file — the corruption is + only narrowed, not closed. This is the bug shared by every prior PR in + the #38249 cluster.""" + env = _TestableEnv() + env._snapshot_ready = True + wrapped = env._wrap_command("echo hi", "/tmp") + assert "$BASHPID" in wrapped + # The bare $$ temp form must be gone. + assert ".tmp.$$" not in wrapped + + def test_temp_path_static_part_is_quoted_bashpid_outside(self): + """The static path portion must be shlex-quoted (Windows/Git-Bash + ``C:/Users/...`` or spaces) while ``$BASHPID`` stays OUTSIDE the quotes + so it still expands.""" + env = _TestableEnv() + env._snapshot_ready = True + env._snapshot_path = "/tmp/has space/hermes-snap-x.sh" + wrapped = env._wrap_command("echo hi", "/tmp") + # The static path (with its space) is shlex-quoted as a single word, with + # $BASHPID appended OUTSIDE the quotes so it still expands at runtime. + assert "'/tmp/has space/hermes-snap-x.sh.tmp.'$BASHPID" in wrapped + # The space must never appear bare/unquoted in the temp token (that would + # word-split into two args and break the redirect/mv). + assert " space/hermes-snap-x.sh.tmp.$BASHPID" not in wrapped + + def test_wrap_command_mv_chained_on_export_success(self): + """A failed/partial ``export -p`` must NOT mv a torn temp over a good + snapshot. The mv is chained with ``&&`` on the export, and the temp is + removed on failure.""" + env = _TestableEnv() + env._snapshot_ready = True + wrapped = env._wrap_command("echo hi", "/tmp") + assert "export -p > " in wrapped and "&& mv -f " in wrapped + assert "rm -f " in wrapped # temp cleanup on failure + + def test_init_session_bootstrap_also_atomic_and_bashpid(self): + """The init_session bootstrap (first snapshot write) is the same shared + file a concurrent command could source — it must be atomic and use + ``$BASHPID`` too.""" + env = _TestableEnv() + captured = {} + + def fake_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None): + captured["cmd"] = cmd_string + raise RuntimeError("stop after capture") # we only need the script + + env._run_bash = fake_run_bash # type: ignore[assignment] + try: + env.init_session() + except Exception: + pass + boot = captured.get("cmd", "") + assert ".tmp." in boot and "mv -f " in boot, boot + assert "$BASHPID" in boot + assert ".tmp.$$" not in boot + + +class TestAtomicSnapshotConcurrencyBehavioral: + """Behavioral regression for #38249 — actually EXECUTES the generated + snapshot write/read concurrently and asserts the file never tears. + + The string-inspection tests prove the right script is emitted; this proves + the emitted script's guarantee holds under real concurrency: N concurrent + writers + readers, and the snapshot is ALWAYS a complete, parseable env + dump — never truncated mid-line with a ``declare -x`` / ``export`` fragment + that would corrupt PATH. Crucially it uses ``$BASHPID`` (per-subshell + unique), which is what closes the race; ``$$`` would still tear here. + """ + + def _run(self, script): + import subprocess + return subprocess.run(["/bin/bash", "-c", script], capture_output=True, text=True) + + def test_concurrent_writes_never_tear_the_snapshot(self, tmp_path): + import shutil + if not shutil.which("bash"): + import pytest + pytest.skip("bash required") + import shlex + snap = str(tmp_path / "hermes-snap-x.sh") + _q = shlex.quote + _snap_tmp = _q(snap + ".tmp.") + "$BASHPID" + # One writer iteration = the exact atomic sequence _wrap_command emits. + writer = ( + "for i in $(seq 1 80); do " + "export BIG_$i=$(head -c 600 /dev/zero | tr '\\0' x); " + f"{{ export -p > {_snap_tmp} && mv -f {_snap_tmp} {_q(snap)}; }} " + f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true; " + "done" + ) + # Reader: repeatedly source the snapshot and check PATH never absorbs + # an `export `/`declare -x` fragment (the corruption signature). + reader = ( + "export PATH=/usr/bin:/bin; " + "for i in $(seq 1 160); do " + f"( source {_q(snap)} >/dev/null 2>&1 || true; " + "case \"$PATH\" in *'declare -x'*|*'export '*) echo CORRUPT;; esac ); " + "done" + ) + self._run(f"export -p > {_q(snap)}") # seed a valid snapshot + # 4 concurrent writers + 4 readers, repeated. + w = " & ".join([writer] * 4) + r = " & ".join([reader] * 4) + procs = [self._run(f"{w} & {r} & wait") for _ in range(3)] + corrupt = any("CORRUPT" in p.stdout for p in procs) + assert not corrupt, "snapshot tore — PATH absorbed a declare-x/export fragment" + final = self._run(f"source {_q(snap)} >/dev/null 2>&1 && echo OK || echo BROKEN") + assert "OK" in final.stdout, f"final snapshot not sourceable: {final.stdout} {final.stderr}" + + def test_failed_export_does_not_destroy_good_snapshot(self, tmp_path): + """If ``export -p`` fails, the ``&&``-chained mv must NOT clobber the + existing good snapshot.""" + import shutil + if not shutil.which("bash"): + import pytest + pytest.skip("bash required") + import shlex + snap = str(tmp_path / "snap.sh") + _q = shlex.quote + self._run(f"echo 'export GOOD=1' > {_q(snap)}") # seed good snapshot + # Redirect export into an unwritable dir so the export side fails; mv + # must then NOT run (&&) and not clobber snap. + bad_tmp = _q("/nonexistent-dir/snap.tmp.") + "$BASHPID" + script = ( + f"{{ export -p > {bad_tmp} && mv -f {bad_tmp} {_q(snap)}; }} " + f"2>/dev/null || rm -f {bad_tmp} 2>/dev/null || true" + ) + self._run(script) + out = self._run(f"cat {_q(snap)}") + assert "export GOOD=1" in out.stdout, "good snapshot was destroyed by a failed export" + + class TestExtractCwdFromOutput: def test_happy_path(self): env = _TestableEnv() diff --git a/tools/environments/base.py b/tools/environments/base.py index d782e5691d235..409fec71d8bf6 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -371,14 +371,24 @@ class BaseEnvironment(ABC): # backends) into every terminal-tool response. _quoted_snap = shlex.quote(self._snapshot_path) _quoted_cwd_file = shlex.quote(self._cwd_file) - # Use atomic file replacement: write to a temp file, then mv to the - # final path. This prevents concurrent source() calls from reading a - # half-written snapshot when another terminal command finishes and - # rewrites the env vars (issue #38249). `mv` is atomic on POSIX - # when src and dest are on the same filesystem, so source() will - # either see the old complete snapshot or the new complete one — - # never a partial/truncated file. - _snap_tmp = f"{self._snapshot_path}.tmp.$$" + # Use atomic file replacement: assemble the snapshot in a temp file, + # then mv it over the final path. This prevents concurrent source() + # calls from reading a half-written snapshot when another terminal + # command finishes and rewrites the env vars (issue #38249). `mv` is + # atomic on POSIX when src and dest are on the same filesystem, so + # source() either sees the old complete snapshot or the new complete + # one — never a partial/truncated file. + # + # The temp name MUST be unique per concurrent writer. ``$$`` is the + # bash PID, but in ``&``-launched subshells (how concurrent terminal + # calls run) ``$$`` stays the *parent* shell's PID — so two concurrent + # writers would pick the SAME temp name, clobber each other's temp + # mid-write, and mv would then publish a torn file (the corruption is + # only narrowed, not closed). ``$BASHPID`` is the actual subshell PID + # and is genuinely unique per writer, which closes the race. The + # static path is shlex-quoted (Windows/Git-Bash drive letters, spaces) + # with ``$BASHPID`` left outside the quotes so it still expands. + _snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$BASHPID" bootstrap = ( f"export -p > {_snap_tmp}\n" f"declare -f | grep -vE '^_[^_]' >> {_snap_tmp}\n" @@ -386,7 +396,9 @@ class BaseEnvironment(ABC): f"echo 'shopt -s expand_aliases' >> {_snap_tmp}\n" f"echo 'set +e' >> {_snap_tmp}\n" f"echo 'set +u' >> {_snap_tmp}\n" - f"mv -f {_snap_tmp} {_quoted_snap}\n" + # Publish atomically only if assembly succeeded; otherwise drop the + # partial temp rather than leave it to be sourced or orphaned. + f"mv -f {_snap_tmp} {_quoted_snap} || rm -f {_snap_tmp}\n" f"builtin cd {_quoted_cwd} 2>/dev/null || true\n" f"pwd -P > {_quoted_cwd_file} 2>/dev/null || true\n" f"printf '\\n{self._cwd_marker}%s{self._cwd_marker}\\n' \"$(pwd -P)\"\n" @@ -437,9 +449,13 @@ class BaseEnvironment(ABC): _quoted_snap = shlex.quote(self._snapshot_path) _quoted_cwd_file = shlex.quote(self._cwd_file) # Use atomic file replacement for env snapshot updates (issue #38249). - # Write to a temp file, then mv to atomically replace the snapshot so - # concurrent source() calls never read a truncated/half-written file. - _snap_tmp = f"{self._snapshot_path}.tmp.$$" + # Assemble into a per-writer-unique temp file, then mv to atomically + # replace the snapshot so concurrent source() calls never read a + # truncated/half-written file. ``$BASHPID`` (not ``$$``) is the actual + # subshell PID — unique per concurrent ``&``-launched writer — so two + # writers never share a temp name and clobber each other before the mv. + # Static path shlex-quoted (Windows/spaces); ``$BASHPID`` left to expand. + _snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$BASHPID" parts = [] @@ -464,10 +480,15 @@ class BaseEnvironment(ABC): parts.append(f"eval '{escaped}'") parts.append("__hermes_ec=$?") - # Re-dump env vars to snapshot (atomic replacement to avoid races) + # Re-dump env vars to snapshot (atomic replacement to avoid races). + # Chain mv on the export succeeding so a failed/partial dump never + # replaces a good snapshot; drop the temp on failure so it isn't + # orphaned (cleaned up wholesale in LocalEnvironment.cleanup too). if self._snapshot_ready: - parts.append(f"export -p > {_snap_tmp} 2>/dev/null || true") - parts.append(f"mv -f {_snap_tmp} {_quoted_snap} 2>/dev/null || true") + parts.append( + f"{{ export -p > {_snap_tmp} && mv -f {_snap_tmp} {_quoted_snap}; }} " + f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true" + ) # Write CWD to file (local reads this) and stdout marker (remote parses this) parts.append(f"pwd -P > {_quoted_cwd_file} 2>/dev/null || true") diff --git a/tools/environments/local.py b/tools/environments/local.py index 3e968d37ea045..50a2522a5a226 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -987,3 +987,14 @@ class LocalEnvironment(BaseEnvironment): os.unlink(f) except OSError: pass + # Remove any orphaned atomic-write temp snapshots (snap.tmp.) + # a failed/interrupted mv could have left behind (#38249). + try: + import glob + for tmp in glob.glob(f"{self._snapshot_path}.tmp.*"): + try: + os.unlink(tmp) + except OSError: + pass + except Exception: + pass