Use MaybeUninit for storage of inline items #162

thomcc · 2019-09-08T09:26:31Z

Fixes #126

I didn't see a PR open for this, and was poking around so I figured I'd do it. Sorry if I stepped on anyone's toes.

This should remove the use of mem::uninitialized, and ensure that all values used directly or via references are initialized. (e.g. T and &T are always initialized, but *const T and *mut T may not be, which is fine).

With these changes, miri passes. I also changed the CI to run miri, hopefully.

That said, this includes three breaking changes, so it will require a min version bump.

The functions on the Array trait ptr and ptr_mut have been removed. Because these took a &self/&mut self argument, there's no way for us to call them when we only have a MaybeUninit<A>. Now, we just use the memory of the object directly.

This limits the flexibility of custom implementations of Array, as they can no longer return pointers to values other than themselves, but hopefully nobody does that (IMO there's a pretty good case for sealing Array...)
SmallVec::from_buf_and_len_unchecked now takes a MaybeUninit and not an A. This means that callers have the option of only partially initializing said array without risking nasal demons. I can undo this change if desired, as it's
Rust 1.36.0 is required to use MaybeUninit, so this bumps the MSRV from 1.20.0 to 1.36.0. It seems tricky to make the use of MaybeUninit optional here, at least without a bunch or work (ignoring the fact that doing so would require adding a build.rs which parses rustc -Vv to tell us the version).

This change is

lib.rs

Shnatsel · 2019-09-08T09:48:45Z

If you're bumping MSRV, you might as well get rid of hacks such as #158

thomcc · 2019-09-08T09:50:23Z

I saw that but didn't want to change too many unrelated things in this PR. I'm willing to make that change too if desired though.

64 · 2019-09-08T23:00:12Z

FWIW this PR fixes instances of UB in firefox, among other things.

e.g (not a comprehensive list):

emilio

So this makes SmallVec<[T; size]> use MaybeUninit<[T; size]>, shouldn't it conceptually use [MaybeUninit<T>; size]?

Anyhow this looks fine, with the Cargo.lock updated to account for the new minimum rust version change.

emilio · 2019-09-20T06:20:20Z

lib.rs

+        // this check should be optimized away entirely for valid ones.
+        assert!(
+            (mem::size_of::<A>() == A::size() * mem::size_of::<A::Item>()) &&
+            (mem::align_of::<A>() >= mem::align_of::<A::Item>())


nit: Parens here are redundant.

This is designed to merged after servo#162, as it deprecates a function that should be no longer necessary on newer Rust versions.

bors-servo · 2019-10-15T12:20:30Z

☔ The latest upstream changes (presumably #167) made this pull request unmergeable. Please resolve the merge conflicts.

jonhoo · 2019-10-16T13:38:48Z

This would be a good change to land given the new mem::unitialized lint in 1.38.

jonhoo · 2019-10-18T17:48:05Z

@thomcc Do you have some spare cycles to look at the merge conflict?
@jdm / @emilio Is this something you'd consider merging soon? Currently, running miri on anything that depends on SmallVec gives

error[E0080]: Miri evaluation error: type validation failed: encountered undefined address in pointer at [0]
   --> /home/jon/.cargo/registry/src/github.com-1ecc6299db9ec823/smallvec-0.6.10/lib.rs:410:49
    |
410 |                 data: SmallVecData::from_inline(mem::uninitialized()),
    |                                                 ^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: type validation failed: encountered undefined address in pointer at [0]
    |
    = note: inside call to `smallvec::SmallVec::<[&str; 1]>::new` at /home/jon/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:227:5

thomcc · 2019-10-18T18:19:54Z

@thomcc Do you have some spare cycles to look at the merge conflict?

Ah, right, thanks for the prod. Sorry about the delay, I'll update it tonight.

This includes two breaking changes, in addition to the fact that it will require a MSRV bump: 1. The functions on the `Array` trait `ptr` and `ptr_mut` have been removed. Because these took a `&self`/`&mut self` argument, there's no way for us to call them when we only have a `MaybeUninit<A>`. Now, we just use the memory of the object directly. This limits the flexibility of custom implementations of `Array`, (they can no longer return pointers to values other than themselves) but I imagine this is very rare and was probably broken somehow to begin with. Anybody who does this will get a compile error. 2. `from_buf_and_len_unchecked` now takes a MaybeUninit<A>, so that callers have the option of only partially initializing the array.

thomcc · 2019-10-18T18:26:50Z

So this makes SmallVec<[T; size]> use MaybeUninit<[T; size]>, shouldn't it conceptually use [MaybeUninit; size]?

IIUC there's no difference in practice. And this is less of a change to the API surface, and less work to implement. But yeah... possibly?

Anyhow this looks fine, with the Cargo.lock updated to account for the new minimum rust version change.

Not sure what you mean by this. This repository has Cargo.lock in it's .gitignore.

emilio · 2019-10-19T00:48:42Z

I meant the Cargo.toml version, sorry. But I guess we can just update it later if you want.

Anyhow this looks good.

thomcc · 2019-10-19T01:53:53Z

I've rebased the branch so that it's up to date with the latest master, but I'm... unsure what github is still upset about regarding conflicts. 😅 It even says on master...thomcc:maybe-uninit that it's able to be merged... Maybe it will sort itself out in a bit...

emilio · 2019-10-19T02:14:41Z

@bors-servo r+

bors-servo · 2019-10-19T02:14:41Z

📌 Commit 28c09eb has been approved by emilio

Use MaybeUninit for storage of inline items Fixes #126 I didn't see a PR open for this, and was poking around so I figured I'd do it. Sorry if I stepped on anyone's toes. This should remove the use of mem::uninitialized, and ensure that all values used directly or via references are initialized. (e.g. T and &T are always initialized, but `*const T` and `*mut T` may not be, which is fine). With these changes, `miri` passes. I also changed the CI to run `miri`, hopefully. That said, this includes three breaking changes, so it will require a min version bump. 1. The functions on the `Array` trait `ptr` and `ptr_mut` have been removed. Because these took a `&self`/`&mut self` argument, there's no way for us to call them when we only have a `MaybeUninit<A>`. Now, we just use the memory of the object directly. This limits the flexibility of custom implementations of `Array`, as they can no longer return pointers to values other than themselves, but hopefully nobody does that (IMO there's a pretty good case for sealing `Array`...) 2. `SmallVec::from_buf_and_len_unchecked` now takes a MaybeUninit<A> and not an A. This means that callers have the option of only partially initializing said array without risking nasal demons. I can undo this change if desired, as it's 3. Rust 1.36.0 is required to use MaybeUninit, so this bumps the MSRV from 1.20.0 to 1.36.0. It seems tricky to make the use of MaybeUninit optional here, at least without a bunch or work (ignoring the fact that doing so would require adding a build.rs which parses `rustc -Vv` to tell us the version).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/162)

bors-servo · 2019-10-19T02:14:45Z

⌛ Testing commit 28c09eb with merge 690d65e...

bors-servo · 2019-10-19T02:20:56Z

☀️ Test successful - checks-travis
Approved by: emilio
Pushing 690d65e to master...

This is designed to merged after #162, as it deprecates a function that should be no longer necessary on newer Rust versions.

RalfJung · 2019-10-19T08:29:20Z

.travis.yml

@@ -11,4 +11,5 @@ script: |
  ([ $TRAVIS_RUST_VERSION != nightly ] || cargo check --verbose --no-default-features) &&
  ([ $TRAVIS_RUST_VERSION != nightly ] || cargo test --verbose --features union) &&
  ([ $TRAVIS_RUST_VERSION != nightly ] || cargo test --verbose --all-features) &&
-  ([ $TRAVIS_RUST_VERSION != nightly ] || cargo bench --verbose bench)
+  ([ $TRAVIS_RUST_VERSION != nightly ] || cargo bench --verbose bench) &&
+  ([ $TRAVIS_RUST_VERSION != nightly ] || bash ./scripts/run_miri.sh)


This is awesome -- I had adding Miri here on my TODO list but didn't get around to actually do that yet. :)

jonhoo · 2019-10-26T16:49:31Z

Any chance we could do a minor release with this too so downstream projects can run miri again? :)

mbrubeck · 2019-10-28T22:55:55Z

I would prefer to do this in the next major release, because it increases our minimum Rust version to 1.36 (from 1.20 in the previous release). Although a Rust version increase is not a breaking change at the source level, it still causes problems for downstream crates when it happens in a minor update to a widely-used library.

#73 is tracking the breaking changes going into the next major release. I'll try to get the last items merged in the next few days, and then publish the release.

This is designed to merged after servo#162, as it deprecates a function that should be no longer necessary on newer Rust versions.

* [breaking change] Use `MaybeUninit` internally to avoid possible undefined behavior (servo#162, servo#170). * [breaking change] The `drain` method now takes a range argument, just like the standard `Vec::drain` (servo#145). * [breaking change] Remove the `unreachable` function and replace it with the new standard `unreachable_unchecked` function (servo#164). * [breaking change] Use `no_std` by default. This crate depends only on `core` and `alloc` by default. If the optional `write` feature is enabled then it depends on `std` so that `SmallVec<[u8, _]>` can implement the `std::io::Write` trait (servo#173). * Add support for 96-element small vectors, `SmallVec<[T; 96]>` (servo#163). * Iterators now implement `FusedIterator` (servo#172). * Indexing now uses the standard `SliceIndex` trait (servo#166). * Remove the deprecated `VecLike` trait (servo#165). * Use `NonNull` internally (servo#171). * Add automatic fuzz testing and MIRI testing (servo#168, servo#162). * Update syntax and formatting to Rust 2018 standard (servo#174, servo#167).

Version 1.0.0 * Requires Rust 1.36 or later. * [breaking change] Use `MaybeUninit` to avoid possible undefined behavior (#162, #170). * [breaking change] The `drain` method now takes a range argument, just like the standard `Vec::drain` (#145). * [breaking change] Remove the `unreachable` function and replace it with the new standard `unreachable_unchecked` function (#164). * [breaking change] Use `no_std` by default. This crate depends only on `core` and `alloc` by default. If the optional `write` feature is enabled then it depends on `std` so that `SmallVec<[u8;_]>` can implement the `std::io::Write` trait (#173). * Add support for 96-element small vectors, `SmallVec<[T; 96]>` (#163). * Iterators now implement `FusedIterator` (#172). * Indexing now uses the standard `SliceIndex` trait (#166). * Remove the deprecated `VecLike` trait (#165). * Use `NonNull` internally (#171). * Add automatic fuzz testing and MIRI testing (#168, #162). * Update syntax and formatting to Rust 2018 standard (#174, #167).

RalfJung · 2019-11-04T07:43:50Z

When rust-lang/rust#66059 lands, many users of the 0.6 series will see their programs break because of the incorrect mem::uninitialized. Is there any way a variant of this patch could land on the 0.6 branch so that getting the fix to them doesn't require a major version bump?

RalfJung · 2019-11-04T07:47:52Z

If the concern is the MSRV bump, one option is to use https://crates.io/crates/maybe-uninit: for Rust versions prior to 1.36, that's not actually fixing the UB, but it is certainly no worse than what smallvec currently does. And for newer Rust versions, the UB is fixed.

This is a backport of servo#162 to the smallvec 0.6 branch. To avoid bumping the minimum Rust version, the `maybe-uninit` crate is used in place of `std::mem::MaybeUninit`. To avoid breaking changes, the `Array::ptr` and `ptr_mut` methods are retained but are no longer used, and the API to `from_buf_and_len_unchecked` is unchanged.

mbrubeck · 2019-11-04T18:12:43Z

Submitted #180 to backport the MaybeUninit changes to 0.6 using the maybe-uninit crate.

This is a backport of servo#162 to the smallvec 0.6 branch. To avoid bumping the minimum Rust version, the `maybe-uninit` crate is used in place of `std::mem::MaybeUninit`. To avoid breaking changes, the `Array::ptr` and `ptr_mut` methods are retained but are no longer used, and the API to `from_buf_and_len_unchecked` is unchanged.

@mbrubeck

bump smallvec to 1.0 This includes servo/rust-smallvec#162, fixing an unsoundness in smallvec. See servo/rust-smallvec#175 for the 1.0 release announcement. Cc @mbrubeck @emilio

thomcc force-pushed the maybe-uninit branch from a50739f to 83ff077 Compare September 8, 2019 09:31

thomcc commented Sep 8, 2019

View reviewed changes

lib.rs Outdated Show resolved Hide resolved

thomcc force-pushed the maybe-uninit branch from 83ff077 to 6da517d Compare September 8, 2019 09:39

Shnatsel mentioned this pull request Sep 8, 2019

Audit smallvec rust-secure-code/safety-dance#9

Open

thomcc force-pushed the maybe-uninit branch from 6da517d to db5f0b3 Compare September 8, 2019 19:13

emilio approved these changes Sep 20, 2019

View reviewed changes

KamilaBorowska added a commit to KamilaBorowska/rust-smallvec that referenced this pull request Oct 4, 2019

Deprecated unreachable function

3eb9202

This is designed to merged after servo#162, as it deprecates a function that should be no longer necessary on newer Rust versions.

KamilaBorowska added a commit to KamilaBorowska/rust-smallvec that referenced this pull request Oct 4, 2019

Deprecate unreachable function

38ab156

This is designed to merged after servo#162, as it deprecates a function that should be no longer necessary on newer Rust versions.

KamilaBorowska mentioned this pull request Oct 4, 2019

Deprecate unreachable function #164

Closed

KamilaBorowska added a commit to KamilaBorowska/rust-smallvec that referenced this pull request Oct 4, 2019

Deprecate unreachable function

fdd2043

This is designed to merged after servo#162, as it deprecates a function that should be no longer necessary on newer Rust versions.

KamilaBorowska added a commit to KamilaBorowska/rust-smallvec that referenced this pull request Oct 4, 2019

Deprecate unreachable function

dde0d8c

This is designed to merged after servo#162, as it deprecates a function that should be no longer necessary on newer Rust versions.

KamilaBorowska mentioned this pull request Oct 5, 2019

Use SliceIndex trait for SmallVec's Index trait #166

Closed

Thom Chiovoloni added 2 commits October 18, 2019 11:22

Run miri in CI, and bump MSRV

28c09eb

thomcc force-pushed the maybe-uninit branch from db5f0b3 to 28c09eb Compare October 19, 2019 01:47

bors-servo merged commit 28c09eb into servo:master Oct 19, 2019

bors-servo mentioned this pull request Oct 19, 2019

Add simple fuzzing #168

Merged

emilio pushed a commit that referenced this pull request Oct 19, 2019

Deprecate unreachable function (#164)

7860d66

This is designed to merged after #162, as it deprecates a function that should be no longer necessary on newer Rust versions.

RalfJung reviewed Oct 19, 2019

View reviewed changes

dpc pushed a commit to dpc/rust-smallvec that referenced this pull request Oct 30, 2019

Deprecate unreachable function (servo#164)

ecaeb59

This is designed to merged after servo#162, as it deprecates a function that should be no longer necessary on newer Rust versions.

mbrubeck mentioned this pull request Oct 30, 2019

Version 1.0.0 #175

Merged

CAD97 mentioned this pull request Nov 2, 2019

Request: new release (miri pass) #177

Closed

RalfJung mentioned this pull request Nov 3, 2019

mem::zeroed/uninit: panic on types that do not permit zero-initialization rust-lang/rust#66059

Merged

RalfJung mentioned this pull request Nov 4, 2019

bump smallvec to 1.0 rust-lang/rust#66086

Merged

This was referenced Nov 4, 2019

Backport MaybeUninit patches to 0.6 #179

Closed

Backport MaybeUninit changes to 0.6 #180

Merged

o0Ignition0o mentioned this pull request Dec 4, 2019

Pass "new" tests servo/rust-url#537

Merged

thomcc deleted the maybe-uninit branch January 29, 2020 12:47

RalfJung mentioned this pull request May 15, 2020

SmallVec < 0.6.13 is unsound rustsec/advisory-db#299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use MaybeUninit for storage of inline items #162

Use MaybeUninit for storage of inline items #162

thomcc commented Sep 8, 2019 •

edited by larsbergstrom

Shnatsel commented Sep 8, 2019

thomcc commented Sep 8, 2019

64 commented Sep 8, 2019

emilio left a comment

emilio Sep 20, 2019

bors-servo commented Oct 15, 2019

jonhoo commented Oct 16, 2019

jonhoo commented Oct 18, 2019 •

edited

thomcc commented Oct 18, 2019 •

edited

thomcc commented Oct 18, 2019

emilio commented Oct 19, 2019

thomcc commented Oct 19, 2019 •

edited

emilio commented Oct 19, 2019

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

RalfJung Oct 19, 2019

jonhoo commented Oct 26, 2019

mbrubeck commented Oct 28, 2019 •

edited

RalfJung commented Nov 4, 2019

RalfJung commented Nov 4, 2019

mbrubeck commented Nov 4, 2019

Use MaybeUninit for storage of inline items #162

Use MaybeUninit for storage of inline items #162

Conversation

thomcc commented Sep 8, 2019 • edited by larsbergstrom

Shnatsel commented Sep 8, 2019

thomcc commented Sep 8, 2019

64 commented Sep 8, 2019

emilio left a comment

Choose a reason for hiding this comment

emilio Sep 20, 2019

Choose a reason for hiding this comment

bors-servo commented Oct 15, 2019

jonhoo commented Oct 16, 2019

jonhoo commented Oct 18, 2019 • edited

thomcc commented Oct 18, 2019 • edited

thomcc commented Oct 18, 2019

emilio commented Oct 19, 2019

thomcc commented Oct 19, 2019 • edited

emilio commented Oct 19, 2019

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

bors-servo commented Oct 19, 2019

RalfJung Oct 19, 2019

Choose a reason for hiding this comment

jonhoo commented Oct 26, 2019

mbrubeck commented Oct 28, 2019 • edited

RalfJung commented Nov 4, 2019

RalfJung commented Nov 4, 2019

mbrubeck commented Nov 4, 2019

thomcc commented Sep 8, 2019 •

edited by larsbergstrom

jonhoo commented Oct 18, 2019 •

edited

thomcc commented Oct 18, 2019 •

edited

thomcc commented Oct 19, 2019 •

edited

mbrubeck commented Oct 28, 2019 •

edited