fix(environments): use $BASHPID for atomic snapshot temp + harden failure path
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 $$).
This commit is contained in:
Teknium
parent
6a2958a521
commit
9f17f16c66
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -987,3 +987,14 @@ class LocalEnvironment(BaseEnvironment):
|
|||
os.unlink(f)
|
||||
except OSError:
|
||||
pass
|
||||
# Remove any orphaned atomic-write temp snapshots (snap.tmp.<bashpid>)
|
||||
# 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
|
||||
|
|
|
|||
Loading…
Reference in New Issue