Insert checks for enum discriminants when debug assertions are enabled #141759

1c3t3a · 2025-05-30T09:44:26Z

Similar to the existing null-pointer and alignment checks, this checks for valid enum discriminants on creation of enums through unsafe transmutes. Essentially this sanitizes patterns like the following:

let val: MyEnum = unsafe { std::mem::transmute<u32, MyEnum>(42) };

An extension of this check will be done in a follow-up that explicitly sanitizes for extern enum values that come into Rust from e.g. C/C++.

This check is similar to Miri's capabilities of checking for valid construction of enum values.

This PR is inspired by saethlin@'s PR
#104862. Thank you so much for keeping this code up and the detailed comments!

I also pair-programmed large parts of this together with vabr-g@.

r? @saethlin

rustbot · 2025-05-30T09:44:32Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

src/tools/rust-analyzer/crates/intern/src/symbol/symbols.rs

Similar to the existing nullpointer and alignment checks, this checks for valid enum discriminants on creation of enums through unsafe transmutes. Essentially this sanitizes patterns like the following: ```rust let val: MyEnum = unsafe { std::mem::transmute<u32, MyEnum>(42) }; ``` An extension of this check will be done in a follow-up that explicitly sanitizes for extern enum values that come into Rust from e.g. C/C++. This check is similar to Miri's capabilities of checking for valid construction of enum values. This PR is inspired by saethlin@'s PR rust-lang#104862. Thank you so much for keeping this code up and the detailed comments! I also pair-programmed large parts of this together with vabr-g@.

Kobzol · 2025-06-10T19:33:47Z

@bors2 try @rust-timer queue

rust-bors · 2025-06-10T19:33:50Z

⌛ Trying commit 5587fd7 with merge 7488b2b…

To cancel the try build, run the command @bors2 try cancel.

Insert checks for enum discriminants when debug assertions are enabled Similar to the existing null-pointer and alignment checks, this checks for valid enum discriminants on creation of enums through unsafe transmutes. Essentially this sanitizes patterns like the following: ```rust let val: MyEnum = unsafe { std::mem::transmute<u32, MyEnum>(42) }; ``` An extension of this check will be done in a follow-up that explicitly sanitizes for extern enum values that come into Rust from e.g. C/C++. This check is similar to Miri's capabilities of checking for valid construction of enum values. This PR is inspired by saethlin@'s PR #104862. Thank you so much for keeping this code up and the detailed comments! I also pair-programmed large parts of this together with vabr-g@. r? `@saethlin`

1c3t3a · 2025-06-10T19:54:07Z

This patch is finally ready for review! Let's see what the perf-impact of this is, but I wouldn't assume it is much, as this only emits checks for transmutes to enums.

rust-bors · 2025-06-10T22:01:57Z

☀️ Try build successful (CI)
Build commit: 7488b2b (7488b2b1de7ccc9272a1b2f6015d82794d20c065)

rust-timer · 2025-06-10T23:53:37Z

Finished benchmarking commit (7488b2b): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 3.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.9%	[1.2%, 8.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 754.321s -> 757.215s (0.38%)
Artifact size: 372.15 MiB -> 372.16 MiB (0.00%)

scottmcm · 2025-06-12T06:09:06Z

compiler/rustc_mir_transform/src/check_enums.rs

+    /// In some cases the enum discriminant is stored in a tag that is represented by
+    /// primitive. This method returns the actual discriminant type and size for that
+    /// tag.
+    fn tag_type_and_size_for_primitive(&self, primitive: Primitive) -> (Ty<'tcx>, Size) {


nit: this feels like something that we've got to have somewhere already.

In a quick search I only found the codegen-side versions, though, like

rust/compiler/rustc_codegen_ssa/src/traits/type_.rs

Lines 55 to 64 in 1434630

fn type_from_integer(&self, i: Integer) -> Self::Type {

use Integer::*;

match i {

I8 => self.type_i8(),

I16 => self.type_i16(),

I32 => self.type_i32(),

I64 => self.type_i64(),

I128 => self.type_i128(),

}

}

.

Yeah I felt the same when I was writing this code but also only found this in codegen. I can look again, but would it make any sense to have this as a member of Primitive?

Aha, here it is for Primitive -> Ty: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/layout/trait.PrimitiveExt.html

And for getting a Size, there's already https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.Primitive.html#method.size

compiler/rustc_mir_transform/src/check_enums.rs

scottmcm · 2025-06-12T06:43:29Z

compiler/rustc_mir_transform/src/check_enums.rs

+    source_op: Operand<'tcx>,
+    discr_ty: Ty<'tcx>,
+    discr_size: Size,
+    op_size: Size,


suggestion: maybe you can pass the Ty or Primitive or Integer or something instead of the Size? The Primitive at least is definitely available from the layout info.

(That might help avoid some matches.)

So I introduced a TyAndSize type that brings down a lot of duplication. Other than that I didn't really see a big benefit passing down the Primitive, because in order to get anything useful out of it I'd need more matches... Are you fine with how the code looks now? :)

scottmcm · 2025-06-12T06:49:15Z

compiler/rustc_mir_transform/src/check_enums.rs

+        });
+    }
+
+    // Branch based on the computed equality.


