fix(interp): restore prior value for same-name inline-command vars by AlexandreYang · Pull Request #262 · DataDog/rshell

AlexandreYang · 2026-05-19T12:19:49Z

Summary

Fix vuln-hunt F-2 (inline_var, P3): A=v1 A=v2 cmd was leaving A=v1 in the parent shell instead of restoring A's true pre-command value.
Root cause: each iteration recaptured prev after the previous assignment took effect, so the 2nd restore entry held the intermediate. The forward-order defer then applied the stale restore last.
Fix: dedupe the restore list by name — only the first prev per name is recorded, matching bash semantics.
Regression scenario added covering the 2- and 3-assignment cases.

Test plan

go test ./interp/... ./tests/ passes locally
New scenario same_name_multi_assign_restores_prior_value.yaml passes (and is asserted against bash by default in CI)
Verified bash output of the scenario matches expectation by running the script directly in debian:bookworm-slim
CI bash-comparison suite (RSHELL_BASH_TEST=1) green

Reference

Finding doc: DataDog/rshell-vuln-hunt — 2026-05-18-initial-audit / findings / inline_var.md

vuln-hunt F-2 (inline_var, P3): `A=v1 A=v2 cmd` left `A=v1` in the parent shell instead of restoring A's true pre-command value. Each loop iteration recaptured `prev` after the previous assignment took effect, so the second restore entry held an intermediate, and the forward-order defer applied it last. Dedupe the restore list by name so only the first prev per name is recorded, matching bash semantics. Adds a regression scenario covering the 2- and 3-assignment cases.

AlexandreYang · 2026-05-19T12:21:15Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-review (iteration 1)

Overall assessment: safe to merge — small, focused fix that moves rshell behaviour closer to bash. No security concerns. Only nice-to-have improvements to test coverage.

Summary table

#	Priority	File	Finding
1		`tests/scenarios/shell/inline_var/same_name_multi_assign_restores_prior_value.yaml`	Add interleaved same-name/different-name case (`A=v1 B=vb A=v2 cmd`) to strengthen regression coverage of the dedup logic
2		`tests/scenarios/shell/inline_var/same_name_multi_assign_restores_prior_value.yaml`	Assert restore to unset for an unset-before variable (currently only the set-before case is covered)

Coverage

Code path	Scenario test	Status
`A=v1 A=v2 cmd` restores prior set value	`same_name_multi_assign_restores_prior_value.yaml`	Covered
`A=v1 A=v2 A=v3 cmd` restores prior set value	`same_name_multi_assign_restores_prior_value.yaml`	Covered
Same-name dedup with interleaved different names	—	Missing (P3)
`A=v1 A=v2 cmd` when A is unset before	—	Missing (P3)
Distinct-name multi-assign (existing)	`multi_assign_all_restored.yaml`	Covered (no regression)

Positive observations

