Skip to content

Conversation

davidwrighton
Copy link
Member

  • Fail with BADCODE instead of asserting, which allows us to complete running some of the test suites, and will actually do the desired thing in actual retail builds.
  • Also protect against unsafe memory access by bounds checking in CEEOpName

- Fail with BADCODE instead of asserting, which allows us to complete running some of the test suites, and will actually do the desired thing in actual retail builds.
- Also protect against unsafe memory access by bounds checking in CEEOpName
@davidwrighton davidwrighton requested a review from BrzVlad as a code owner October 3, 2025 21:39
@Copilot Copilot AI review requested due to automatic review settings October 3, 2025 21:39
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves error handling in the CLR interpreter by replacing assertion failures with proper BADCODE exceptions when encountering unknown opcodes. This allows test suites to complete execution rather than crashing on unsupported operations.

  • Replace assert() calls with BADCODE() macro for unknown opcodes and unsupported static field accessors
  • Add bounds checking in CEEOpName() to prevent unsafe memory access when looking up opcode names
  • Wrap debug output in #ifdef DEBUG to reduce overhead in release builds

Reviewed Changes

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

File Description
src/coreclr/interpreter/intops.cpp Added bounds checking to prevent array access beyond valid opcode range
src/coreclr/interpreter/compiler.cpp Replaced assertions with BADCODE exceptions and added debug-only output

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 3, 2025
@kg
Copy link
Member

kg commented Oct 3, 2025

Will using BADCODE break fallback to JIT?

@davidwrighton
Copy link
Member Author

Maybe, but we don't really want that fallback if it isn't fixing things, and the current path is just an assert which is just wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants