fix: Add timeout configuration for WASM execution#1840
fix: Add timeout configuration for WASM execution#1840
Conversation
|
Cursor Agent can help with this pull request. Just |
chefsale
changed the title
cursor
bot
force-pushed
the
bounty-fix/add-timeout-configuration-for-wasm-execu-ml6pzeyr
branch
from
c047977 to
b24a82d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
cursor
bot
force-pushed
the
bounty-fix/add-timeout-configuration-for-wasm-execu-ml6pzeyr
branch
from
f0f14ed to
7e2e84e
Compare
xilosada
left a comment
xilosada
left a comment
There was a problem hiding this comment.
🤖 AI Code Review
Reviewed by 1 agents | Quality score: 85% | Review time: 182.9s
🔴 Critical (1)
1. Compilation error: create_engine() function no longer exists
File: crates/runtime/src/lib.rs (line 122-122) | Consensus: 1/1 agents ✓
In the headless() function, there is a call to Self::create_engine() on line 122 (within the #[cfg(feature = "profiling")] block), but this function was renamed to create_engine_with_metering() in this PR. This will cause a compilation error when the profiling feature is enabled.
Suggested fix:
Change `Self::create_engine()` to `Self::create_engine_with_metering(&limits)` in the headless() function's profiling block.
Found by: cursor-agent
🟡 Warning (1)
1. The profiling feature is now effectively empty
File: crates/runtime/Cargo.toml (line 50-50) | Consensus: 1/1 agents ✓
The profiling feature was changed from ["wasmer-compiler-cranelift"] to [], making it a no-op since Cranelift is now always included. Users enabling this feature would expect it to do something, and the existing code in headless() and create_engine_with_metering() still checks for this feature flag to enable PerfMap profiling. Consider either removing the feature entirely or documenting that it only controls runtime profiling behavior, not compiler selection.
Suggested fix:
Update the feature documentation or consider if the feature is still needed. At minimum, add a comment explaining what the feature now controls (runtime env var check behavior).
Found by: cursor-agent
💡 Suggestion (2)
1. Uniform cost function may not accurately reflect wall-clock time
File: crates/runtime/src/lib.rs (line 96-96) | Consensus: 1/1 agents ✓
The metering middleware uses |_op| 1 which assigns equal cost to all WASM operations. In practice, different operations (memory access, arithmetic, calls) have vastly different execution times. This means the 'timeout' is actually an operation count limit rather than a true time limit. The actual wall-clock time could vary significantly based on the operation mix.
Suggested fix:
Consider documenting this limitation in the `max_execution_time` field documentation, clarifying that it's an approximation based on operation count rather than actual wall-clock time. Alternatively, consider using a more sophisticated cost function if precision is important.
Found by: cursor-agent
2. Timeout check should occur before attempting error downcast
File: crates/runtime/src/lib.rs (line 371-385) | Consensus: 1/1 agents ✓
Currently, when an error occurs during WASM execution, the code first checks for timeout (fuel exhaustion) and returns ExecutionTimeout. However, the order of checks means that if the error happens to be a VMLogicError that coincidentally occurs when fuel is also exhausted, it would be reported as a timeout rather than the actual error. This could potentially mask real errors.
Suggested fix:
Consider checking if the RuntimeError is specifically the fuel exhaustion trap type from wasmer before assuming it's a timeout. Wasmer's metering should throw a specific trap that can be matched.
Found by: cursor-agent
📝 Nitpick (2)
1. Magic number could be extracted as a named constant
File: crates/runtime/src/lib.rs (line 70-70) | Consensus: 1/1 agents ✓
The value 10_000_000 for OPS_PER_SECOND is defined locally in the function. Since this is an important tuning parameter that affects timeout behavior, it might be clearer to define it as a module-level constant alongside other defaults in logic.rs.
Suggested fix:
Consider moving `OPS_PER_SECOND` to be a module-level constant for easier discovery and tuning.
Found by: cursor-agent
2. Unrelated dependency change in Cargo.lock
File: Cargo.lock (line 6707-6707) | Consensus: 1/1 agents ✓
The tempfile dependency was added to meroctl package in Cargo.lock, but this PR is about runtime timeout configuration. This appears to be an unrelated change that may have been included accidentally during lock file regeneration.
Suggested fix:
Verify if this change is intentional. If not, consider regenerating Cargo.lock from a clean state to avoid including unrelated changes.
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-35cae70e
cursor
bot
force-pushed
the
bounty-fix/add-timeout-configuration-for-wasm-execu-ml6pzeyr
branch
2 times, most recently
from
32de894 to
b44b2bc
Compare
xilosada
left a comment
xilosada
left a comment
There was a problem hiding this comment.
🤖 AI Code Review
Reviewed by 3 agents | Quality score: 85% | Review time: 106.7s
🟡 Issues pending resolution
✅ 4 Fixed | 🆕 13 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
Low Severity(crates/runtime/src/lib.rs:176)Low Severity(crates/runtime/src/errors.rs:175)High Severity(crates/runtime/src/lib.rs:90)Medium Severity(crates/runtime/src/lib.rs:269)
🆕 New Issues
These issues were found in the latest changes:
🟡 Warning (5)
1. Dependency version not using workspace
File: crates/runtime/Cargo.toml (line 28-28) | Consensus: 1/3 agents
The wasmer-middlewares dependency uses a hardcoded version "6.1.0" instead of workspace versioning like other wasmer dependencies (wasmer.workspace = true, wasmer-types.workspace = true). This could lead to version mismatches if the workspace updates wasmer versions.
Suggested fix:
Add `wasmer-middlewares` to the workspace `[dependencies]` section and use `wasmer-middlewares.workspace = true` here to maintain version consistency with other wasmer crates.
Found by: quality-agent
2. Headless engine silently disables operation limits
File: crates/runtime/src/lib.rs (line 96-108) | Consensus: 1/3 agents
The headless() function explicitly disables operation limits by setting max_operations = 0. While documented in the docstring, this creates a security risk: precompiled modules loaded via headless engines will run without any execution time protection, potentially allowing infinite loops or DoS if an attacker can cause a headless engine to be used. The VMLimits returned by headless() differs from VMLimits::default() in a security-critical way.
Suggested fix:
Consider either: (1) preventing headless engines from being used in production contexts where limits are required, (2) returning an error or logging a security warning when `headless()` is used with modules that would normally be limited, or (3) adding a compile-time flag or runtime check to ensure headless engines are only used when explicitly intended.
Found by: security-agent
3. Headless engine metering inconsistency with precompiled modules
File: crates/runtime/src/lib.rs (line 87-117) | Consensus: 1/3 agents
When a module is compiled with metering enabled (max_operations > 0) but later loaded via a headless engine, the metering middleware is still baked into the compiled code and will trigger traps when exhausted. However, since headless engines force max_operations = 0, the error handling in create_instance_and_call won't check get_remaining_points and won't convert the metering trap to WasmTrap::ExecutionTimeout. The error will surface as an opaque runtime error rather than the proper timeout error, making debugging harder and breaking error handling contracts.
Suggested fix:
Either detect metering traps in the error handler regardless of `max_ops` value, or document clearly that precompiled modules must be loaded with matching engine configurations. Consider adding a method to detect if a loaded module has metering enabled.
Found by: performance-agent
4. Error context lost when metering exhausted
File: crates/runtime/src/lib.rs (line 364-376) | Consensus: 1/3 agents
When metering points are exhausted, the code returns ExecutionTimeout and discards the original runtime error. This shadows any additional error context that wasmer might provide, making debugging harder. The original error should at least be logged before returning the more specific timeout error.
Suggested fix:
Log the original error before returning ExecutionTimeout: `debug!(%context_id, method, original_error = ?err, "Metering exhausted, returning ExecutionTimeout");`
Found by: quality-agent
5. Metering exhaustion check may miss edge cases on error path
File: crates/runtime/src/lib.rs (line 367-378) | Consensus: 1/3 agents
The check for MeteringPoints::Exhausted only happens after a RuntimeError occurs. However, wasmer metering can return exhausted state even without a trap if the last instruction exactly depleted the budget. Additionally, if a host function error occurs simultaneously with metering exhaustion, the metering error will shadow the host error, potentially hiding the root cause.
Suggested fix:
Consider checking metering state before attempting to downcast the error, and potentially include both pieces of information in the error if metering was also exhausted during a host error.
Found by: performance-agent
💡 Suggestion (7)
1. Insufficient documentation on Engine::with_limits
File: crates/runtime/src/lib.rs (line 52-56) | Consensus: 1/3 agents
The with_limits method lacks documentation about an important constraint: the metering middleware is baked into compiled modules at compile time. If users want different operation limits for the same WASM code, they must recompile with a new Engine. This limitation should be documented to prevent confusion.
Suggested fix:
Add documentation: `/// Note: The operation limit is embedded at compile time. Modules compiled with this engine
/// will use the specified `max_operations` limit. To use different limits, create a new Engine
/// and recompile the module.`
Found by: quality-agent
2. Missing test for headless engine behavior with precompiled metered modules
File: crates/runtime/src/lib.rs (line 676-806) | Consensus: 1/3 agents
The documentation states that headless engines cannot enforce operation limits, but there's no test verifying this behavior. A test should confirm that a module compiled with metering runs correctly (or with documented behavior) when loaded via a headless engine, especially since the headless engine sets max_operations = 0.
Suggested fix:
Add a test that compiles a module with a full engine (with metering), serializes it, then deserializes with a headless engine and verifies the expected behavior.
Found by: quality-agent
3. Operation limit of zero allows unbounded execution
File: crates/runtime/src/logic.rs (line 160-165) | Consensus: 1/3 agents
Setting max_operations = 0 completely disables execution limits. If VMLimits can be configured by external input or less-trusted configuration sources, an attacker could set this to zero to allow unbounded WASM execution, enabling DoS attacks via infinite loops or computationally expensive operations.
Suggested fix:
Consider making the zero-disabling behavior opt-in with a separate flag (e.g., `disable_operation_limit: bool`) rather than overloading the meaning of zero. Alternatively, add a minimum allowed value for `max_operations` when limits are enabled, or validate that this value cannot be externally set to zero in production.
Found by: security-agent
4. Metering points reset on every method call including hooks
File: crates/runtime/src/lib.rs (line 282-285) | Consensus: 1/3 agents
The metering points are set to max_ops before execution, which is correct. However, after the registration hook (__calimero_register_crdt_types) runs, the remaining points aren't preserved. If the hook consumes significant operations, the actual method gets fewer operations than max_ops. This is likely intentional for security, but could cause surprising behavior where the same method succeeds or fails depending on whether the hook was called.
Suggested fix:
Document this behavior explicitly - that `max_operations` is a budget for the entire execution including hooks, not just the user method. Alternatively, consider tracking hook vs method consumption separately for debugging/metrics.
Found by: performance-agent
5. Uniform cost function may allow resource amplification
File: crates/runtime/src/lib.rs (line 80-85) | Consensus: 1/3 agents
The metering cost function assigns each WASM operation a cost of 1 unit (|_op| 1), regardless of actual resource consumption. Operations like memory accesses, function calls, or certain instructions may have significantly different real costs. An attacker could craft WASM code that maximizes expensive operations while staying within the operation count limit, potentially causing resource exhaustion (CPU time, memory bandwidth) despite passing the metering check.
Suggested fix:
Consider implementing a weighted cost function that assigns higher costs to expensive operations (e.g., memory grow, indirect calls, division). This provides better DoS protection by more accurately reflecting actual resource consumption.
Found by: security-agent
6. Cranelift compiler always included increases binary size and compile time
File: crates/runtime/src/lib.rs (line 63-68) | Consensus: 1/3 agents
The change makes wasmer-compiler-cranelift a required dependency (previously optional with profiling feature). Cranelift is a full optimizing compiler that adds significant compile-time overhead compared to wasmer's default interpreter or singlepass compiler. For modules that don't need metering (max_operations = 0), this adds unnecessary compilation cost.
Suggested fix:
Consider conditionally using a faster compiler (singlepass) when metering is disabled, or document the performance trade-off. Alternatively, provide a feature flag to opt-out of metering entirely for performance-critical deployments that trust their WASM code.
Found by: performance-agent
7. Metering points set per-execution allows reset between calls
File: crates/runtime/src/lib.rs (line 277-283) | Consensus: 1/3 agents
The metering::set_remaining_points is called at the start of each run_inner execution with the full max_ops value. If a module is executed multiple times within a session or context, each execution gets a fresh operation budget. This could allow cumulative resource consumption if there's no higher-level rate limiting across multiple invocations.
Suggested fix:
Consider whether there should be aggregate limits across multiple WASM invocations within a session or time window, particularly if the same module can be called repeatedly without external rate limiting.
Found by: security-agent
📝 Nitpick (1)
1. VMLimits field mutability pattern inconsistent with other fields
File: crates/runtime/src/logic.rs (line 154-162) | Consensus: 1/3 agents
In tests, max_operations is set by direct field mutation (limits.max_operations = 10_000), while VMLimits uses Default::default(). For better API ergonomics and to prevent invalid states, consider adding a builder pattern or constructor methods, consistent with how other Rust APIs handle configuration structs.
Suggested fix:
Consider adding `VMLimits::builder()` or `VMLimits::with_max_operations(u64)` methods to provide a more ergonomic API that could also validate constraints between fields.
Found by: quality-agent
🤖 Generated by AI Code Reviewer | Review ID: review-60c9b92a
cursor
bot
force-pushed
the
bounty-fix/add-timeout-configuration-for-wasm-execu-ml6pzeyr
branch
from
0ccc09f to
befdd2b
Compare
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 1035.7s
🟡 Issues pending resolution
✅ 21 Fixed | 🆕 5 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
Low Severity(crates/runtime/src/lib.rs:176)Low Severity(crates/runtime/src/errors.rs:189)High Severity(crates/runtime/src/lib.rs:148)Medium Severity(crates/runtime/src/lib.rs:269)Dependency version not using workspace(crates/runtime/Cargo.toml:28)Headless engine silently disables operation limits(crates/runtime/src/lib.rs:156)Headless engine metering inconsistency with precompiled modules(crates/runtime/src/lib.rs:145)Error context lost when metering exhausted(crates/runtime/src/lib.rs:456)Metering exhaustion check may miss edge cases on error path(crates/runtime/src/lib.rs:367)Insufficient documentation on Engine::with_limits(crates/runtime/src/lib.rs:105)Missing test for headless engine behavior with precompiled metered modules(crates/runtime/src/lib.rs:1012)Operation limit of zero allows unbounded execution(crates/runtime/src/logic.rs:165)Metering points reset on every method call including hooks(crates/runtime/src/lib.rs:374)Uniform cost function may allow resource amplification(crates/runtime/src/lib.rs:138)Medium Severity(crates/runtime/src/lib.rs:377)Medium Severity(crates/runtime/src/lib.rs:377)Medium Severity(crates/runtime/src/lib.rs:378)Low Severity(crates/runtime/src/lib.rs:127)High Severity(crates/runtime/src/lib.rs:376)Medium Severity(crates/runtime/src/lib.rs:1141)Low Severity(crates/runtime/Cargo.toml:52)
🆕 New Issues
These issues were found in the latest changes:
🟡 Warning (2)
1. Excessive warning logging on each headless engine creation
File: crates/runtime/src/lib.rs (line 168-172) | Consensus: 1/1 agents ✓
The tracing::warn! is called every time headless() is invoked. In production scenarios where headless engines are created frequently (e.g., for each precompiled module load), this will spam the logs with repeated identical warnings.
Suggested fix:
Use a `once_cell` or `std::sync::Once` to emit the warning only once, or use `tracing::warn_span!` with a deduplication mechanism. Alternatively, consider logging at `debug!` level with the security implications documented elsewhere.
Found by: cursor-agent
2. Shared operation budget between registration hooks and user methods may cause unexpected timeouts
File: crates/runtime/src/lib.rs (line 364-379) | Consensus: 1/1 agents ✓
The metering budget is set once before calling __calimero_register_merge hook AND the user method. If the registration hook consumes significant operations, the user method may timeout unexpectedly even if the user's code is efficient. This could be exploited by a malicious module with expensive registration hooks, or cause confusing errors for legitimate modules.
Suggested fix:
Consider either: (1) setting a separate smaller budget for hooks, (2) resetting the budget after hooks complete, or (3) documenting this limitation more prominently and adjusting the default budget to account for hook overhead.
Found by: cursor-agent
💡 Suggestion (2)
1. The 'profiling' feature is now effectively empty and misleading
File: crates/runtime/Cargo.toml (line 50-52) | Consensus: 1/1 agents ✓
The profiling = [] feature no longer gates any dependencies since wasmer-compiler-cranelift is now always included. The feature only affects runtime behavior via ENABLE_WASMER_PROFILING env var check. Users may expect enabling this feature to have more impact.
Suggested fix:
Either remove the feature entirely and rely solely on the env var, or rename it to clarify it only enables the env var check (e.g., `profiling-support`). Update the comment to better explain what the feature actually does now.
Found by: cursor-agent
2. Original error is logged but not preserved when metering is exhausted
File: crates/runtime/src/lib.rs (line 456-476) | Consensus: 1/1 agents ✓
When metering is exhausted, the original RuntimeError is logged at debug level but then replaced with ExecutionTimeout. This makes debugging harder as the actual trap location/cause is lost in the returned error. The original error might contain useful stack trace information.
Suggested fix:
Consider including the original error information in the `ExecutionTimeout` variant, or return a new error type that wraps both the timeout indication and the underlying error details.
Found by: cursor-agent
📝 Nitpick (1)
1. Uniform cost function may not accurately reflect operation costs
File: crates/runtime/src/lib.rs (line 133-133) | Consensus: 1/1 agents ✓
The metering cost function |_op| 1 assigns the same cost to all WASM operations. In reality, memory operations, calls, and branches have different performance characteristics. This could allow adversarial modules to perform more expensive operations within the same budget.
Suggested fix:
Consider implementing a more nuanced cost function that assigns higher costs to memory operations, function calls, and indirect calls. Wasmer's metering middleware supports per-opcode costs.
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-bd149f43
| tracing.workspace = true | ||
| ureq.workspace = true | ||
| wasmer.workspace = true | ||
| wasmer-compiler-cranelift.workspace = true |
There was a problem hiding this comment.
💡 Cranelift compiler changed from optional to required dependency
The wasmer-compiler-cranelift dependency was previously optional (gated by the profiling feature) but is now always included. This increases binary size for all users, even those who don't need profiling or could use precompiled modules. The feature comment acknowledges this but the tradeoff may not be ideal for all use cases.
Suggested fix:
Consider making the metering/Cranelift dependency optional with a separate feature flag (e.g., `execution-limits`), allowing users who only work with trusted precompiled modules to opt out.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
| @@ -394,12 +458,47 @@ impl Module { | |||
| "WASM method execution failed" | |||
There was a problem hiding this comment.
📝 Redundant logging at both info and error levels for same event
When metering is exhausted, the code logs at info level with the original error details, then immediately logs at error level. This creates duplicate log entries for the same event, which can clutter logs and make debugging harder.
Suggested fix:
Consolidate into a single `warn!` or `error!` log statement that includes both the context and the original error information.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
Merobox Proposals Workflows FailedThe following proposal workflow(s) failed:
Please check the workflow logs for more details. |
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 111.4s
🟡 Issues pending resolution
✅ 37 Fixed | 🆕 5 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
Low Severity(crates/runtime/src/lib.rs:176)Low Severity(crates/runtime/src/errors.rs:189)High Severity(crates/runtime/src/lib.rs:148)Medium Severity(crates/runtime/src/lib.rs:269)Dependency version not using workspace(crates/runtime/Cargo.toml:28)Headless engine silently disables operation limits(crates/runtime/src/lib.rs:156)Headless engine metering inconsistency with precompiled modules(crates/runtime/src/lib.rs:145)Error context lost when metering exhausted(crates/runtime/src/lib.rs:458)Metering exhaustion check may miss edge cases on error path(crates/runtime/src/lib.rs:367)Insufficient documentation on Engine::with_limits(crates/runtime/src/lib.rs:105)Missing test for headless engine behavior with precompiled metered modules(crates/runtime/src/lib.rs:1015)Operation limit of zero allows unbounded execution(crates/runtime/src/logic.rs:176)Metering points reset on every method call including hooks(crates/runtime/src/lib.rs:376)Uniform cost function may allow resource amplification(crates/runtime/src/lib.rs:138)Medium Severity(crates/runtime/src/lib.rs:379)Medium Severity(crates/runtime/src/lib.rs:379)Medium Severity(crates/runtime/src/lib.rs:380)Low Severity(crates/runtime/src/lib.rs:127)High Severity(crates/runtime/src/lib.rs:378)Medium Severity(crates/runtime/src/lib.rs:1144)Low Severity(crates/runtime/Cargo.toml:54)Excessive warning logging on each headless engine creation(crates/runtime/src/lib.rs:170)Shared operation budget between registration hooks and user methods may cause unexpected timeouts(crates/runtime/src/lib.rs:366)The 'profiling' feature is now effectively empty and misleading(crates/runtime/Cargo.toml:50)Original error is logged but not preserved when metering is exhausted(crates/runtime/src/lib.rs:458)Uniform cost function may not accurately reflect operation costs(crates/runtime/src/lib.rs:133)Warning log emitted on every headless engine creation(crates/runtime/src/lib.rs:170)Metering exhaustion check may mask underlying errors(crates/runtime/src/lib.rs:461)Default max_operations lacks practical context(crates/runtime/src/logic.rs:114)Uniform cost function may not accurately model execution time(crates/runtime/src/lib.rs:137)Operation budget shared across hooks and user method without visibility(crates/runtime/src/lib.rs:366)Medium Severity(crates/runtime/src/lib.rs:478)Misleading error name and message for operation limit(crates/runtime/src/errors.rs:188)Security-relevant headless engine warning logged at debug level(crates/runtime/src/lib.rs:165)Cranelift compiler changed from optional to required dependency(crates/runtime/Cargo.toml:28)Metering budget set after instance creation but before hook calls(crates/runtime/src/lib.rs:366)Redundant logging at both info and error levels for same event(crates/runtime/src/lib.rs:458)
🆕 New Issues
These issues were found in the latest changes:
🟡 Warning (2)
1. Headless engine precompiled module metering behavior is inconsistent with docs
File: crates/runtime/src/lib.rs (line 162-176) | Consensus: 1/1 agents ✓
The doc comment on lines 154-161 states that modules compiled with metering will still trap when loaded via headless engine, but those traps will surface as generic runtime errors. However, because headless engines set max_operations = 0 (line 166), the check at line 458 (if max_ops > 0) will never execute, meaning the code won't even attempt to detect metering exhaustion. The error will indeed be generic, but this is because of the runtime check, not just because headless engines can't detect metering state.
Suggested fix:
Clarify in the documentation that the runtime explicitly skips metering detection for headless engines, or consider attempting metering detection regardless of the local max_ops value to provide better error messages for precompiled-with-metering modules.
Found by: cursor-agent
2. Metering enforcement depends on compile-time configuration
File: crates/runtime/src/lib.rs (line 265-280) | Consensus: 1/1 agents ✓
The max_ops value used at runtime (line 268) comes from self.limits.max_operations, which reflects the limits at the time the Engine was created and the module compiled. If a Module is somehow reused with different VMLimits, or if the module was compiled with max_operations=0 and later executed with the expectation of metering, the set_remaining_points call will have no effect because metering instructions aren't embedded in the compiled WASM. While the current code flow appears consistent (Module carries its Engine's limits), this invariant isn't enforced and could lead to a false sense of security if the architecture changes.
Suggested fix:
Consider adding a debug assertion or documentation that explicitly validates the module was compiled with metering enabled when max_ops > 0 at runtime, or store a flag in the Module indicating whether metering was enabled at compile time.
Found by: cursor-agent
💡 Suggestion (2)
1. Original trap error is logged but not preserved in error chain
File: crates/runtime/src/lib.rs (line 458-476) | Consensus: 1/1 agents ✓
When metering exhaustion is detected, the original error is logged at info level (line 465-470) but the returned ExecutionTimeout error doesn't preserve it. This makes production debugging harder since logs might be at different verbosity levels or might not correlate easily with the returned error.
Suggested fix:
Consider wrapping the original error in the ExecutionTimeout variant or adding a source field to preserve the error chain, e.g., `ExecutionTimeout { original: Option<Box<dyn Error>> }`.
Found by: cursor-agent
2. max_operations field documentation could clarify hook budget impact
File: crates/runtime/src/logic.rs (line 168-191) | Consensus: 1/1 agents ✓
The documentation mentions that hooks share the operation budget, but doesn't provide guidance on how to account for this. Users setting tight operation limits might be surprised when their main method gets fewer operations than expected because hooks consumed part of the budget.
Suggested fix:
Add guidance suggesting users add a buffer (e.g., 10-20%) to their operation limit if they use registration hooks, or consider providing separate hook and method budgets in future versions.
Found by: cursor-agent
📝 Nitpick (1)
1. wasmer-compiler-cranelift changed from optional to required dependency
File: crates/runtime/Cargo.toml (line 28-29) | Consensus: 1/1 agents ✓
The dependency wasmer-compiler-cranelift was previously optional (gated by the profiling feature) but is now always required. While this is necessary for metering functionality, it increases the minimum dependency footprint for all users of this crate, even those who might want a minimal runtime without metering.
Suggested fix:
Consider documenting this change in a CHANGELOG or migration guide, and optionally provide a feature flag to disable metering for users who need minimal dependencies and only run trusted code.
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-e107467f
| @@ -218,6 +265,9 @@ impl Module { | |||
|
|
|||
There was a problem hiding this comment.
🟡 Metering enforcement depends on compile-time configuration
The max_ops value used at runtime (line 268) comes from self.limits.max_operations, which reflects the limits at the time the Engine was created and the module compiled. If a Module is somehow reused with different VMLimits, or if the module was compiled with max_operations=0 and later executed with the expectation of metering, the set_remaining_points call will have no effect because metering instructions aren't embedded in the compiled WASM. While the current code flow appears consistent (Module carries its Engine's limits), this invariant isn't enforced and could lead to a false sense of security if the architecture changes.
Suggested fix:
Consider adding a debug assertion or documentation that explicitly validates the module was compiled with metering enabled when max_ops > 0 at runtime, or store a flag in the Module indicating whether metering was enabled at compile time.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
cursor
bot
force-pushed
the
bounty-fix/add-timeout-configuration-for-wasm-execu-ml6pzeyr
branch
from
f9ca083 to
24bd6b7
Compare
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 265.5s
🟡 Issues pending resolution
✅ 42 Fixed | 🆕 5 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
Low Severity(crates/runtime/src/lib.rs:176)Low Severity(crates/runtime/src/errors.rs:189)High Severity(crates/runtime/src/lib.rs:148)Medium Severity(crates/runtime/src/lib.rs:269)Dependency version not using workspace(crates/runtime/Cargo.toml:28)Headless engine silently disables operation limits(crates/runtime/src/lib.rs:156)Headless engine metering inconsistency with precompiled modules(crates/runtime/src/lib.rs:145)Error context lost when metering exhausted(crates/runtime/src/lib.rs:458)Metering exhaustion check may miss edge cases on error path(crates/runtime/src/lib.rs:367)Insufficient documentation on Engine::with_limits(crates/runtime/src/lib.rs:105)Missing test for headless engine behavior with precompiled metered modules(crates/runtime/src/lib.rs:1015)Operation limit of zero allows unbounded execution(crates/runtime/src/logic.rs:176)Metering points reset on every method call including hooks(crates/runtime/src/lib.rs:376)Uniform cost function may allow resource amplification(crates/runtime/src/lib.rs:138)Medium Severity(crates/runtime/src/lib.rs:379)Medium Severity(crates/runtime/src/lib.rs:379)Medium Severity(crates/runtime/src/lib.rs:380)Low Severity(crates/runtime/src/lib.rs:127)High Severity(crates/runtime/src/lib.rs:378)Medium Severity(crates/runtime/src/lib.rs:1144)Low Severity(crates/runtime/Cargo.toml:54)Excessive warning logging on each headless engine creation(crates/runtime/src/lib.rs:170)Shared operation budget between registration hooks and user methods may cause unexpected timeouts(crates/runtime/src/lib.rs:366)The 'profiling' feature is now effectively empty and misleading(crates/runtime/Cargo.toml:50)Original error is logged but not preserved when metering is exhausted(crates/runtime/src/lib.rs:458)Uniform cost function may not accurately reflect operation costs(crates/runtime/src/lib.rs:133)Warning log emitted on every headless engine creation(crates/runtime/src/lib.rs:170)Metering exhaustion check may mask underlying errors(crates/runtime/src/lib.rs:461)Default max_operations lacks practical context(crates/runtime/src/logic.rs:114)Uniform cost function may not accurately model execution time(crates/runtime/src/lib.rs:137)Operation budget shared across hooks and user method without visibility(crates/runtime/src/lib.rs:366)Medium Severity(crates/runtime/src/lib.rs:478)Misleading error name and message for operation limit(crates/runtime/src/errors.rs:188)Security-relevant headless engine warning logged at debug level(crates/runtime/src/lib.rs:165)Cranelift compiler changed from optional to required dependency(crates/runtime/Cargo.toml:28)Metering budget set after instance creation but before hook calls(crates/runtime/src/lib.rs:366)Redundant logging at both info and error levels for same event(crates/runtime/src/lib.rs:458)Headless engine precompiled module metering behavior is inconsistent with docs(crates/runtime/src/lib.rs:162)Metering enforcement depends on compile-time configuration(crates/runtime/src/lib.rs:265)Original trap error is logged but not preserved in error chain(crates/runtime/src/lib.rs:458)max_operations field documentation could clarify hook budget impact(crates/runtime/src/logic.rs:168)wasmer-compiler-cranelift changed from optional to required dependency(crates/runtime/Cargo.toml:28)
🆕 New Issues
These issues were found in the latest changes:
🟡 Warning (2)
1. Operation budget reset after WASM start function execution
File: crates/runtime/src/lib.rs (line 366-378) | Consensus: 1/1 agents ✓
The metering points are set via set_remaining_points() AFTER Instance::new() completes. WASM start functions (if present) execute during instantiation, consuming operations from the initial compile-time budget. Then the budget is reset to max_ops, giving hooks and the main method a fresh budget. This means start function operations don't count against the main method's limit, potentially allowing a malicious module to perform significant computation in its start function without affecting the visible operation budget.
Suggested fix:
Document this behavior explicitly, or consider checking remaining points after instantiation to detect start function budget consumption. If the start function nearly exhausted the budget, the operation could be rejected early.
Found by: cursor-agent
2. Headless engine silently disables all operation limit enforcement
File: crates/runtime/src/lib.rs (line 159-175) | Consensus: 1/1 agents ✓
The headless() method sets max_operations = 0 which disables metering middleware entirely. While documented in comments, there's no runtime warning when a headless engine is used, and no way to verify if a precompiled module was originally compiled with metering. If a module compiled without metering is loaded, infinite loops will run indefinitely.
Suggested fix:
Consider logging a warning (not just debug) when creating headless engines, or provide a method to check if a serialized module was compiled with metering enabled. For defense in depth, consider adding a separate thread-based timeout as a backstop.
Found by: cursor-agent
💡 Suggestion (2)
1. Redundant logging at multiple levels for metering exhaustion
File: crates/runtime/src/lib.rs (line 468-480) | Consensus: 1/1 agents ✓
When metering is exhausted, the code logs at both info level (line 471-476) and error level (line 479). This creates duplicate log entries for the same event. The info log includes the original error details while the error log has less context.
Suggested fix:
Consolidate into a single log statement at `error` or `warn` level that includes all relevant information (context_id, method, original_error). Remove the redundant logging.
Found by: cursor-agent
2. Misleading error variant name - 'ExecutionTimeout' implies time-based limit
File: crates/runtime/src/errors.rs (line 188-189) | Consensus: 1/1 agents ✓
The error variant ExecutionTimeout with message 'execution exceeded timeout' suggests a time-based limit, but this is actually triggered by exceeding an operation count limit. This naming inconsistency could confuse users debugging WASM execution failures.
Suggested fix:
Consider renaming to `OperationLimitExceeded` or `ExecutionLimitExceeded` with a message like 'execution exceeded operation limit' to accurately reflect the metering-based nature of this limit.
Found by: cursor-agent
📝 Nitpick (1)
1. Default max_operations value lacks empirical justification
File: crates/runtime/src/logic.rs (line 112-121) | Consensus: 1/1 agents ✓
The default of 300 million operations is documented as corresponding to '10-60 seconds of wall-clock time' but this is a very wide range (6x variance). The comment suggests adjusting based on profiling data, but no baseline benchmarks or methodology are provided to help users calibrate this value for their workloads.
Suggested fix:
Consider adding a reference to benchmarks or providing more specific guidance (e.g., 'arithmetic-heavy loops: ~30M ops/sec, memory-heavy: ~5M ops/sec on reference hardware').
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-f1f2981e
| @@ -314,6 +366,18 @@ impl Module { | |||
| // Attach memory to VMLogic, which will clean it up in finish() | |||
There was a problem hiding this comment.
🟡 Operation budget reset after WASM start function execution
The metering points are set via set_remaining_points() AFTER Instance::new() completes. WASM start functions (if present) execute during instantiation, consuming operations from the initial compile-time budget. Then the budget is reset to max_ops, giving hooks and the main method a fresh budget. This means start function operations don't count against the main method's limit, potentially allowing a malicious module to perform significant computation in its start function without affecting the visible operation budget.
Suggested fix:
Document this behavior explicitly, or consider checking remaining points after instantiation to detect start function budget consumption. If the start function nearly exhausted the budget, the operation could be rejected early.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
| /// middleware must be applied at compile time. Modules loaded with a headless engine | ||
| /// will run without operation limits regardless of `VMLimits.max_operations`. | ||
| /// | ||
| /// This means: |
There was a problem hiding this comment.
🟡 Headless engine silently disables all operation limit enforcement
The headless() method sets max_operations = 0 which disables metering middleware entirely. While documented in comments, there's no runtime warning when a headless engine is used, and no way to verify if a precompiled module was originally compiled with metering. If a module compiled without metering is loaded, infinite loops will run indefinitely.
Suggested fix:
Consider logging a warning (not just debug) when creating headless engines, or provide a method to check if a serialized module was compiled with metering enabled. For defense in depth, consider adding a separate thread-based timeout as a backstop.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
| // Log the original error at info level for production debugging | ||
| // The error type from metering exhaustion is typically an unreachable trap | ||
| info!( | ||
| %context_id, |
There was a problem hiding this comment.
💡 Redundant logging at multiple levels for metering exhaustion
When metering is exhausted, the code logs at both info level (line 471-476) and error level (line 479). This creates duplicate log entries for the same event. The info log includes the original error details while the error log has less context.
Suggested fix:
Consolidate into a single log statement at `error` or `warn` level that includes all relevant information (context_id, method, original_error). Remove the redundant logging.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
| Unreachable, | ||
| #[error("unaligned atomic operation")] | ||
| UnalignedAtomic, | ||
| #[error("execution exceeded timeout")] |
There was a problem hiding this comment.
💡 Misleading error variant name - 'ExecutionTimeout' implies time-based limit
The error variant ExecutionTimeout with message 'execution exceeded timeout' suggests a time-based limit, but this is actually triggered by exceeding an operation count limit. This naming inconsistency could confuse users debugging WASM execution failures.
Suggested fix:
Consider renaming to `OperationLimitExceeded` or `ExecutionLimitExceeded` with a message like 'execution exceeded operation limit' to accurately reflect the metering-based nature of this limit.
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
| const DEFAULT_MAX_BLOB_CHUNK_SIZE_MIB: u64 = 10; | ||
| /// Default maximum method name length in bytes. | ||
| const DEFAULT_MAX_METHOD_NAME_LENGTH: u64 = 256; | ||
| /// Default maximum operations for WASM execution (300 million). |
There was a problem hiding this comment.
📝 Default max_operations value lacks empirical justification
The default of 300 million operations is documented as corresponding to '10-60 seconds of wall-clock time' but this is a very wide range (6x variance). The comment suggests adjusting based on profiling data, but no baseline benchmarks or methodology are provided to help users calibrate this value for their workloads.
Suggested fix:
Consider adding a reference to benchmarks or providing more specific guidance (e.g., 'arithmetic-heavy loops: ~30M ops/sec, memory-heavy: ~5M ops/sec on reference hardware').
There was a problem hiding this comment.
✅ Resolved - This issue has been addressed in the latest changes.
Co-authored-by: sandi <sandi@calimero.network>
- Add wasmer-middlewares to workspace dependencies for version consistency - Add documentation for Engine::with_limits explaining compile-time metering - Add security warning log when creating headless engines - Improve error handling by logging original error before returning ExecutionTimeout - Document that max_operations budget is shared with registration hooks - Expand max_operations field documentation with important usage notes Co-authored-by: sandi <sandi@calimero.network>
Co-authored-by: sandi <sandi@calimero.network>
Co-authored-by: sandi <sandi@calimero.network>
Co-authored-by: sandi <sandi@calimero.network>
cursor
bot
force-pushed
the
bounty-fix/add-timeout-configuration-for-wasm-execu-ml6pzeyr
branch
from
24bd6b7 to
4f4bc09
Compare
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 118.0s
🟡 Issues pending resolution
✅ 47 Fixed | 🆕 5 New
✅ Fixed Issues
The following issues from previous reviews have been addressed:
Low Severity(crates/runtime/src/lib.rs:176)Low Severity(crates/runtime/src/errors.rs:191)High Severity(crates/runtime/src/lib.rs:148)Medium Severity(crates/runtime/src/lib.rs:269)Dependency version not using workspace(crates/runtime/Cargo.toml:28)Headless engine silently disables operation limits(crates/runtime/src/lib.rs:156)Headless engine metering inconsistency with precompiled modules(crates/runtime/src/lib.rs:145)Error context lost when metering exhausted(crates/runtime/src/lib.rs:490)Metering exhaustion check may miss edge cases on error path(crates/runtime/src/lib.rs:367)Insufficient documentation on Engine::with_limits(crates/runtime/src/lib.rs:105)Missing test for headless engine behavior with precompiled metered modules(crates/runtime/src/lib.rs:1047)Operation limit of zero allows unbounded execution(crates/runtime/src/logic.rs:189)Metering points reset on every method call including hooks(crates/runtime/src/lib.rs:408)Uniform cost function may allow resource amplification(crates/runtime/src/lib.rs:138)Medium Severity(crates/runtime/src/lib.rs:411)Medium Severity(crates/runtime/src/lib.rs:411)Medium Severity(crates/runtime/src/lib.rs:412)Low Severity(crates/runtime/src/lib.rs:127)High Severity(crates/runtime/src/lib.rs:410)Medium Severity(crates/runtime/src/lib.rs:1337)Low Severity(crates/runtime/Cargo.toml:54)Excessive warning logging on each headless engine creation(crates/runtime/src/lib.rs:170)Shared operation budget between registration hooks and user methods may cause unexpected timeouts(crates/runtime/src/lib.rs:398)The 'profiling' feature is now effectively empty and misleading(crates/runtime/Cargo.toml:50)Original error is logged but not preserved when metering is exhausted(crates/runtime/src/lib.rs:490)Uniform cost function may not accurately reflect operation costs(crates/runtime/src/lib.rs:133)Warning log emitted on every headless engine creation(crates/runtime/src/lib.rs:170)Metering exhaustion check may mask underlying errors(crates/runtime/src/lib.rs:493)Default max_operations lacks practical context(crates/runtime/src/logic.rs:116)Uniform cost function may not accurately model execution time(crates/runtime/src/lib.rs:137)Operation budget shared across hooks and user method without visibility(crates/runtime/src/lib.rs:398)Medium Severity(crates/runtime/src/lib.rs:510)Misleading error name and message for operation limit(crates/runtime/src/errors.rs:190)Security-relevant headless engine warning logged at debug level(crates/runtime/src/lib.rs:165)Cranelift compiler changed from optional to required dependency(crates/runtime/Cargo.toml:28)Metering budget set after instance creation but before hook calls(crates/runtime/src/lib.rs:398)Redundant logging at both info and error levels for same event(crates/runtime/src/lib.rs:490)Headless engine precompiled module metering behavior is inconsistent with docs(crates/runtime/src/lib.rs:162)Metering enforcement depends on compile-time configuration(crates/runtime/src/lib.rs:297)Original trap error is logged but not preserved in error chain(crates/runtime/src/lib.rs:490)max_operations field documentation could clarify hook budget impact(crates/runtime/src/logic.rs:181)wasmer-compiler-cranelift changed from optional to required dependency(crates/runtime/Cargo.toml:28)Operation budget reset after WASM start function execution(crates/runtime/src/lib.rs:398)Headless engine silently disables all operation limit enforcement(crates/runtime/src/lib.rs:159)Redundant logging at multiple levels for metering exhaustion(crates/runtime/src/lib.rs:500)Misleading error variant name - 'ExecutionTimeout' implies time-based limit(crates/runtime/src/errors.rs:190)Default max_operations value lacks empirical justification(crates/runtime/src/logic.rs:114)
🆕 New Issues
These issues were found in the latest changes:
🟡 Warning (2)
1. Public Engine::new() allows metering bypass
File: crates/runtime/src/lib.rs (line 105-112) | Consensus: 1/1 agents ✓
The Engine::new() function remains public and accepts any wasmer::Engine. If a caller passes an engine without the metering middleware but provides VMLimits with max_operations > 0, the code will attempt to call metering::set_remaining_points() on a module that wasn't compiled with metering. This will either fail at runtime or silently not enforce limits, creating a security gap where operation limits appear to be configured but aren't actually enforced.
Suggested fix:
Consider making `Engine::new()` private or adding a `#[doc(hidden)]` attribute, and encouraging all callers to use `Engine::with_limits()` or `Engine::default()`. Alternatively, add validation to detect this mismatch at runtime.
Found by: cursor-agent
2. Metering set after Instance creation may miss start functions
File: crates/runtime/src/lib.rs (line 398-414) | Consensus: 1/1 agents ✓
The metering::set_remaining_points() call happens after Instance::new() completes. If the WASM module has a start function (declared via (start ...) in WAT), it executes during instantiation before the metering budget is explicitly set. While the metering middleware should still count operations with its compile-time default, this could lead to inconsistent behavior where the start function uses a different budget than expected.
Suggested fix:
Document this behavior explicitly, or investigate if there's a way to set the metering points before instantiation. Consider whether start functions pose a denial-of-service risk in your threat model.
Found by: cursor-agent
💡 Suggestion (2)
1. Uniform cost function doesn't model real execution time
File: crates/runtime/src/lib.rs (line 134-140) | Consensus: 1/1 agents ✓
The metering uses |_op| 1 which assigns equal cost to all WASM operations. Memory operations, indirect calls, and complex floating-point operations are significantly more expensive in wall-clock time than simple arithmetic. An attacker could craft code that maximizes expensive operations while staying under the operation budget, potentially causing longer-than-expected execution times.
Suggested fix:
Consider implementing a weighted cost function based on operation categories (e.g., memory ops = 2, calls = 5, arithmetic = 1). Document the current uniform cost model's limitations and the rationale for using it.
Found by: cursor-agent
2. Metering exhaustion check may miss edge cases
File: crates/runtime/src/lib.rs (line 490-510) | Consensus: 1/1 agents ✓
The metering exhaustion check only runs when an error occurs (if let Err(err) = ...). If get_remaining_points fails (e.g., returns an unexpected state), the code falls through to the generic error handling. The current implementation assumes get_remaining_points will reliably return Exhausted when limits are hit, but there's no handling for potential edge cases where the metering state is indeterminate.
Suggested fix:
Add a match arm or error logging for unexpected metering states. Consider also checking metering state on successful completion to detect near-exhaustion scenarios for monitoring purposes.
Found by: cursor-agent
📝 Nitpick (1)
1. Default max_operations timing guidance is imprecise
File: crates/runtime/src/logic.rs (line 111-126) | Consensus: 1/1 agents ✓
The documentation states '300M operations corresponds to roughly 10-60 seconds of wall-clock time' which is a very wide range (6x variance). This could lead to confusion when operators try to set appropriate limits. The variance depends heavily on the operation mix, host hardware, and WASM module characteristics.
Suggested fix:
Either narrow the guidance with specific benchmarks (e.g., 'pure arithmetic loops: ~10s, memory-intensive code: ~60s on X hardware') or provide a way to profile operation costs for specific workloads. Consider adding a debug mode that reports operations/second for calibration.
Found by: cursor-agent
🤖 Generated by AI Code Reviewer | Review ID: review-166dfa13
| @@ -107,29 +105,78 @@ impl Engine { | |||
| Self { limits, engine } | |||
There was a problem hiding this comment.
🟡 Public Engine::new() allows metering bypass
The Engine::new() function remains public and accepts any wasmer::Engine. If a caller passes an engine without the metering middleware but provides VMLimits with max_operations > 0, the code will attempt to call metering::set_remaining_points() on a module that wasn't compiled with metering. This will either fail at runtime or silently not enforce limits, creating a security gap where operation limits appear to be configured but aren't actually enforced.
Suggested fix:
Consider making `Engine::new()` private or adding a `#[doc(hidden)]` attribute, and encouraging all callers to use `Engine::with_limits()` or `Engine::default()`. Alternatively, add validation to detect this mismatch at runtime.
| @@ -346,6 +398,18 @@ impl Module { | |||
| // Attach memory to VMLogic, which will clean it up in finish() | |||
There was a problem hiding this comment.
🟡 Metering set after Instance creation may miss start functions
The metering::set_remaining_points() call happens after Instance::new() completes. If the WASM module has a start function (declared via (start ...) in WAT), it executes during instantiation before the metering budget is explicitly set. While the metering middleware should still count operations with its compile-time default, this could lead to inconsistent behavior where the start function uses a different budget than expected.
Suggested fix:
Document this behavior explicitly, or investigate if there's a way to set the metering points before instantiation. Consider whether start functions pose a denial-of-service risk in your threat model.
| config.enable_perfmap(); | ||
| return wasmer::Engine::from(config); | ||
| compiler.enable_perfmap(); | ||
| } |
There was a problem hiding this comment.
💡 Uniform cost function doesn't model real execution time
The metering uses |_op| 1 which assigns equal cost to all WASM operations. Memory operations, indirect calls, and complex floating-point operations are significantly more expensive in wall-clock time than simple arithmetic. An attacker could craft code that maximizes expensive operations while staying under the operation budget, potentially causing longer-than-expected execution times.
Suggested fix:
Consider implementing a weighted cost function based on operation categories (e.g., memory ops = 2, calls = 5, arithmetic = 1). Document the current uniform cost model's limitations and the rationale for using it.
| @@ -426,12 +490,47 @@ impl Module { | |||
| "WASM method execution failed" | |||
There was a problem hiding this comment.
💡 Metering exhaustion check may miss edge cases
The metering exhaustion check only runs when an error occurs (if let Err(err) = ...). If get_remaining_points fails (e.g., returns an unexpected state), the code falls through to the generic error handling. The current implementation assumes get_remaining_points will reliably return Exhausted when limits are hit, but there's no handling for potential edge cases where the metering state is indeterminate.
Suggested fix:
Add a match arm or error logging for unexpected metering states. Consider also checking metering state on successful completion to detect near-exhaustion scenarios for monitoring purposes.
| @@ -111,6 +111,16 @@ const DEFAULT_MAX_BLOB_CHUNK_SIZE_MIB: u64 = 10; | |||
| const DEFAULT_MAX_METHOD_NAME_LENGTH: u64 = 256; | |||
There was a problem hiding this comment.
📝 Default max_operations timing guidance is imprecise
The documentation states '300M operations corresponds to roughly 10-60 seconds of wall-clock time' which is a very wide range (6x variance). This could lead to confusion when operators try to set appropriate limits. The variance depends heavily on the operation mix, host hardware, and WASM module characteristics.
Suggested fix:
Either narrow the guidance with specific benchmarks (e.g., 'pure arithmetic loops: ~10s, memory-intensive code: ~60s on X hardware') or provide a way to profile operation costs for specific workloads. Consider adding a debug mode that reports operations/second for calibration.
3 participants
N/A
Note
Medium Risk
Touches core WASM execution by introducing metering-based operation limits and a new
ExecutionTimeouttrap, which can change runtime behavior/performance and cause previously long-running contracts to start trapping.Overview
Adds a configurable WASM execution timeout by introducing
VMLimits.max_operations(default300_000_000) and compiling modules with Wasmer metering middleware so executions trap when the operation budget is exhausted.Updates the runtime to set/check remaining metering points around execution (including hooks), map exhaustion to a new
WasmTrap::ExecutionTimeout, and emit debug stats;Engine::headless()explicitly disables operation limiting and documents the security implications. Also adjusts dependencies/features to always include Cranelift + newwasmer-middlewares, and adds integration tests covering custom limits, infinite-loop termination, and disabled limits.Written by Cursor Bugbot for commit 4f4bc09. This will update automatically on new commits. Configure here.