Skip to content

fix(snapshot): saturate session counters#1878

Merged
chaliy merged 2 commits into
mainfrom
2026-06-05-fix-snapshot-counters-overflow-issue
Jun 5, 2026
Merged

fix(snapshot): saturate session counters#1878
chaliy merged 2 commits into
mainfrom
2026-06-05-fix-snapshot-counters-overflow-issue

Conversation

@chaliy
Copy link
Copy Markdown
Contributor

@chaliy chaliy commented Jun 5, 2026

Motivation

  • Restoring snapshot-provided u64 session counters could set counters to u64::MAX and the subsequent unchecked += 1 would overflow or wrap, allowing a forged snapshot to crash the process or bypass session exec-call limits.
  • Prevent overflow/wrap while preserving the intended monotonic merge semantics for authenticated snapshots.

Description

  • Use saturating increments for per-exec and session counters by replacing += 1 with saturating_add(1) in tick_command and tick_exec_call in crates/bashkit/src/limits.rs.
  • Add unit tests test_command_counter_saturates_on_overflow and test_exec_counter_saturates_on_overflow in crates/bashkit/src/limits.rs to assert counters saturate at their max values.
  • Add integration regression snapshot_restore_extreme_exec_counter_errors_without_overflow in crates/bashkit/tests/integration/snapshot_tests.rs to assert restoring session_exec_calls = u64::MAX does not panic or wrap and still enforces limits.

Testing

  • Ran formatting check with cargo fmt --all -- --check and it passed.
  • Ran unit tests including the new saturation tests with cargo test -p bashkit and the new tests passed.
  • Ran snapshot integration tests with cargo test -p bashkit --test integration snapshot_tests:: and the snapshot suite (including the new regression) passed (all tests green).

Codex Task

Copilot AI review requested due to automatic review settings June 5, 2026 01:03
@chaliy chaliy added the aardvark label Jun 5, 2026
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Jun 5, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit b837165 Commit Preview URL Jun 05 2026, 01:12 AM

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens snapshot/restore session accounting against counter overflow by switching exec/session counter increments to saturating arithmetic, preventing panics (debug) or wraparound (release) when restoring forged/extreme snapshot values.

Changes:

  • Replace += 1 with saturating_add(1) for command and session exec-call counters in ExecutionCounters.
  • Add unit tests to verify counters saturate at usize::MAX / u64::MAX instead of overflowing.
  • Add an integration regression test ensuring restoring session_exec_calls = u64::MAX doesn’t panic/wrap and still blocks further exec() calls via session limits.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
crates/bashkit/src/limits.rs Use saturating increments for counters; add unit tests covering saturation behavior.
crates/bashkit/tests/integration/snapshot_tests.rs Add regression test for restoring extreme exec-call counters without overflow/wrap.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/bashkit/tests/integration/snapshot_tests.rs
is_err() alone could be satisfied by unrelated failures; verify the error
text contains "session exec() call limit" to pin the exact failure path.
@chaliy chaliy merged commit 1caba78 into main Jun 5, 2026
35 checks passed
@chaliy chaliy deleted the 2026-06-05-fix-snapshot-counters-overflow-issue branch June 5, 2026 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants