hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 33 Packages Projects Releases Wiki Activity

fix(tools): allocate snapshot temp paths with mktemp instead of $BASHPID

Browse Source 
Extracted from #54314 (@flag0x369), re-derived onto current main: macOS
ships bash 3.2 as /bin/bash, which lacks $BASHPID entirely — the
variable expands to empty string, collapsing every concurrent writer's
'unique' temp path onto the same file (torn snapshot writes under
concurrency). mktemp allocates per-writer unique paths portably.
Live-verified: /bin/bash -c 'echo $BASHPID' prints empty on this box.
This commit is contained in:
f1aggo_macair
2026-08-03 13:25:49 +05:30
committed by kshitij
parent 0125281609
commit 911d380296
3 changed files with 61 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
tests/tools/test_base_environment.py
View File

@@ -115,25 +115,27 @@ class TestAtomicSnapshotWrite:
assert f"> '{snap}'" not in wrapped
assert f"> {snap}\n" 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."""
def test_temp_path_uses_mktemp_not_pid_variables(self):
"""The temp name MUST be allocated by ``mktemp`` — never ``$$`` (in
``&``-launched concurrent subshells it stays the parent shell's PID, so
two writers would pick the same temp name and publish a torn file) and
never ``$BASHPID`` (macOS ships bash 3.2, which lacks it — the name
expands empty, collapsing every writer onto one temp path and
reopening the #38249 race). Regression for PR #54314."""
env = _TestableEnv()
env._snapshot_ready = True
wrapped = env._wrap_command("echo hi", "/tmp")
assert "$BASHPID" in wrapped
assert "mktemp " in wrapped
assert ".tmp.XXXXXXXXXX" in wrapped
assert "$BASHPID" not in wrapped
# The bare $$ temp form must be gone.
assert ".tmp.$$" not in wrapped
def test_init_session_bootstrap_also_atomic_and_bashpid(self):
def test_init_session_bootstrap_also_atomic_and_mktemp(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."""
``mktemp`` too (no ``$BASHPID``: absent on macOS bash 3.2)."""
env = _TestableEnv()
captured = {}
@@ -148,7 +150,8 @@ class TestAtomicSnapshotWrite:
pass
boot = captured.get("cmd", "")
assert ".tmp." in boot and "mv -f " in boot, boot
assert "$BASHPID" in boot
assert "mktemp " in boot
assert "$BASHPID" not in boot
assert ".tmp.$$" not in boot
@@ -178,8 +181,9 @@ class TestAtomicSnapshotConcurrencyBehavioral:
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.
that would corrupt PATH. Crucially it allocates the temp with ``mktemp``
(per-writer unique, works on macOS bash 3.2 which lacks ``$BASHPID``),
which is what closes the race; ``$$`` would still tear here.
"""
def _run(self, script):
@@ -194,13 +198,14 @@ class TestAtomicSnapshotConcurrencyBehavioral:
import shlex
snap = str(tmp_path / "hermes-snap-x.sh")
_q = shlex.quote
_snap_tmp = _q(snap + ".tmp.") + "$BASHPID"
_tmpl = _q(snap + ".tmp.XXXXXXXXXX")
# 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; "
f"__hermes_snap_tmp=$(mktemp {_tmpl}) && "
f"{{ export -p > \"$__hermes_snap_tmp\" && mv -f \"$__hermes_snap_tmp\" {_q(snap)}; }} "
f"2>/dev/null || rm -f \"$__hermes_snap_tmp\" 2>/dev/null || true; "
"done"
)
# Reader: repeatedly source the snapshot and check PATH never absorbs
@@ -235,10 +240,11 @@ class TestAtomicSnapshotConcurrencyBehavioral:
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"
bad_tmp = _q("/nonexistent-dir/snap.tmp.XXXXXXXXXX")
script = (
f"{{ export -p > {bad_tmp} && mv -f {bad_tmp} {_q(snap)}; }} "
f"2>/dev/null || rm -f {bad_tmp} 2>/dev/null || true"
f"__hermes_snap_tmp=$(mktemp {bad_tmp}) && "
f"{{ export -p > \"$__hermes_snap_tmp\" && mv -f \"$__hermes_snap_tmp\" {_q(snap)}; }} "
f"2>/dev/null || rm -f \"$__hermes_snap_tmp\" 2>/dev/null || true"
)
self._run(script)
out = self._run(f"cat {_q(snap)}")

15
tests/tools/test_snapshot_session_id_leak.py
View File

@@ -42,7 +42,7 @@ def test_regex_matches_bridged_session_vars():
def test_export_snippet_shape():
snippet = _export_dump_excluding_session_vars("/tmp/snap.tmp.$BASHPID")
snippet = _export_dump_excluding_session_vars('"$__hermes_snap_tmp"')
assert "export -p" in snippet
# Unset-by-name (not line-grep): multi-line declare values must not leave
# continuation lines in the snapshot (issue #71296).
@@ -51,15 +51,16 @@ def test_export_snippet_shape():
assert "${!HERMES_CRON_AUTO_DELIVER_*}" in snippet
assert "HERMES_UI_SESSION_ID" in snippet
assert "grep -vE" not in snippet
assert "/tmp/snap.tmp.$BASHPID" in snippet
assert '"$__hermes_snap_tmp"' in snippet
# The redirection must be attached to a brace group wrapping the dump,
# NOT to a pipeline segment: a redirect on a pipeline segment expands
# $BASHPID inside that segment's subshell (a different PID than the parent
# that expands the follow-up ``mv`` operand), silently orphaning the dump
# and breaking snapshot env persistence entirely.
# NOT to a pipeline segment: a redirect on a pipeline segment expands the
# temp-path variable inside that segment's subshell (potentially
# inconsistently with the parent that expands the follow-up ``mv``
# operand), silently orphaning the dump and breaking snapshot env
# persistence entirely.
assert snippet.lstrip().startswith("{ ")
assert "|| true; }" in snippet
assert snippet.rstrip().endswith("> /tmp/snap.tmp.$BASHPID")
assert snippet.rstrip().endswith('> "$__hermes_snap_tmp"')
# ---------------------------------------------------------------------------

49
tools/environments/base.py
View File

@@ -491,12 +491,12 @@ def _export_dump_excluding_session_vars(
lines. ``|| true`` keeps the success contract for callers that chain on it.
The dump MUST be wrapped in a brace group with the redirection applied to
the group. *tmp_path* typically embeds ``$BASHPID`` for concurrency-safe
temp names; a redirection attached to a pipeline segment would expand
``$BASHPID`` inside that segment's subshell (a different PID than the
parent that expands the follow-up ``mv``), silently orphaning the dump.
The brace-group redirect is expanded in the current shell, keeping both
expansions consistent.
the group. *tmp_path* is typically a shell-variable expansion (a
mktemp-allocated per-writer temp name); a redirection attached to a
pipeline segment would expand it inside that segment's subshell,
potentially inconsistently with the parent that expands the follow-up
``mv``. The brace-group redirect is expanded in the current shell,
keeping both expansions consistent.
"""
# ${!PREFIX*} is bash 3.2+ name-prefix expansion; empty matches are fine
# because ``unset`` with only missing names is ignored under 2>/dev/null.
@@ -668,14 +668,19 @@ class BaseEnvironment(ABC):
# 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 shell-quoted (Windows/Git-Bash drive letters, spaces)
# with ``$BASHPID`` left outside the quotes so it still expands.
_snap_tmp = self._quote_shell_path(self._snapshot_path + ".tmp.") + "$BASHPID"
# only narrowed, not closed). ``$BASHPID`` would be unique per writer,
# but macOS ships bash 3.2 which does NOT provide it — the name expands
# empty there, so every writer shares one temp path and the race is
# back. ``mktemp`` allocates a per-writer unique path portably across
# bash versions. The template is shell-quoted (Windows/Git-Bash drive
# letters, spaces) and the resulting path lives in a shell variable so
# every later expansion is consistent.
_snap_tmp_template = self._quote_shell_path(self._snapshot_path + ".tmp.XXXXXXXXXX")
_snap_tmp = '"$__hermes_snap_tmp"'
snapshot_excluded = self._snapshot_excluded_passthrough_names()
bootstrap = (
f"umask 077\n"
f"__hermes_snap_tmp=$(mktemp {_snap_tmp_template}) || exit 1\n"
f"{_export_dump_excluding_session_vars(_snap_tmp, snapshot_excluded)}\n"
# Dump function definitions, filtering out private (``_``-prefixed)
# helpers — mainly bash-completion internals (``_git``, ``_make``…)
@@ -784,11 +789,13 @@ class BaseEnvironment(ABC):
# Use atomic file replacement for env snapshot updates (issue #38249).
# 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 shell-quoted (Windows/spaces); ``$BASHPID`` left to expand.
_snap_tmp = self._quote_shell_path(self._snapshot_path + ".tmp.") + "$BASHPID"
# truncated/half-written file. ``mktemp`` is used instead of
# ``$BASHPID``/``$$`` because macOS bash 3.2 lacks ``$BASHPID`` (it
# expands empty, collapsing every writer onto one temp name) and ``$$``
# is shared by ``&``-launched subshells. Template shell-quoted
# (Windows/spaces); the allocated path lives in a shell variable.
_snap_tmp_template = self._quote_shell_path(self._snapshot_path + ".tmp.XXXXXXXXXX")
_snap_tmp = '"$__hermes_snap_tmp"'
parts = []
passthrough_names = self._snapshot_excluded_passthrough_names()
@@ -842,13 +849,13 @@ class BaseEnvironment(ABC):
# 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).
# NOTE: the redirection must be attached to a brace group — ``_snap_tmp``
# embeds ``$BASHPID``, and a redirect on a pipeline segment expands
# inside that segment's subshell (a different PID than the parent that
# expands the ``mv`` operand), silently orphaning the dump. See
# _export_dump_excluding_session_vars.
# NOTE: the temp path is allocated with mktemp into a shell variable
# first — the redirection inside _export_dump_excluding_session_vars is
# attached to a brace group so the variable expands in the same shell
# that later expands the ``mv`` operand, keeping both consistent.
if self._snapshot_ready:
parts.append(
f"__hermes_snap_tmp=$(mktemp {_snap_tmp_template}) && "
f"{{ {_export_dump_excluding_session_vars(_snap_tmp, passthrough_names)} "
f"&& mv -f {_snap_tmp} {_quoted_snap}; }} "
f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true"