Skip to content

Fix unintentional UB in ui tests #107972

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

Merged
merged 1 commit into from
Feb 16, 2023
Merged

Fix unintentional UB in ui tests #107972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions tests/ui/closures/2229_closure_analysis/migrations/auto_traits.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ fn test_send_trait() {
//~| HELP: add a dummy let to cause `fptr` to be fully captured
*fptr.0 = 20;
//~^ NOTE: in Rust 2018, this closure captures all of `fptr`, but in Rust 2021, it will only capture `fptr.0`
} });
} }).join().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Doesn't look like a UB fix. Is this about fixing memory leaks? Is that even a goal or should Miri be run with -Zmiri-ignore-leaks when running ui tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this join, the thread can execute after the function returns, which is a use-after-free.

Copy link
Member

Choose a reason for hiding this comment

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

Oh good point!

Copy link
Member Author

Choose a reason for hiding this comment

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

No need to give me too much credit here, I just looked at Miri's output :p

}

/* Test Sync Trait Migration */
Expand All @@ -47,7 +47,7 @@ fn test_sync_trait() {
//~| HELP: add a dummy let to cause `fptr` to be fully captured
*fptr.0.0 = 20;
//~^ NOTE: in Rust 2018, this closure captures all of `fptr`, but in Rust 2021, it will only capture `fptr.0.0`
} });
} }).join().unwrap();
}

/* Test Clone Trait Migration */
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/closures/2229_closure_analysis/migrations/auto_traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ fn test_send_trait() {
//~| HELP: add a dummy let to cause `fptr` to be fully captured
*fptr.0 = 20;
//~^ NOTE: in Rust 2018, this closure captures all of `fptr`, but in Rust 2021, it will only capture `fptr.0`
});
}).join().unwrap();
}

/* Test Sync Trait Migration */
Expand All @@ -47,7 +47,7 @@ fn test_sync_trait() {
//~| HELP: add a dummy let to cause `fptr` to be fully captured
*fptr.0.0 = 20;
//~^ NOTE: in Rust 2018, this closure captures all of `fptr`, but in Rust 2021, it will only capture `fptr.0.0`
});
}).join().unwrap();
}

/* Test Clone Trait Migration */
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/closures/2229_closure_analysis/migrations/auto_traits.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ LL ~ thread::spawn(move || { let _ = &fptr; unsafe {
LL |
...
LL |
LL ~ } });
LL ~ } }).join().unwrap();
|

error: changes to closure capture in Rust 2021 will affect which traits the closure implements
Expand All @@ -41,7 +41,7 @@ LL ~ thread::spawn(move || { let _ = &fptr; unsafe {
LL |
...
LL |
LL ~ } });
LL ~ } }).join().unwrap();
|

error: changes to closure capture in Rust 2021 will affect drop order and which traits the closure implements
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/closures/2229_closure_analysis/migrations/multi_diagnostics.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ fn test_multi_traits_issues() {
//~^ NOTE: in Rust 2018, this closure captures all of `fptr1`, but in Rust 2021, it will only capture `fptr1.0.0`
*fptr2.0 = 20;
//~^ NOTE: in Rust 2018, this closure captures all of `fptr2`, but in Rust 2021, it will only capture `fptr2.0`
} });
} }).join().unwrap();
}

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/closures/2229_closure_analysis/migrations/multi_diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ fn test_multi_traits_issues() {
//~^ NOTE: in Rust 2018, this closure captures all of `fptr1`, but in Rust 2021, it will only capture `fptr1.0.0`
*fptr2.0 = 20;
//~^ NOTE: in Rust 2018, this closure captures all of `fptr2`, but in Rust 2021, it will only capture `fptr2.0`
});
}).join().unwrap();
}

fn main() {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/closures/2229_closure_analysis/migrations/multi_diagnostics.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ LL ~ thread::spawn(move || { let _ = (&fptr1, &fptr2); unsafe {
LL |
...
LL |
LL ~ } });
LL ~ } }).join().unwrap();
|

