-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ICE when calling mem::uninitialized for an enum with 256 variants. #1456
Comments
Interesting, does it really take a 256-variant enum to trigger this? Good catch! |
jup, it probably causes a failure earlier in case the enum discriminant is not valid for all its possible values. |
with 255 variants:
|
Ah yes, our scalar bounds verification code actually special-cases "all values valid" to not complain about code until we have a decision in rust-lang/unsafe-code-guidelines#71, so with this big enum you end up causing an uninit read later when determining the discriminant. it is probably possible to cause this in CTFE from rustc (and it needs a fix in rustc). |
That is strange, why does this not error? |
Possible it ends up in this branch but I did not think we would ever get there for enums... |
Ah, I think we are traversing the enum's fields before we read the discriminant, and the field actually has appropriate integer type and as such CTFE doesn't allow it to remain uninitialized. So this is indeed Miri-only. |
it does error in validation, did you expect an earlier error? |
I expected the same ICE as in the OP. But I figured out why it does not happen. |
So... did you mean Isn't the difference between the reported issue and the CTFE repro just that ctfe doesn't validate locals at all? |
CTFE validation of the final value shows a proper error instead of an ICE. I wanted to understand why. |
Ah, that part I can help with: https://github.com/rust-lang/rust/blob/master/src/librustc_mir/interpret/validity.rs#L497-L504 we explicitly ignore uninitialized integers, so when you read the tag and validate it (which is of integer type), you get a validation error, but miri doesn't, so it continues and does the actual variant validation. To do that, it needs to know the discriminant, at which point the discriminant computation from the tag fails because the tag is uninitialized. which... is basically what you wrote above, I just didn't grok it :D |
I'm getting this error in a log
Hope it helps |
@niluxv If there is no such big enum it is likely a different ICE... are you able to reduce this to a self-contained example? If not, I hope next week-end I can get back to this problem; once it is fixed you can see if that also fixes your problem. |
The errors and backtraces seem to be the same... But I would be surprised if enums with 256 variants are used. I tried to pinpoint it a bit and it is indeed caused by |
Smallest example I could getuse linked_hash_map::LinkedHashMap;
use serde::Derserialize;
use serde_test::Token;
use serde_test::de::Deserializer; // Note this is privat
#[test]
fn test_ser_de_simple() {
assert_de_tokens::<'_,LinkedHashMap<char, usize>>(&[
Token::Map { len: Some(1) },
Token::Char('b'),
Token::I32(20),
Token::MapEnd,
]);
}
fn assert_de_tokens<'de, T>(tokens: &'de [Token])
where
T: Deserialize<'de>,
{
let mut de = de::Deserializer::new(tokens);
let mut deserialized_val: T = T::deserialize(&mut de).unwrap();
let mut de2 = de::Deserializer::new(tokens);
let deserialized_val2: T = T::deserialize(&mut de2).unwrap();
let reference = &mut deserialized_val;
*reference = deserialized_val2; // ICE happens here
println!("never reached");
} |
Even simpleruse linked_hash_map::LinkedHashMap;
#[test]
fn test_ser_de_simpler() {
let mut val = LinkedHashMap::new();
val.insert('a', 0);
let mut val2 = LinkedHashMap::new();
val2.insert('a', 0);
let reference = &mut val;
*reference = val2; // ICE is triggered here
println!("never reached");
} The drop implementation of |
Btw, do you know if this version of linked-hash-map is with or without contain-rs/linked-hash-map#100? |
@niluxv I think your bug is different, and probably should be tracked separately. However, uninitialized |
I used the master branch so it includes this patch. |
rust-lang/rust#74059 should fix this. |
…oli-obk Miri value validation: fix handling of uninit memory Fixes rust-lang/miri#1456 Fixes rust-lang/miri#1467 r? @oli-obk
Running MIRI on the following code causes an ICE:
code:
The text was updated successfully, but these errors were encountered: