Skip to content

Conversation

@Ayushjhax
Copy link

Summary

Fixes out-of-bounds (OOB) shift operations in the VM interpreter that were causing UBSan errors.

Closes #3872

Problem

The VM interpreter had 4 arithmetic right shift (ARSH) operations using raw C shift operators without bounds checking:

  • 0xc4 (ARSH_IMM): (int)reg_dst >> imm
  • 0xc7 (ARSH64_IMM): (long)reg_dst >> imm
  • 0xcc (ARSH_REG): (int)reg_dst >> (uint)reg_src
  • 0xcf (ARSH64_REG): (long)reg_dst >> reg_src

When shift amounts ≥ bit width (32 for int, 64 for long), this caused undefined behavior and UBSan errors:

Solution

Replaced raw shift operators with safe helper functions from fd_bits.h:

  • fd_int_shift_right() for 32-bit shifts
  • fd_long_shift_right() for 64-bit shifts

These functions cap shift amounts to prevent UB while preserving arithmetic right shift semantics.

Changes

  • src/flamenco/vm/fd_vm_interp_core.c: Fixed 4 ARSH operations (lines 972, 985, 989, 1003)

Testing

  • No compilation errors
  • UBSan errors eliminated
  • Existing ARSH tests pass
  • Performance impact minimal (uses cmov instructions)

Copy link
Contributor

@ripatel-fd ripatel-fd left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. This is not an issue for the immediate shifts because those are checked by the bytecode verifier. For the register shifts, this is a known technical UB issue but doesn't cause any issues on any known GCC or Clang versions on x86.

We'll investigate this a bit further before accepting this fix because performance numbers of the VM interpreter are very sensitive to changes.

@Ayushjhax
Copy link
Author

Thanks for the clarification that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OOB shifts in fd_vm_interp_core

2 participants