Fix unintentional UB in ui tests

[saethlin]

@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]

r? @michaelwoerister

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

[michaelwoerister]

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

[RalfJung]

Also there is no unsafe code here I think.

[saethlin]

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)

[CraftSpider]

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.

[RalfJung]

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

[saethlin]

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

[RalfJung]

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?

[saethlin]

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

[RalfJung]

Oh good point!

[saethlin]

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

[RalfJung]

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

[saethlin]

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.

[RalfJung]

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.

[saethlin]

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

[asquared31415]

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

[saethlin]

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]

Thanks, @saethlin!

@bors r+

[bors]

📌 Commit 5925400 has been approved by michaelwoerister

It is now in the queue for this repository.

[Dylan-DPC]

failed in rollup

@bors r-

[RalfJung]

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

[saethlin]

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

[saethlin]

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

[RalfJung]

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?

[RalfJung]

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

[michaelwoerister]

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

[RalfJung]

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

@bors r+

[bors]

📌 Commit de01ea2 has been approved by michaelwoerister

It is now in the queue for this repository.