Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Differential Security Review — [mono-move] Add gas benchmarks (PR #19470)
Date: 2026-04-16
Scope: 48c6902d0bb1fc2b04d5a89da07e1a3d2942698b...2bd66122499f5fc664346f13a353a94fa863d7c8
Reviewer: Automated differential review
Executive Summary
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| HIGH | 0 |
| MEDIUM | 0 |
| LOW | 2 |
Overall risk: Low. The change is scoped entirely to benchmark infrastructure and test programs inside third_party/move/mono-move/programs/; no production execution path, no consensus, no VM dispatch logic, and no storage are touched.
Key metrics: 12 files changed (12 Rust, 0 Move), 1 module touched (mono-move/programs + mono-move/gas), 2 findings.
Recommendation: APPROVE WITH NOTES
What Changed
Files changed: 12 (all Rust) | Lines: +545 / -8
| Module | Files Changed | Risk Level |
|---|---|---|
mono-move/gas/src/lib.rs |
1 | Low |
mono-move/core/src/instruction/mod.rs |
1 | Low |
mono-move/programs/benches/ |
5 (3 modified, 2 new) | Low |
mono-move/programs/src/ |
2 (1 modified, 1 new) | Low |
mono-move/programs/tests/ |
1 (new) | Low |
mono-move/programs/Cargo.toml |
1 | Trivial |
Findings
[LOW] Finding 1 — NoOpGasMeter exported from production crate without a feature or #[cfg] guard
File: third_party/move/mono-move/gas/src/lib.rs:82–92
Test coverage: Not tested (intended as bench/test helper)
Description: NoOpGasMeter is defined as a top-level pub struct in mono_move_gas::lib, the same crate and visibility level as SimpleGasMeter. Its doc comment says "for testing," but there is no #[cfg(any(test, feature = "testing"))] gate, no #[doc(hidden)], and no module-level barrier preventing its use in production execution paths.
InterpreterContext in the runtime is generic over G: GasMeter. Any future code that passes NoOpGasMeter where a real meter is expected would silently bypass all gas enforcement, with balance() always returning u64::MAX.
Concrete impact: No current exploit — the type is only consumed by bench binaries in this PR. The risk is that this API footgun grows into a future misuse as the codebase matures.
Why here: The type was introduced specifically to support the micro_op (plain, no-instrumentation) benchmark variants. A #[cfg(test)] guard or placement inside a testing feature gate would match the pattern already used elsewhere (e.g. #[cfg(feature = "testing")] in programs/src/lib.rs).
[LOW] Finding 2 — gas_instrument bench helper silently drops GC root metadata
File: third_party/move/mono-move/programs/benches/helpers.rs:228–238
Test coverage: Bench-only, not part of any test target
Description: gas_instrument builds gas-instrumented copies of functions but replaces both frame_layout and safe_point_layouts with empty stubs:
frame_layout: FrameLayoutInfo::empty(&arena),
safe_point_layouts: SortedSafePointEntries::empty(&arena),The function's doc comment acknowledges this: "Frame layouts are re-created as empty; these benchmark programs do not trigger GC, so the omission has no effect on execution." That is correct for the four benchmark programs today (all scalar-only, no heap pointer slots).
However, the function signature accepts any &[Option<ExecutableArenaPtr<Function>>] with no type-level or debug_assert! enforcement of the "no heap pointer locals" precondition. If a future program with GC-managed pointer slots is passed to this helper, the GC would fail to scan pointer-holding frame slots, causing silent corruption. There is no #[cfg(test)] or #[cfg(feature = "testing")] guard on the function itself.
Concrete impact: No current exploit — all four programs passed to gas_instrument today are scalar-only. The risk is a latent copy-paste footgun for any future benchmark program that allocates heap objects.
Test Coverage Analysis
| Changed Function / Path | Coverage | Notes |
|---|---|---|
NoOpGasMeter |
Bench-only | No unit test asserting the no-op contract |
gas_instrument helper |
Bench-only | No test asserting correctness of instrumented output |
micro_op_match_sum + bench |
Well-tested | tests/match_sum.rs covers native, micro_op, move_bytecode |
MicroOp: Copy + Clone derive |
Implicit | Exercised by raw.to_vec() in gas_instrument |
Blast Radius
All changed functions are confined to benchmark and test code; none are callable from the production execution pipeline. The NoOpGasMeter type is the only change reachable from outside test/bench contexts (as a pub export), but no production code currently imports it.
Correctness Notes (Not Findings)
-
Arena lifetime in
match_sum/nested_loopgas setup:let (functions, _, _arena) = micro_op_match_sum()followed bylet (functions_gas, _arena) = unsafe { helpers::gas_instrument(&wrapped) }shadows the original_arena. This is correct: Rust evaluates the right-hand side before the shadow takes effect, so the original arena outlives thegas_instrumentcall. The danglingfunctions/wrappedpointers after shadowing are never used in any closure. -
Hardcoded
functions[6]inbst.rs: Pre-existing pattern; the comment// Function 6 — run_opsinsrc/bst.rs:315confirms the index is stable. Not introduced by this PR. -
Missing
Function::resolve_callsformatch_sum/nested_loopgas variants: Correct omission — these programs contain noCallFuncops (no inter-function calls), so no patching is needed. -
descriptorsreuse acrossbst.rsbench setups: The gas benchmark closure capturesdescriptorsfrom the firstmicro_op_bst()call while usingfunctions_gasfrom the second. This is safe becauseObjectDescriptoris plain data with no arena pointers.
Recommendations
Before Production Use
- Gate
NoOpGasMeterbehind#[cfg(any(test, feature = "testing"))]or move it to a dedicated test-helper module — prevents accidental use in execution paths as the codebase grows. - Add a
debug_assert!togas_instrumentthat all input functions have emptyframe_layout(or document the precondition more explicitly), to catch future benchmark programs with heap pointer locals before they produce GC bugs.
Sent by Cursor Automation: Security Review Bot
| } | ||
|
|
||
| /// A no-op gas meter for testing. | ||
| pub struct NoOpGasMeter; |
There was a problem hiding this comment.
[LOW] NoOpGasMeter is exported as a top-level pub struct with no #[cfg(test)] or feature gate. Since InterpreterContext is generic over G: GasMeter, this type is silently usable in any execution context. The doc comment says "for testing" but there is no compile-time enforcement. Consider #[cfg(any(test, feature = "testing"))], consistent with the pattern already used in programs/src/lib.rs.
| }) | ||
| .collect(); | ||
| (new_fns, arena) | ||
| } |
There was a problem hiding this comment.
[LOW] frame_layout and safe_point_layouts are silently dropped (replaced with empty stubs). The doc comment explains this is safe for programs that don't trigger GC, but the function accepts any input without enforcing that precondition. If a future benchmark program has heap-pointer locals, the GC will silently miss those roots. A debug_assert! on func.frame_layout.heap_ptr_offsets being empty, or a note in the # Safety section, would make the precondition explicit.
calintat
force-pushed
the
calin/gas-benchmarks
branch
from
2bd6612 to
54cf9d7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
3 participants
Description
Add gas-instrumented variants to all micro-op benchmarks (
fib,bst,merge_sort,nested_loop) and a newmatch_sumbenchmark with a wide-diamond CFG shape. Each benchmark now runs both a plain and a gas-instrumented version, making it easy to measure gas metering overhead going forward.How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist
Note
Low Risk
Low risk: changes are limited to benchmarking/test utilities and a new synthetic program, with minimal impact on runtime behavior outside benches.
Overview
Adds gas-measurement coverage to the micro-op benchmarks by running each benchmark in two modes: a plain execution using the new
NoOpGasMeter, and a gas-instrumented execution that replays the same program afterGasInstrumentorhas insertedChargeops.Introduces shared bench helper
gas_instrumentto clone and instrument micro-opFunctiontables into a fresh arena, and adds a newmatch_sumsynthetic program + Criterion bench (including correctness tests) designed with a wide-diamond CFG to stress basic-block boundary instrumentation. Also makesMicroOpCopy/Cloneto simplify handling in instrumentation/bench code.Reviewed by Cursor Bugbot for commit 54cf9d7. Bugbot is set up for automated code reviews on this repo. Configure here.