Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix #121126: index out of bounds exceeds max value #123483

Closed
wants to merge 2 commits into from
Closed

Fix #121126: index out of bounds exceeds max value #123483

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44e66b6
Fix #121126: index out of bounds exceeds max value
c4rrao Apr 4, 2024
57a5b75
Merge branch 'rust-lang:master' into master
c4rrao May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions compiler/rustc_mir_transform/src/known_panics_lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,14 @@ impl<'tcx> Value<'tcx> {
}
(PlaceElem::Index(idx), Value::Aggregate { fields, .. }) => {
let idx = prop.get_const(idx.into())?.immediate()?;
let idx = prop.ecx.read_target_usize(idx).ok()?;
fields.get(FieldIdx::from_u32(idx.try_into().ok()?)).unwrap_or(&Value::Uninit)
let idx: u32 = prop.ecx.read_target_usize(idx).ok()?.try_into().ok()?;

let max: u32 = FieldIdx::MAX.index().try_into().ok()?;
if idx > max {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please implement this as a method on all index types instead. Like try_from_u32 and then implement from_u32 by using that

Copy link
Author

Choose a reason for hiding this comment

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

Can we have from_u32 returning Option instead of Self? I can't see a way to inform the caller that the operation was unsuccessful without using Option, Result or panicking. And I guess Result is not the way and panicking was what we had before with the assert()

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... we could just always return an option, good point. Yea lets do that instead of my idea of having both a try_from and a from method

Copy link
Author

Choose a reason for hiding this comment

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

I have a doubt here. In this match inside project method from Value type, why do we always cast the usize idx to a u32 just to call then from_u32 when we have also a from_usize method?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably just an oversight. using from_usize is better

return None;
}

fields.get(FieldIdx::from_u32(idx)).unwrap_or(&Value::Uninit)
}
(
PlaceElem::ConstantIndex { offset, min_length: _, from_end: false },
Expand Down
13 changes: 13 additions & 0 deletions tests/ui/indexing/index-out-of-bounds-exceeds-max-value-issue-121126.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Regression test for #121126. Compiler was panicking when indexing an array
// with an index that is out of bounds and its value is greater than the max
// value allowed for an index.

//@ build-fail

fn main() {
[0][0xFFFF_FF01];
//~^ ERROR this operation will panic at runtime [unconditional_panic]
}

// NOTE: In order for the test to be valid, the index can take on any value
// between FieldIdx::MAX + 1 (= 0xFFF_FF01) and u32::MAX (= 0xFFF_FFFF)
10 changes: 10 additions & 0 deletions tests/ui/indexing/index-out-of-bounds-exceeds-max-value-issue-121126.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: this operation will panic at runtime
--> $DIR/issue-121126-index-out-of-bounds-exceeds-max-value.rs:8:5
|
LL | [0][0xFFFF_FF01];
| ^^^^^^^^^^^^^^^^ index out of bounds: the length is 1 but the index is 4294967041
|
= note: `#[deny(unconditional_panic)]` on by default

error: aborting due to 1 previous error

Loading