fix: Hansde in-instruction branches.

[tcrypt25519]

No description provided.

[semanticdiff-com]

Review changes with  

Changed Files

File	Status
crates/jet/src/builder/ops.rs	23% smaller
crates/jet/src/builder/contract.rs	0% smaller
crates/jet/tests/test_roms.rs	0% smaller

[gemini-code-assist]

Summary of Changes

Hello @tcrypt25519, I'm Gemini Code Assist¹! 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 enhances the robustness of the LLVM IR generation for EVM instructions by addressing potential undefined behavior in arithmetic and bitwise operations. By replacing conditional select statements with explicit branching, the generated code now strictly adheres to EVM specifications for edge cases like division by zero or out-of-bounds shifts, ensuring predictable and correct execution. Additionally, minor structural improvements were made to the contract building context for improved internal access and naming clarity.

Highlights

• LLVM Undefined Behavior Prevention: Refactored several EVM arithmetic and bitwise operations (DIV, SDIV, MOD, SMOD, ADDMOD, MULMOD, BYTE, SHL, SHR, SAR) to use explicit conditional branching instead of LLVM's select instruction for handling edge cases. This prevents potential 'poison' values or undefined behavior that can arise when LLVM's intrinsic operations are given invalid inputs (e.g., division by zero, large shift amounts), even if the result is later discarded by a select.
• Code Structure Refinement: Improved the internal structure of the contract builder by making the func field in BuildCtx publicly accessible within the crate and renaming basic_block to entry_block in the CodeBlock struct for better clarity and consistency.

Changelog

• crates/jet/src/builder/contract.rs
  • Updated visibility of the func field in BuildCtx to pub(crate).
  • Renamed the basic_block field to entry_block in the CodeBlock struct.
  • Adjusted all references and usages of basic_block to entry_block within the file.
• crates/jet/src/builder/ops.rs
  • Refactored div to use explicit conditional branching for division by zero, returning 0 as per EVM spec.
  • Refactored sdiv to use explicit conditional branching for division by zero and MIN_INT256 / -1 overflow.
  • Refactored _mod to use explicit conditional branching for modulo by zero, returning 0 as per EVM spec.
  • Refactored smod to use explicit conditional branching for modulo by zero and MIN_INT256 % -1 overflow.
  • Refactored addmod to use explicit conditional branching for modulo by zero (N == 0), returning 0.
  • Refactored mulmod to use explicit conditional branching for modulo by zero (N == 0), returning 0.
  • Refactored byte to use explicit conditional branching for out-of-bounds indices (i >= 32), returning 0.
  • Refactored shl to use explicit conditional branching and clamping for shift amounts >= 256, returning 0.
  • Refactored shr to use explicit conditional branching and clamping for shift amounts >= 256, returning 0.
  • Refactored sar to use explicit conditional branching and clamping for shift amounts >= 256 to 255.

Activity

• The pull request was created by tcrypt25519.
• Changes were implemented to fix potential LLVM undefined behavior in EVM instruction translation.
• Refactoring was performed in contract.rs to improve code organization and access.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces critical correctness fixes for edge cases in arithmetic and bitwise operations in LLVM, aiming to implement EVM semantics for division/modulo by zero, signed arithmetic overflows, and large shifts, and to address out-of-bounds BYTE access. However, it also introduces several critical security vulnerabilities and EVM compliance issues. The JIT-compiled code is vulnerable to host memory corruption via stack underflow and Use-After-Free (UAF) due to the engine's pointer-based stack design and lack of bounds checking. Additionally, the addmod and mulmod instructions do not correctly handle 256-bit overflows, violating the EVM specification. Notably, despite the PR's intent to address BYTE access, the BYTE instruction appears to have an indexing logic error that will result in incorrect execution. While the approach of using branches instead of select for division and modulo is excellent, a suggestion is to refactor repeated branching logic into a helper function for improved maintainability.

I am having trouble creating individual review comments. Click here to see my feedback.

crates/jet/src/builder/ops.rs (265)

The stack_pop_3 function (and other stack_pop_n variants) calls the runtime stack_pop function, which lacks bounds checking on the stack_ptr. If a contract executes an opcode that pops more elements than are currently on the stack, the stack_ptr will underflow (e.g., 0 - 1 becomes u32::MAX), leading to an out-of-bounds memory access on the host when accessing the stack array. This can result in a host process crash or potentially arbitrary code execution.

crates/jet/src/builder/ops.rs (288-289)

The EVM specification requires that the intermediate addition in addmod and multiplication in mulmod be performed with unlimited precision (or at least 512 bits) to prevent overflow before the modulo operation. This implementation performs the addition using 256-bit integers (i256), which will wrap around if the result exceeds 2^256 - 1. This leads to incorrect results, violating EVM compliance and potentially breaking smart contract logic.

crates/jet/src/builder/ops.rs (321-322)

The EVM specification requires that the intermediate multiplication in mulmod be performed with unlimited precision (or at least 512 bits) to prevent overflow before the modulo operation. This implementation performs the multiplication using 256-bit integers (i256), which will wrap around if the result exceeds 2^256 - 1. This leads to incorrect results, violating EVM compliance and potentially breaking smart contract logic.

