fix(snapshot): validate restored last background pid#2114
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
bashkit | 80b4433 | Commit Preview URL | Jun 21 2026, 06:19 PM |
There was a problem hiding this comment.
Pull request overview
Hardens snapshot restore by validating ShellState::last_bg_pid (used for $!) and charging its size against snapshot restore memory budgeting to prevent crafted snapshots from injecting oversized or malformed values.
Changes:
- Add
MAX_RESTORED_LAST_BG_PID_LENand reject restoredlast_bg_pidvalues that are empty, non-numeric, or too long. - Include restored
last_bg_pidlength in the computedMemoryBudgetfor snapshot restore checks. - Add an integration regression test that tampers
shell.last_bg_pidand expects restore to fail.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/bashkit/src/interpreter/mod.rs | Adds validation for restored last_bg_pid and charges its length to the restore-time memory budget. |
| crates/bashkit/tests/integration/snapshot_tests.rs | Adds an integration test that tampers snapshot JSON to set invalid last_bg_pid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+708
to
+714
| let limits = MemoryLimits::new().max_total_variable_bytes(1); | ||
| let mut restored = Bash::builder().memory_limits(limits).build(); | ||
| let result = restored.restore_snapshot(&tampered_bytes); | ||
| assert!( | ||
| result.is_err(), | ||
| "restore must reject invalid snapshot-controlled last_bg_pid" | ||
| ); |
Comment on lines
+2275
to
+2277
| if let Some(last_bg_pid) = &state.last_bg_pid { | ||
| budget.variable_bytes = budget.variable_bytes.saturating_add(last_bg_pid.len()); | ||
| } |
chaliy
force-pushed
the
2026-06-21-fix-snapshot-memory-limit-validation
branch
from
071f095 to
54e386e
Compare
Reject empty, non-numeric, or oversized restored $! and count it toward the snapshot restore memory budget, keeping the live budget consistent after restore. Adds a regression test under permissive limits that asserts the last_bg_pid validation error.
chaliy
force-pushed
the
2026-06-21-fix-snapshot-memory-limit-validation
branch
from
54e386e to
80b4433
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
ShellState::last_bg_pidis now serialized into snapshots but was not validated during restore, allowing a crafted snapshot to set an arbitrarily large or non-numeric$!and bypass memory limits.$!is expanded, creating an availability risk under tightMemoryLimits.Description
MAX_RESTORED_LAST_BG_PID_LENand reject restoredlast_bg_pidthat is empty, non-numeric, or longer than the constant. (edit:crates/bashkit/src/interpreter/mod.rs)last_bg_pidlength in the snapshot-restoreMemoryBudgetso it counts towardmax_total_variable_bytes. (edit:crates/bashkit/src/interpreter/mod.rs)snapshot_restore_rejects_invalid_last_bg_pidthat tampersshell.last_bg_pidwith oversized and non-numeric values and asserts restore failure under tight limits. (edit:crates/bashkit/tests/integration/snapshot_tests.rs)Testing
cargo fmt --checkwhich succeeded.cargo test -p bashkit --test integration snapshot_restore_rejectsand the snapshot restore tests including the newsnapshot_restore_rejects_invalid_last_bg_pidpassed.last_bg_pidvalues are rejected duringrestore_snapshotunder a smallMemoryLimitsbudget.Codex Task