Fix is minimal and matches the finding's preferred Option 1 (dedup by name).
Inline comment explains why only the first prev is the true pre-command value — good for future readers.
cd PWD/OLDPWD skip is preserved (the dedup happens before defer; the loop's restore filter still applies).
Restore semantics are unchanged for the readonly-target case (FOO=ok RO_VAR=evil cmd): existing pentest TestVulnHuntShellFeatureReadonlyBypass_MultiVarInlineWithReadonly still passes.
No filesystem, no new imports, no concurrency surface.

AlexandreYang · 2026-05-19T12:24:19Z

Iteration 1 self-review: 0 P0 / 0 P1 / 0 P2 / 2 P3 — no blocking findings. P0_P1_P2_COUNT=0.

The two P3 findings are nice-to-have test-coverage extensions (interleaved same/different name case, and unset-before case). See the review at #262 (review).

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f89a95915b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Add interleaved same/different-name case (`A=v1 B=vb A=v2 cmd`) and a never-set-before case (`C=v1 C=v2 cmd`) to the regression scenario. Both verified against debian:bookworm-slim bash.

AlexandreYang · 2026-05-19T12:30:18Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-review (iteration 2)

Overall assessment: safe to merge — no new findings. All P3 items from iteration 1 have been addressed (interleaved + never-set cases added to scenario; expect.stderr uses |+ block scalar per AGENTS.md).

Summary table

No findings.

Coverage

Code path	Scenario test	Status
`A=v1 A=v2 cmd` restores prior set value	`same_name_multi_assign_restores_prior_value.yaml`	Covered
`A=v1 A=v2 A=v3 cmd` restores prior set value	`same_name_multi_assign_restores_prior_value.yaml`	Covered
Same-name dedup interleaved with distinct name	`same_name_multi_assign_restores_prior_value.yaml`	Covered
Same-name dedup when var was never set before	`same_name_multi_assign_restores_prior_value.yaml`	Covered
Distinct-name multi-assign (existing)	`multi_assign_all_restored.yaml`	Covered (no regression)

P0_P1_P2_COUNT=0 for this iteration.

AlexandreYang · 2026-05-19T12:31:12Z

Iteration 2 self-review: 0 P0 / 0 P1 / 0 P2 / 0 P3 — clean. P0_P1_P2_COUNT=0. See #262 (review).

AlexandreYang · 2026-05-19T12:32:56Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

chatgpt-codex-connector · 2026-05-19T12:33:06Z

Codex Review: Didn't find any major issues. Bravo.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

AlexandreYang

Self-review (iteration 3)

No new commits since iteration 2 (HEAD still at 3969b86b). Re-ran the same review checks: security (no fs/exec/import deltas), bash compatibility (verified against debian:bookworm-slim), correctness (dedup logic preserves the existing cd PWD/OLDPWD restore filter), coverage (4 scenario cases for the same-name path).

No findings.

P0_P1_P2_COUNT=0 for this iteration.

AlexandreYang · 2026-05-19T12:33:33Z

Iteration 3 self-review: 0 P0 / 0 P1 / 0 P2 / 0 P3 — clean (no new commits since iter 2). P0_P1_P2_COUNT=0.

AlexandreYang · 2026-05-19T12:34:42Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-review (iteration 4)

No new commits since iteration 3 (HEAD still at 3969b86b). No findings.

P0_P1_P2_COUNT=0.

AlexandreYang · 2026-05-19T12:34:54Z

Iteration 4 self-review: 0 P0 / 0 P1 / 0 P2 / 0 P3 — clean (no new commits since iter 3). P0_P1_P2_COUNT=0.

AlexandreYang · 2026-05-19T12:35:23Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-review (iteration 5)

No new commits since iteration 4 (HEAD still at 3969b86b). No findings.

P0_P1_P2_COUNT=0.

AlexandreYang · 2026-05-19T12:35:32Z

Iteration 5 self-review: 0 P0 / 0 P1 / 0 P2 / 0 P3 — clean (no new commits since iter 4). P0_P1_P2_COUNT=0.

AlexandreYang · 2026-05-19T12:36:20Z

Review-Fix Loop Summary

PR: fix(interp): restore prior value for same-name inline-command vars #262 (fix(interp): restore prior value for same-name inline-command vars #262)
Iterations completed: 5
Final status: CLEAN
Stopped because: SUCCESS_COUNT reached 5 consecutive clean iterations

Iteration log

#	Unresolved threads	Fixes applied	CI status
1	1 (codex P3 YAML style)	3 (interleaved + never-set test cases, `\|+` block scalar for `expect.stderr`)	Passing
2	0	—	Passing
3	0	—	Passing
4	0	—	Passing
5	0	—	Passing

Final state

Unresolved threads: 0
P0/P1/P2 findings: 0
CI: Passing (39/40 green, 1 still IN_PROGRESS at summary time; no failures)
PR description: includes a clickable link to the vuln-hunt finding doc

Remaining issues

None.

astuyve · 2026-05-19T13:28:05Z

 			vr   expand.Variable
 		}
 		var restores []restoreVar
+		seenRestore := map[string]bool{}


nit: this could be a set. I don't think Go has one in the std impl thouhg?

indeed, there is no set in Go 😭

AlexandreYang commented May 19, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed May 19, 2026

View reviewed changes

Comment thread tests/scenarios/shell/inline_var/same_name_multi_assign_restores_prior_value.yaml Outdated

AlexandreYang added 2 commits May 19, 2026 14:26

[iter 1] Expand inline_var same-name regression coverage

42babc6

Add interleaved same/different-name case (`A=v1 B=vb A=v2 cmd`) and a never-set-before case (`C=v1 C=v2 cmd`) to the regression scenario. Both verified against debian:bookworm-slim bash.

[iter 1] Use |+ block scalar for expect.stderr per AGENTS.md

3969b86

AlexandreYang commented May 19, 2026

View reviewed changes

AlexandreYang marked this pull request as ready for review May 19, 2026 12:37

AlexandreYang requested review from astuyve, julesmcrt, matt-dz, thieman and val06 as code owners May 19, 2026 12:37

astuyve reviewed May 19, 2026

View reviewed changes

astuyve approved these changes May 19, 2026

View reviewed changes

AlexandreYang added this pull request to the merge queue May 19, 2026

Merged via the queue into main with commit 2904b38 May 19, 2026
40 checks passed

AlexandreYang deleted the alex/fix-inline_var branch May 19, 2026 13:37

Uh oh!

Conversation

AlexandreYang commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Reference

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-review (iteration 1)

Summary table

Coverage

Positive observations

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-review (iteration 2)

Summary table

Coverage

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 19, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-review (iteration 3)

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-review (iteration 4)

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-review (iteration 5)

Uh oh!

AlexandreYang commented May 19, 2026

Uh oh!

AlexandreYang commented May 19, 2026

Review-Fix Loop Summary

Iteration log

Final state

Remaining issues

Uh oh!

astuyve May 19, 2026

Choose a reason for hiding this comment

Uh oh!

AlexandreYang May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

AlexandreYang commented May 19, 2026 •

edited

Loading