suggestion: If you want to branch based on a set of values, how about inserting a https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/enum.TerminatorKind.html#variant.SwitchInt instead? That can branch to the "check failed" block from the otherwise, and continue successfully from all the valid values.

Ohh good point! I would do this if we decide to stick with this comparison.

scottmcm · 2025-06-12T06:51:25Z

compiler/rustc_mir_transform/src/check_enums.rs

+        ))),
+    });
+
+    // Loop over the list of the discriminants and insert checks for equality.


Hmm, this can be a very large amount of additional MIR. I worry about things like the transmute from usize to ptr::Alignment, for example -- adding another, what, at least 128 MIR statements every time that happens?

Yeah I was already thinking about this... my perception was that such enums are not super prevalent, but ptr::Alignment does look relevant.

One idea I've had was that most enums have contiguous range (ptr::Alignment is a bad example :/) and we could transform this list to a list of WrappingRanges and then compare them. As I said that wouldn't work for ptr::Alignment though...

We could also think about excluding everything with more than e.g. 10 variants and have an option to opt-in for all checks?

every time that happens?

I'm not sure it happens very often. Such transmutes are usually done in a helper function, so they should not be inserted very many times in a debug build.

And the perf results suggest that it does not happen very often. At least currently.

compiler/rustc_mir_transform/src/check_enums.rs

saethlin · 2025-06-13T21:36:54Z

compiler/rustc_mir_transform/src/check_enums.rs

+use rustc_session::Session;
+use tracing::debug;
+
+/// This pass inserts checks at places where enums are constructed and checks


Which places? (Also you should probably say "locations" here, it's more similar to the types)

If you don't want to be too specific about where checks are inserted maybe talk around why the pass currently inserts the checks it does. "We insert checks where they are most likely to find UB, because checking everywhere like Miri would generate too much MIR". Or something like that.

saethlin · 2025-06-13T21:39:54Z

compiler/rustc_mir_transform/src/check_enums.rs

+                // An empty enum that tries to be constructed from an inhabited value, this
+                // is never correct.
+                Variants::Empty => {
+                    // The enum layout is uninhabited but we construct it from sth inhabited.


Isn't this case already detected statically?

saethlin · 2025-06-13T22:00:12Z

tests/mir-opt/enum_check_niche.rs

+
+fn main() {
+    // CHECK-LABEL: fn main(
+    // CHECK assert(copy .*, "trying to construct an enum from an invalid value {}", .*)


Why are there mir-opt tests in here? I can't figure out what they are testing for that the UI tests are not.

Also, I think this CHECK is just checking for any enum check after the start of main. Writing the FileCheck annotations well seems like a lot of work here. So they should be testing for something that's worth it.

I suggested them because I wanted to read the MIR in the .diff instead of puzzling it from the code.

Turning off file-check for them would be completely fine, though, if the CHECKs aren't that useful.

If the tests stay in, they need a clear comment in the test that explains what the expected diff is, so that subsequent contributors and reviewers are able to discern if the test "passed" or not when the diff changes.

scottmcm · 2025-06-13T22:38:39Z

tests/mir-opt/enum_check_no_niche.main.CheckEnums.diff

+         _7 = Eq(copy _4, const 1_u128);
+         _5 = BitOr(copy _7, copy _5);
+         _8 = copy _4 as usize (IntToInt);
+         assert(copy _5, "trying to construct an enum from an invalid value {}", copy _8) -> [success: bb1, unwind unreachable];


Pondering: why is a usize passed to this assert? What happens if you use repr(i128) on the enum and make a variant with i128::MIN as the discriminant?

rustbot assigned saethlin May 30, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 30, 2025