Fix Bash script body leak on job cancellation

Lachlan Grantrequested to merge
fix/issue-39005-script-dump-race-condition into main

What does this MR do?

Closes the cancellation-time leak in which the runner's generated job script can be dumped — including expanded secret variables — into the public job log when a job is cancelled.

Closes #39005 (closed). Follow-up to #37952 (closed) and !4980 (merged).

Root cause

When a job is cancelled, the Kubernetes executor sends SIGTERM to every PID matching the stage script. The runner generates a bash script that runs the user code as:

: | eval '<entire job script body, with variables expanded>'

There are actually two distinct bash code paths that can dump the eval body to stderr when this pipeline component is signal-killed:

  1. bash <4.4 — a process killed by SIGTERM with no trap prints its current command before terminating. Upstream commit https://cgit.git.savannah.gnu.org/cgit/bash.git/commit/?id=0275a139abe94c198eb04b05b39ca74c137bfc65 added DONT_REPORT_SIGTERM in bash 4.4 to suppress this for SIGTERM specifically. The original !4980 (merged) fix targets this path.
  2. bash 5.3+ — a new code path introduced in upstream commit https://git.savannah.gnu.org/cgit/bash.git/commit/?id=d17d185fff39168941814e88e6814b686e295f13 adds a print_pipeline call inside notify_of_job_status. The dump happens on the parent shell when it reaps a child that was killed by a signal — independently of the parent's TERM trap state. The !4980 (merged) fix does not cover this path.

Both paths produce log output of the form:

step_script: line N: <pid> Done : <pid> Terminated | eval '<entire body>'

Customer log excerpts in #37952 (closed) and #39005 (closed) match this format byte-for-byte.

Fix

Run the user script in an explicit subshell with stdin closed via < /dev/null instead of a pipeline, and install a TERM trap inside the subshell:

(trap 'exit 1' TERM; eval '<body>') < /dev/null

Two changes vs. the prior form:

  1. Drop the : | pipeline. < /dev/null closes stdin just as effectively, and removing the pipeline removes the bash <4.4 dump format that includes the pipeline text.
  2. Install the TERM trap inside the subshell. Traps reset to defaults across the subshell boundary, so a trap in the outer shell does not protect the subshell that actually receives SIGTERM. With the inner trap installed as the first thing in the subshell, SIGTERM causes a clean exit 1 instead of a signal-induced death — and bash 5.3's print_pipeline only fires when the parent reaps a child that was killed by a signal. A clean trap-driven exit defeats that code path.

The outer trap 'exit 1' TERM is kept for defense in depth.

(Note: the prior code had trap exit 1 TERM unquoted, which bash parses as "trap the exit command on signals 1 and TERM" — i.e. trapping HUP+TERM with the wrong command. The dump suppression worked incidentally because TERM was still in the trap list, but the intent was always trap 'exit 1' TERM. The MR also fixes that quoting.)

Second leak site: the native step runner

The eval invocation is generated in two independent places:

  1. shells/bash.go — the classic "abstract" shell executor (everything above).
  2. functions/concrete/run/stages/internal/scriptwriter/scriptwriter.go — the native step-runner counterpart. functions/concrete is the step-runner builtin counterpart to shells/abstract.go; it is registered as the concrete step function in commands/steps/steps.go and builds + runs the per-step scripts (pre_clone_script, post_clone_script, the user script, after_script, release, …) when a job executes via step-runner rather than the classic monolithic bash script.

The step runner carried the same leak independently, with all three problems the classic path had:

  • The outer trap was the unquoted trap exit 1 TERM.
  • Both eval branches used the : | eval / : | (eval) pipeline form, leaking the body on bash <4.4.
  • Neither branch installed a TERM trap inside the eval subshell, so bash 5.3+ still dumps the body when the parent reaps the signal-killed child. Notably, even the pre-existing FF_USE_NEW_BASH_EVAL_STRATEGY=true subshell form (: | (eval …)) closed neither leak path.

This MR applies the same (trap 'exit 1' TERM; eval '<body>') < /dev/null form there and fixes the trap quoting, so both execution models are protected.

Escape hatch

The new behaviour is the default. This MR also introduces a new feature flag FF_USE_LEGACY_BASH_EVAL (default false) that restores the prior : | eval '...' form, in case the new generated script causes unforeseen issues in environments not covered by local testing. Matches the existing FF_USE_LEGACY_* naming convention. Both the classic shell path and the native step runner honour it.

Scope creep note: FF_USE_NEW_BASH_EVAL_STRATEGY

This MR also marks the existing FF_USE_NEW_BASH_EVAL_STRATEGY flag as deprecated (no-op). That flag previously toggled between : | eval '...' and : | (eval '...'); both forms are vulnerable to this leak and are superseded by the new safe form.

The classic shell path stopped reading it once the safe form became unconditional. The native step runner was still gating its (incomplete) subshell form on it; this MR switches the step runner to FF_USE_LEGACY_BASH_EVAL to match shells/bash.go, which makes FF_USE_NEW_BASH_EVAL_STRATEGY a true no-op across the entire codebase, consistent with its deprecated description. The serialized Step JSON field is renamed use_new_eval_strategy → use_legacy_bash_eval (same-binary producer/consumer; the meaning inverts, so reusing the key would misread a stale value, and the default degrades to the safe form regardless).

We acknowledge that deprecating that flag plus the step-runner consolidation is borderline scope creep for a bug-fix MR — but the step runner is a genuine second instance of the same leak, leaving a feature-flag-controlled field that nothing reads any more is dead code, and silently ignoring a documented flag without telling users is worse than marking it deprecated. The constant remains in the registry so any existing runner config that sets the flag continues to parse without error.

Happy to split this into a follow-up MR if reviewers prefer.

How was this tested?

Unit tests

  • shells/bash_test.go TestBash_Finish_EvalForm asserts that BashWriter.Finish emits the new trapped-subshell form by default, and the legacy : | eval form when FF_USE_LEGACY_BASH_EVAL is set.
  • functions/concrete/run/stages/internal/scriptwriter tests assert the step runner emits the (trap 'exit 1' TERM; eval …) < /dev/null form by default and the legacy : | eval form when FF_USE_LEGACY_BASH_EVAL is set, and that the outer trap is quoted.
  • functions/concrete/builder tests assert the feature-flag wiring (FF_USE_LEGACY_BASH_EVAL → Step.UseLegacyBashEval).

End-to-end reproduction (bash 5.3)

Reproduced with the runner's exact generated-script shape against docker.io/bash:5.3 in podman — the version where the new notify_of_job_status → print_pipeline code path lives:

  • bash 5.3, legacy : \| eval form, SIGTERM to bash PIDs → full eval body (canary + fake token) dumps to stderr. Bug reproduced.
  • bash 5.2, same legacy form (control) → silent. Confirms the bug is specifically the new bash 5.3 mechanism, not present in earlier versions.
  • bash 5.3, fixed (trap 'exit 1' TERM; eval ...) < /dev/null form, SIGTERM to bash PIDs + descendants (matching what stageCancellationScript does) → silent, ~3 second clean cancel. Confirms the fix.

The reproduction scripts (demo-bash-52.sh, demo-bash-53.sh, demo-bash-53-fixed.sh) and the e2e Go test driving BashWriter against bash:5.3 are local-only artefacts and not committed.

Regressions

Existing shells/..., helpers/featureflags/..., and functions/concrete/... unit tests pass.

Test plan

  • CI passes
  • Verify on a real cancellation that the dump no longer appears in job logs (Kubernetes executor, image with bash 5.3+)
  • Verify the escape hatch: setting FF_USE_LEGACY_BASH_EVAL=true produces the prior : | eval form (run the unit tests, or inspect a generated job script)

🤖 Generated with Claude Code

Edited by Lachlan Grant

Merge request reports