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.

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

Fix vec![x; n] with null raw fat pointer zeroing the pointer metadata #64891

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Sep 29, 2019

#49496 introduced specialization based on:

unsafe impl<T: ?Sized> IsZero for *mut T {
    fn is_zero(&self) -> bool {
        (*self).is_null()
    }
}

… to call RawVec::with_capacity_zeroed for creating Vec<*mut T>, which is incorrect for fat pointers since <*mut T>::is_null only looks at the data component. That is, a fat pointer can be “null” without being made entirely of zero bits.

This commit fixes it by removing the ?Sized bound on this impl (and the corresponding *const T one). This regresses vec![x; n] with x a null raw slice of length zero, but that seems exceptionally uncommon. (Vtable pointers are never null, so raw trait objects would not take the fast path anyway.)

An alternative to keep the ?Sized bound (or even generalize to impl<U: Copy> IsZero for U) would be to cast to &[u8] of length size_of::<U>(), but the optimizer seems not to be able to propagate alignment information and sticks with comparing one byte at a time:

https://rust.godbolt.org/z/xQFkwL


Without the library change, the new test fails as follows:

---- vec::vec_macro_repeating_null_raw_fat_pointer stdout ----
[src/liballoc/tests/vec.rs:1301] ptr_metadata(raw_dyn) = 0x00005596ef95f9a8
[src/liballoc/tests/vec.rs:1306] ptr_metadata(vec[0]) = 0x0000000000000000
thread 'vec::vec_macro_repeating_null_raw_fat_pointer' panicked at 'assertion failed: vec[0] == null_raw_dyn', src/liballoc/tests/vec.rs:1307:5

rust-lang#49496 introduced specialization based on:

```
unsafe impl<T: ?Sized> IsZero for *mut T {
    fn is_zero(&self) -> bool {
        (*self).is_null()
    }
}
```

… to call `RawVec::with_capacity_zeroed` for creating `Vec<*mut T>`,
which is incorrect for fat pointers
since `<*mut T>::is_null` only looks at the data component.
That is, a fat pointer can be “null” without being made entirely of zero bits.

This commit fixes it by removing the `?Sized` bound on this impl
(and the corresponding `*const T` one).
This regresses `vec![x; n]` with `x` a null raw slice of length zero,
but that seems exceptionally uncommon.
(Vtable pointers are never null, so raw trait objects would not take
the fast path anyway.

An alternative to keep the `?Sized` bound
(or even generalize to `impl<U: Copy> IsZero for U`)
would be to cast to `&[u8]` of length `size_of::<U>()`,
but the optimizer seems not to be able to propagate alignment information
and sticks with comparing one byte at a time:

https://rust.godbolt.org/z/xQFkwL

----

Without the library change, the new test fails as follows:

```
---- vec::vec_macro_repeating_null_raw_fat_pointer stdout ----
[src/liballoc/tests/vec.rs:1301] ptr_metadata(raw_dyn) = 0x00005596ef95f9a8
[src/liballoc/tests/vec.rs:1306] ptr_metadata(vec[0]) = 0x0000000000000000
thread 'vec::vec_macro_repeating_null_raw_fat_pointer' panicked at 'assertion failed: vec[0] == null_raw_dyn', src/liballoc/tests/vec.rs:1307:5
```
@rust-highfive
Copy link
Collaborator

r? @KodrAus

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2019
@Centril
Copy link
Contributor

Centril commented Sep 29, 2019

r? @RalfJung

@rust-highfive rust-highfive assigned RalfJung and unassigned KodrAus Sep 29, 2019
@RalfJung
Copy link
Member

(I am traveling, so I won't be able to review for at least another week.)

@SimonSapin
Copy link
Contributor Author

r? @sfackler

@sfackler
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2019

📌 Commit ce60da4 has been approved by sfackler

@bors 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 Sep 29, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Sep 30, 2019
…ackler

Fix `vec![x; n]` with null raw fat pointer zeroing the pointer metadata

rust-lang#49496 introduced specialization based on:

```rust
unsafe impl<T: ?Sized> IsZero for *mut T {
    fn is_zero(&self) -> bool {
        (*self).is_null()
    }
}
```

… to call `RawVec::with_capacity_zeroed` for creating `Vec<*mut T>`, which is incorrect for fat pointers since `<*mut T>::is_null` only looks at the data component. That is, a fat pointer can be “null” without being made entirely of zero bits.

This commit fixes it by removing the `?Sized` bound on this impl (and the corresponding `*const T` one). This regresses `vec![x; n]` with `x` a null raw slice of length zero, but that seems exceptionally uncommon. (Vtable pointers are never null, so raw trait objects would not take the fast path anyway.)

An alternative to keep the `?Sized` bound (or even generalize to `impl<U: Copy> IsZero for U`) would be to cast to `&[u8]` of length `size_of::<U>()`, but the optimizer seems not to be able to propagate alignment information and sticks with comparing one byte at a time:

https://rust.godbolt.org/z/xQFkwL

----

Without the library change, the new test fails as follows:

```rust
---- vec::vec_macro_repeating_null_raw_fat_pointer stdout ----
[src/liballoc/tests/vec.rs:1301] ptr_metadata(raw_dyn) = 0x00005596ef95f9a8
[src/liballoc/tests/vec.rs:1306] ptr_metadata(vec[0]) = 0x0000000000000000
thread 'vec::vec_macro_repeating_null_raw_fat_pointer' panicked at 'assertion failed: vec[0] == null_raw_dyn', src/liballoc/tests/vec.rs:1307:5
```
tmandry added a commit to tmandry/rust that referenced this pull request Sep 30, 2019
…ackler

Fix `vec![x; n]` with null raw fat pointer zeroing the pointer metadata

rust-lang#49496 introduced specialization based on:

```rust
unsafe impl<T: ?Sized> IsZero for *mut T {
    fn is_zero(&self) -> bool {
        (*self).is_null()
    }
}
```

… to call `RawVec::with_capacity_zeroed` for creating `Vec<*mut T>`, which is incorrect for fat pointers since `<*mut T>::is_null` only looks at the data component. That is, a fat pointer can be “null” without being made entirely of zero bits.

This commit fixes it by removing the `?Sized` bound on this impl (and the corresponding `*const T` one). This regresses `vec![x; n]` with `x` a null raw slice of length zero, but that seems exceptionally uncommon. (Vtable pointers are never null, so raw trait objects would not take the fast path anyway.)

An alternative to keep the `?Sized` bound (or even generalize to `impl<U: Copy> IsZero for U`) would be to cast to `&[u8]` of length `size_of::<U>()`, but the optimizer seems not to be able to propagate alignment information and sticks with comparing one byte at a time:

https://rust.godbolt.org/z/xQFkwL

----

Without the library change, the new test fails as follows:

```rust
---- vec::vec_macro_repeating_null_raw_fat_pointer stdout ----
[src/liballoc/tests/vec.rs:1301] ptr_metadata(raw_dyn) = 0x00005596ef95f9a8
[src/liballoc/tests/vec.rs:1306] ptr_metadata(vec[0]) = 0x0000000000000000
thread 'vec::vec_macro_repeating_null_raw_fat_pointer' panicked at 'assertion failed: vec[0] == null_raw_dyn', src/liballoc/tests/vec.rs:1307:5
```
bors added a commit that referenced this pull request Oct 1, 2019
Rollup of 9 pull requests

Successful merges:

 - #64377 (Add long error explanation for E0493)
 - #64786 (Use https for curl when building for linux)
 - #64828 (Graphviz debug output for generic dataflow analysis)
 - #64838 (Add long error explanation for E0550)
 - #64891 (Fix `vec![x; n]` with null raw fat pointer zeroing the pointer metadata)
 - #64893 (Zero-initialize `vec![None; n]` for `Option<&T>`, `Option<&mut T>` and `Option<Box<T>>`)
 - #64911 (Fixed a misleading documentation issue #64844)
 - #64921 (Add test for issue-64662)
 - #64923 (Add missing links for mem::needs_drop)

Failed merges:

 - #64918 (Add long error explanation for E0551)

r? @ghost
@bors bors merged commit ce60da4 into rust-lang:master Oct 1, 2019
@RalfJung
Copy link
Member

RalfJung commented Oct 9, 2019

Wow, good catch! miri-test-libstd will run this test in Miri as well, and it is green.

@SimonSapin SimonSapin deleted the vec-of-fat-raw-ptr branch November 28, 2019 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants