Merge error handling refactor#27
Conversation
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
…x error codes Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Resolved merge conflicts in runtime error handling refactor: - builtins.rs: Integrated improved error handling with proper enum mapping - exec.rs: Fixed Context::new() return statement syntax - lib.rs: Merged constant definitions with error exports Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changed Files
|
Summary of ChangesHello @tcrypt25519, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the error handling mechanisms within the runtime. The primary goal is to move away from abrupt panics towards a more robust and explicit error propagation model using Rust's Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR refactors runtime and builder/engine codepaths to use structured, non-panicking error handling, introducing a shared RuntimeError/Result and improving FFI builtin pointer validation and error code documentation.
Changes:
- Added
crates/jet_runtime::errorwithRuntimeError+Result, and re-exported them fromjet_runtime. - Made
jet_runtime::exec::Context::new()and sub-context creation fallible and propagated errors throughjetbuilder/engine APIs. - Refined contract-call builtins with explicit error-code enums and null-pointer checks.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/jet_runtime/src/lib.rs | Exposes new runtime error module + re-exports RuntimeError/Result. |
| crates/jet_runtime/src/exec.rs | Makes context allocation fallible; makes sub-context creation return Result; avoids panics in Drop. |
| crates/jet_runtime/src/error.rs | Introduces RuntimeError and a runtime Result<T> alias via thiserror. |
| crates/jet_runtime/src/builtins.rs | Adds explicit FFI error codes + pointer validation and updates return-data-copy error handling/docs. |
| crates/jet_runtime/Cargo.toml | Adds thiserror dependency for the new error module. |
| crates/jet/src/engine/mod.rs | Propagates runtime/builder errors instead of unwrapping; adapts to fallible Context::new()/Env::new(). |
| crates/jet/src/builder/mod.rs | Adds Runtime error variant wrapping jet_runtime::RuntimeError. |
| crates/jet/src/builder/manager.rs | Replaces unwraps in IR printing/verification with non-panicking handling. |
| crates/jet/src/builder/env.rs | Makes Env::new fallible when runtime symbols can’t be resolved. |
| crates/jet/src/builder/contract.rs | Replaces unwraps with invariant violations; makes code-block creation fallible. |
| crates/jet/src/bin/jetdbg.rs | Avoids unwrap on logger init (but current control flow is awkward). |
Comments suppressed due to low confidence (1)
crates/jet_runtime/src/builtins.rs:165
- Several
u32additions here can overflow (src_offset + requested_ret_len,ret_offset + requested_ret_len,dest_offset + requested_ret_len). In release builds that can wrap and bypass bounds checks, leading to panics or out-of-bounds slicing. Usechecked_add/checked_sub(or widen tou64/usize) and return an error code on overflow before computing ranges.
if src_offset + requested_ret_len > ret_len {
return CopyError::BoundsCheckFailed as u8;
}
let ret_offset_end = ret_offset + requested_ret_len;
if ret_offset_end > ret_len {
return CopyError::BoundsCheckFailed as u8;
}
// TODO: Enable this check after adding memory len handling
// if ret_offset_end > mem_len {
// return 3;
// }
// Ensure memory is large enough for the write
let required_memory_len = dest_offset + requested_ret_len;
if ctx.memory_len() < required_memory_len {
There was a problem hiding this comment.
Code Review
This pull request significantly refactors error handling across the jet and jet_runtime crates, replacing panics (unwrap, expect, panic!) with Result-based propagation and introducing structured RuntimeErrors, which greatly improves robustness. However, critical vulnerabilities were identified in the jet_contract_call_return_data_copy builtin function's bounds checking logic. These checks are susceptible to integer overflows and fail to validate the contract-supplied return length against memory capacity, potentially leading to information leaks (arbitrary memory reads) or denial of service (panics). Additionally, the review includes a suggestion to improve error handling consistency in the jetdbg binary and points out a potential logic bug in a bounds check within jet_runtime's built-in functions.
MSTORE does not update memory_len, so checking ret_offset + ret_len <= memory_len always fails. This check was deliberately omitted in the original code with a TODO comment. Restore the TODO until memory tracking is implemented.
* Add error handling to jet_runtime and jet crates Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * Address code review feedback - optimize unreachable error checks Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * Improve safety comments and error logging per code review Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * Replace unwrap_unchecked with regular unwrap and document error codes Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * Address PR review comments: remove unwrap, add pointer validation, fix error codes Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * Convert error codes to enums and fix duplicate -1 values Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * fix * style: apply rustfmt formatting 🤖 * fix: remove unused jet_runtime self import in engine/mod.rs * fix: remove unused Style import in builder/manager.rs * fix: remove invalid memory_len bounds check in return_data_copy_impl MSTORE does not update memory_len, so checking ret_offset + ret_len <= memory_len always fails. This check was deliberately omitted in the original code with a TODO comment. Restore the TODO until memory tracking is implemented. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary
Merges the
copilot/refactor-error-handlingbranch which introduces comprehensive error handling improvements to the runtime.Changes
error.rswithRuntimeErrorandResulttypesContext::new()and sub-context creationTest plan
🤖 Generated with Claude Code