Fix unintentional UB in ui tests #107972

saethlin · 2023-02-13T00:04:22Z

@matthiaskrgr found UB in a bunch of the ui tests. This PR fixes a batch of miscellaneous tests I didn't think needed reviewers from a particular part of the project.

rustbot · 2023-02-13T00:04:28Z

r? @michaelwoerister

(rustbot has picked a reviewer for you, use r? to override)

michaelwoerister · 2023-02-13T10:07:03Z

tests/ui/self/arbitrary_self_types_raw_pointer_trait.rs

-    let null_u32 = ptr::null::<u32>() as *const dyn Foo;
-
-    assert_eq!("I'm an i32!", null_i32.foo());
-    assert_eq!("I'm a u32!", null_u32.foo());


Is that really UB? There's only raw pointers involved and they are not dereferenced?

Also there is no unsafe code here I think.

error: Undefined Behavior: dereferencing pointer failed: null pointer is a dangling pointer (it has no provenance) --> tests/ui/self/arbitrary_self_types_raw_pointer_trait.rs:40:31 | 40 | assert_eq!("I'm an i32!", null_i32.foo()); | ^^^^^^^^^^^^^^ dereferencing pointer failed: null pointer is a dangling pointer (it has no provenance)

As an outside observer, I feel like this indicates a bug in Miri - there shouldn't be a dereference to call a raw-pointer receiver, I'd think.

I am not sure where the deref is coming from, definitely needs investigation. UB from safe code is no good.^^

Tracking this at rust-lang/miri#2786

I dropped the changes to this file. If we decide this still needs to be changed (unlikely) I can always do another PR.

RalfJung · 2023-02-13T11:04:56Z

tests/ui/closures/2229_closure_analysis/migrations/auto_traits.fixed

@@ -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();


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?

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

Oh good point!

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

RalfJung · 2023-02-13T11:06:06Z

tests/ui/unsized/unsized3-rpass.rs

@@ -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));


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

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.

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.

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...

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

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.

michaelwoerister · 2023-02-13T16:09:02Z

Thanks, @saethlin!

@bors r+

bors · 2023-02-13T16:09:14Z

📌 Commit 5925400 has been approved by michaelwoerister

It is now in the queue for this repository.

…rister Fix unintentional UB in ui tests `@matthiaskrgr` found UB in a bunch of the ui tests. This PR fixes a batch of miscellaneous tests I didn't think needed reviewers from a particular part of the project.

…rister Fix unintentional UB in ui tests ``@matthiaskrgr`` found UB in a bunch of the ui tests. This PR fixes a batch of miscellaneous tests I didn't think needed reviewers from a particular part of the project.

Dylan-DPC · 2023-02-14T18:02:19Z

failed in rollup

@bors r-

RalfJung · 2023-02-14T18:09:59Z

tests/ui/consts/const-eval/issue-91827-extern-types.rs

-        unsafe { std::slice::from_raw_parts(self.data.as_ptr(), self.len) }
+        unsafe {
+            let ptr = (self as *const List<T>).cast::<usize>().add(1).cast::<T>();
+            std::slice::from_raw_parts(ptr, self.len)


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...

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) } | ^^^^^^^^^^^^^^^^^^

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

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?

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

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;?

& 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.

michaelwoerister · 2023-02-15T15:07:29Z

@bors r+

bors · 2023-02-15T15:07:48Z

📌 Commit de01ea2 has been approved by michaelwoerister

It is now in the queue for this repository.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#107034 (Migrating rustc_infer to session diagnostics (part 4)) - rust-lang#107972 (Fix unintentional UB in ui tests) - rust-lang#108010 (Make `InferCtxt::can_eq` and `InferCtxt::can_sub` return booleans) - rust-lang#108021 (make x look for x.py if shell script does not exist) - rust-lang#108047 (Use `target` instead of `machine` for mir interpreter integer handling.) - rust-lang#108049 (Don't suggest `#[doc(hidden)]` trait methods with matching return type) - rust-lang#108066 (Better names for illegal impl trait positions) - rust-lang#108076 (rustdoc: Use more let chain) - rust-lang#108088 (clarify correctness of `black_box`) - rust-lang#108094 (Demonstrate I/O in File examples) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned michaelwoerister Feb 13, 2023

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. labels Feb 13, 2023

michaelwoerister reviewed Feb 13, 2023

View reviewed changes

RalfJung reviewed Feb 13, 2023

View reviewed changes

saethlin force-pushed the fix-test-ub branch from d44149f to 5925400 Compare February 13, 2023 15:28

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2023

Dylan-DPC mentioned this pull request Feb 14, 2023

Rollup of 8 pull requests #108027

Closed

matthiaskrgr mentioned this pull request Feb 14, 2023

Rollup of 8 pull requests #108028

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 14, 2023

RalfJung reviewed Feb 14, 2023

View reviewed changes

Fix unintentional UB in ui tests

de01ea2

saethlin force-pushed the fix-test-ub branch from 4e04d6d to de01ea2 Compare February 15, 2023 14:05

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 15, 2023

matthiaskrgr mentioned this pull request Feb 15, 2023

Rollup of 10 pull requests #108096

Merged

bors merged commit d40c13a into rust-lang:master Feb 16, 2023

rustbot added this to the 1.69.0 milestone Feb 16, 2023

saethlin deleted the fix-test-ub branch March 15, 2023 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unintentional UB in ui tests #107972

Fix unintentional UB in ui tests #107972

saethlin commented Feb 13, 2023

rustbot commented Feb 13, 2023

michaelwoerister Feb 13, 2023

RalfJung Feb 13, 2023

saethlin Feb 13, 2023

CraftSpider Feb 13, 2023

RalfJung Feb 13, 2023

saethlin Feb 13, 2023

RalfJung Feb 13, 2023

saethlin Feb 13, 2023

RalfJung Feb 13, 2023

saethlin Feb 13, 2023

RalfJung Feb 13, 2023

saethlin Feb 13, 2023

RalfJung Feb 13, 2023

saethlin Feb 13, 2023 •

edited

Loading

asquared31415 Feb 13, 2023 •

edited

Loading

saethlin Feb 13, 2023 •

edited

Loading

michaelwoerister commented Feb 13, 2023

bors commented Feb 13, 2023

Dylan-DPC commented Feb 14, 2023

RalfJung Feb 14, 2023

saethlin Feb 15, 2023 •

edited

Loading

saethlin Feb 15, 2023

RalfJung Feb 15, 2023 •

edited

Loading

RalfJung Feb 15, 2023

michaelwoerister Feb 15, 2023

RalfJung Feb 15, 2023 •

edited

Loading

michaelwoerister commented Feb 15, 2023

bors commented Feb 15, 2023

Fix unintentional UB in ui tests #107972

Fix unintentional UB in ui tests #107972

Conversation

saethlin commented Feb 13, 2023

rustbot commented Feb 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

asquared31415 Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

saethlin Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

michaelwoerister commented Feb 13, 2023

bors commented Feb 13, 2023

Dylan-DPC commented Feb 14, 2023

Choose a reason for hiding this comment

saethlin Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

michaelwoerister commented Feb 15, 2023

bors commented Feb 15, 2023

saethlin Feb 13, 2023 •

edited

Loading

asquared31415 Feb 13, 2023 •

edited

Loading

saethlin Feb 13, 2023 •

edited

Loading

saethlin Feb 15, 2023 •

edited

Loading

RalfJung Feb 15, 2023 •

edited

Loading

RalfJung Feb 15, 2023 •

edited

Loading