error: aborting due to 5 previous errors
Expand Down
5 changes: 4 additions & 1 deletion tests/ui/consts/const-eval/issue-91827-extern-types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ pub struct ListImpl<T, const N: usize> {

impl<T> List<T> {
const fn as_slice(&self) -> &[T] {
unsafe { std::slice::from_raw_parts(self.data.as_ptr(), self.len) }
unsafe {
let ptr = addr_of!(self.tail) as *const T;
std::slice::from_raw_parts(ptr, self.len)
Copy link
Member

Choose a reason for hiding this comment

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

What is this even doing? It looks like the original code would run into the &Header issue, but that issue doesn't have a work-around in Stacked Borrows...

Copy link
Member Author

@saethlin saethlin Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

CI is failing because of padding that I failed to account for in my poorly-written, manual, offset_of!.

I don't entirely follow your reasoning, but I think the code is UB before we even get to where you are seeing the &Header pattern. The slice can't be constructed because the array is length 0:

error: Undefined Behavior: trying to retag from <3086> for SharedReadOnly permission at alloc1[0x8], but that tag does not exist in the borrow stack for this location
   --> /home/ben/rust/library/core/src/slice/raw.rs:100:9
    |
100 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to retag from <3086> for SharedReadOnly permission at alloc1[0x8], but that tag does not exist in the borrow stack for this location
    |         this error occurs as part of retag at alloc1[0x8..0x38]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3086> would have been created here, but this is a zero-size retag ([0x8..0x8]) so the tag in question does not exist anywhere
   --> tests/ui/consts/const-eval/issue-91827-extern-types.rs:31:45
    |
31  |         unsafe { std::slice::from_raw_parts(self.data.as_ptr(), self.len) }
    |                                             ^^^^^^^^^^^^^^^^^^

Copy link
Member Author

Choose a reason for hiding this comment

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

I added an even more horrifying implementation that passes

x run miri --args tests/ui/consts/const-eval/issue-91827-extern-types.rs --target=wasm32-unknown-unknown

Copy link
Member

@RalfJung RalfJung Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The empty array is the header, isn't it?

pub struct List<T> {
    len: usize,
    data: [T; 0],
    tail: Opaque,
}

is basically a variable-sized array, serving as the header for ListImpl.

Also doesn't tail_offset further down already compute the offset you need?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I entirely forgot that I made &Header work with extern types, so what is what is happening here.

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand why this is done in the way it is. Is there any value for the test to not just express this as let ptr = (&self.data) as *const T;?

Copy link
Member

@RalfJung RalfJung Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

& restricts provenance to that field, which has size 0, so the resulting reference is only valid for 0 bytes, which is not very useful.

But addr_of!(self.data) should work.

}
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/unsized/unsized3-rpass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ pub fn main() {
}

let data: Box<Foo_<i32>> = Box::new(Foo_ { f: [1, 2, 3] });
let x: &Foo<i32> = mem::transmute(slice::from_raw_parts(&*data, 3));
let x: &Foo<i32> = mem::transmute(ptr::slice_from_raw_parts(&*data, 3));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand why this helps, given that both are transmuted to the same target type?

Copy link
Member Author

Choose a reason for hiding this comment

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

The &Foo<i32> is fine. The problem is the slice creation itself, because that call tries to build a slice of 3 elements from a pointer to a Box of 1 element.

Copy link
Member

Choose a reason for hiding this comment

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

Oh so the temporary slice actually pretends to have 3 Foo_<i32>. Wtf, why? At that point one might just transmute a (ptr, usize) directly without going through from_raw_parts...

Anyway, patch looks good.

Copy link
Member Author

@saethlin saethlin Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

transmute a (ptr, usize)

That would require relying on the still-unstable slice layout 😉

But yeah I have no idea why anyone would want to write the code in this test. Considering the amount of code in the SIMD tests that were using a pointer to the first element as a pointer to the whole array it might not hurt for people to skim over the tests for some feature or another from time to time...

Copy link
Contributor

@asquared31415 asquared31415 Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's possible to make a reference to a DST without relying on transmute or assuming that any two slice-like fat pointers have the same layout by using the fact that as preserves metadata. playground example

Copy link
Member Author

@saethlin saethlin Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah that's the sort of thing I would advise someone write, but this is a test... so I am not really sure what it is supposed to test. So I'm preserving as much of it as I can in the hope that works.

assert_eq!(x.f.len(), 3);
assert_eq!(x.f[0], 1);

Expand All @@ -70,7 +70,7 @@ pub fn main() {

let data: Box<_> =
Box::new(Baz_ { f1: 42, f2: ['a' as u8, 'b' as u8, 'c' as u8, 'd' as u8, 'e' as u8] });
let x: &Baz = mem::transmute(slice::from_raw_parts(&*data, 5));
let x: &Baz = mem::transmute(ptr::slice_from_raw_parts(&*data, 5));
assert_eq!(x.f1, 42);
let chs: Vec<char> = x.f2.chars().collect();
assert_eq!(chs.len(), 5);
Expand Down