crates/jet/src/builder/ops.rs (266-268)

The engine's design stores pointers to 32-byte words on the stack, which can point to the memory buffer (e.g., after an MLOAD). However, the memory buffer can be reallocated and moved in memory during a mem_expand call (triggered by MSTORE). When reallocation occurs, any existing pointers to the memory buffer stored on the stack become dangling pointers. Subsequent instructions that pop these pointers and call load_i256 will perform a Use-After-Free access on the host.

crates/jet/src/builder/ops.rs (521)

The BYTE instruction implementation uses 31 - index to access the byte in the 32-byte word. This indexing appears to be inconsistent with the EVM specification (where index 0 is the most significant byte) and the way words are stored in this engine (where push reverses the data). This will likely result in the instruction returning the wrong byte from the word.

crates/jet/src/builder/ops.rs (106-125)

The branching logic to handle division by zero is well-implemented. I've noticed that this pattern of creating zero_block, nonzero_block, and cont blocks is repeated across several functions (div, _mod, sdiv, smod, addmod, mulmod, and byte with a slight variation). To improve code reuse and maintainability, you could extract this logic into a generic build_zero_guard helper function.

Here's a possible implementation for the helper function:

fn build_zero_guard<'ctx, F>(
    bctx: &BuildCtx<'ctx, '_>,
    is_zero_cond: IntValue<'ctx>,
    op_name: &str,
    nonzero_builder: F,
) -> Result<(), Error>
where
    F: FnOnce(BasicBlock<'ctx>) -> Result<(), Error>,
{
    let zero = bctx.env.types().i256.const_zero();

    let zero_block = bctx.env.context().append_basic_block(bctx.func, &format!("{}_zero", op_name));
    let nonzero_block = bctx.env.context().append_basic_block(bctx.func, &format!("{}_nonzero", op_name));
    let cont_block = bctx.env.context().append_basic_block(bctx.func, &format!("{}_cont", op_name));

    bctx.builder.build_conditional_branch(is_zero_cond, zero_block, nonzero_block)?;

    // Zero case
    bctx.builder.position_at_end(zero_block);
    stack_push_int(bctx, zero)?;
    bctx.builder.build_unconditional_branch(cont_block)?;

    // Non-zero case
    bctx.builder.position_at_end(nonzero_block);
    nonzero_builder(cont_block)?;

    // Continue
    bctx.builder.position_at_end(cont_block);
    Ok(())
}

Using this helper, the div function can be simplified as shown in the suggestion.

    let b_is_zero =
        bctx.builder
            .build_int_compare(inkwell::IntPredicate::EQ, b, zero, "div_by_zero")?;

    build_zero_guard(bctx, b_is_zero, "div", |cont_block| {
        let result = bctx.builder.build_int_unsigned_div(a, b, "div_result")?;
        stack_push_int(bctx, result)?;
        bctx.builder.build_unconditional_branch(cont_block)?;
        Ok(())
    })

[Copilot]

Pull request overview

This PR fixes critical LLVM poison value issues in EVM opcode implementations by introducing explicit branches instead of select statements where necessary. The changes prevent undefined behavior that could occur when LLVM operations like division, modulo, or pointer arithmetic receive invalid operands.

Changes:

• Replaces select-based guards with explicit branches for division/modulo operations (DIV, SDIV, MOD, SMOD, ADDMOD, MULMOD) to avoid LLVM poison from zero divisors
• Implements overflow handling for SDIV and SMOD (MIN_INT256 / -1 edge case)
• Adds out-of-bounds branching for BYTE operation to prevent invalid GEP
• Implements shift amount clamping for SHL, SHR, and SAR to handle shifts >= 256
• Makes BuildCtx.func field public to enable basic block creation in opcode implementations
• Renames CodeBlock.basic_block field to entry_block for clarity

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
crates/jet/src/builder/ops.rs	Refactored DIV, SDIV, MOD, SMOD, ADDMOD, MULMOD to use explicit branching; added overflow guards for SDIV/SMOD; added out-of-bounds branching for BYTE; implemented shift clamping for SHL/SHR/SAR
crates/jet/src/builder/contract.rs	Made BuildCtx.func field public; renamed CodeBlock.basic_block to entry_block and updated all references

Comments suppressed due to low confidence (1)

crates/jet/src/builder/ops.rs:556

• The shift operations (SHL, SHR, SAR) now handle large shift amounts (>= 256) correctly by clamping the shift value. However, there are no tests covering this edge case. Consider adding tests for shift amounts >= 256 to verify the implementations handle this correctly per EVM spec.

    Ok(())
}

pub(crate) fn or(bctx: &BuildCtx<'_, '_>) -> Result<(), Error> {
    let (a, b) = stack_pop_2(bctx)?;
    let a = load_i256(bctx, a)?;
    let b = load_i256(bctx, b)?;
    let result = bctx.builder.build_or(a, b, "or_result")?;
    stack_push_int(bctx, result)?;
    Ok(())
}

pub(crate) fn xor(bctx: &BuildCtx<'_, '_>) -> Result<(), Error> {
    let (a, b) = stack_pop_2(bctx)?;