Fix unsoundness issues (mem::zeroed / ManuallyDrop -> MaybeUninit) #458

cynecx · 2020-01-01T23:48:47Z

Several libs in crossbeam contain unsound code (eg. Block::new). This PR fixes them and requires a MSRV upgrade to 1.36.

ghost · 2020-01-06T16:40:16Z

Thank you for the PR! Is it still in progress? Looks good to merge to me :)

We also have some instances of uninitialized memory in bounded queues (files array.rs and array_queue.rs) where it would be good to use MaybeUninit.

cynecx · 2020-01-06T18:11:15Z

@stjepang Well, the issue is the MSRV bump to 1.36 (MaybeUninit), that's why I've opened this as a draft. So you are saying that we should go ahead with the MSRV bump?

We also have some instances of uninitialized memory in bounded queues (files array.rs and array_queue.rs) where it would be good to use MaybeUninit.

Yes, I will update them shortly and mark this PR ready for review then :).

cynecx · 2020-01-06T20:04:58Z

@stjepang There is a failure in crossbeam's array flavoured channel: travis.

---- drops stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `9336`,
 right: `9338`', crossbeam-channel/tests/array.rs:533:9
note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

The MaybeUninit unsoundness fix should be right, unless I am missing something?

EDIT:

nvm I think I missed something, pushing the fix in a moment.

crossbeam-channel/src/flavors/array.rs

crossbeam-queue/src/array_queue.rs

cynecx · 2020-01-06T20:51:41Z

@stjepang It's ready for review now.

cynecx · 2020-01-09T16:05:34Z

This PR is ready now.

README.md

cynecx · 2020-01-17T22:08:39Z

Friendly neighborhood ping.

taiki-e · 2020-01-20T02:18:06Z

fyi: by using https://crates.io/crates/maybe-uninit crate, we can avoid the MSRV bump (like smallvec 0.6 did)

spacejam · 2020-01-27T12:54:40Z

@taiki-e that seems really appealing to me. maybe-uninit seems like a pretty lightweight crate, 0 dependencies and over 1 million downloads since its fairly recent release in November 2019. Some pretty large projects rely on this crate and it's nice to avoid the transitive MSRV bump decision if they would otherwise be willing to update. This then allows a larger number of Rust users of unsafe code to make more progress when running miri. (that is actually what led me here - I want to use miri on more parts of sled).

ghost · 2020-01-27T21:48:28Z

I think we should just bump the minimum Rust version and consider publishing crossbeam 1.0 soon. A lot of unsafe code in crossbeam is really dealing with uninitialized memory so it's important we start using MaybeUninit ASAP.

spacejam · 2020-01-28T08:42:00Z

one final consideration, @stjepang mentioned to me yesterday that rayon lags by a year. bumping MSRV to 1.36 means they need to wait until July 4 2020 before they can reduce their reliance on the uninitialized usage here. By using the maybe-uninit crate as a stopgap measure until july 4, it's possible for crossbeam to do a non-breaking patch update that silently reduces risk for more users. but, I'm not the one maintaining this, so I'll duck out at this point. although, if using that crate is a desired solution, I'm happy to implement it real quick since I'm an unemployed hobo.

cynecx · 2020-01-29T17:22:41Z

@spacejam’s reasoning is really convincing. ping @stjepang.

jonhoo · 2020-01-31T21:31:05Z

I would love to see this land so that miri will work on downstream crates like https://github.com/jonhoo/flurry too!

jonhoo · 2020-02-04T16:37:11Z

@jeehoonkang @stjepang what are next steps for this PR? I think we need your decision for who to best move forward. It'd probably be good to land this PR in one form or another sooner rather than later.

This removes the patch that incorporates crossbeam-rs/crossbeam#458, since that PR also bumps the version number, and 0.9 doesn't exist on crates.io. This in turn means that the miri test has to be disabled for this release in particular. I'll push a commit after release that restores it and the crossbeam patch. This commit also adds a changelog!

jeehoonkang · 2020-02-06T08:26:49Z

I prefer maybe-uninit to bumping MSRV. It looks like a fine temporary measure we can take.

jonhoo · 2020-02-06T14:37:21Z

Thanks! @cynecx, do you have the time to update the PR to use maybe-uninit?

cynecx · 2020-02-06T15:57:31Z

@jonhoo Sure :) The changes should be up in an hour(-ish)!

cynecx · 2020-02-06T16:26:17Z

A question though, instead of adding maybe-uninit to each crate, should I just put it in crossbeam-utils and export a type-alias (MaybeUninit)? This would have a benefit, that future updates would simply have to change the type-alias in order to change the underlying MaybeUninit implementation. Also most of the affected crates are already having crossbeam-utils as a dependency, so that we just have to add the maybe-uninit dependency in crossbeam-utils. Thoughts?

jonhoo · 2020-02-06T16:28:40Z

I would keep it separate, since (I think) it's weird for crossbeam-utils crate to publicly expose that type alias. If it could expose it just to other crossbeam crates, then sure, but I don't think that's really an option.

cuviper · 2020-02-06T16:30:42Z

If you reexport it, it becomes part of the public API, including the fact that it's identical to maybe-uninit's type. You'd have to wrap it in a newtype to avoid that equality, to let you freely change the impl.

…rop_in_place

…MaybeUninit

…ss statement)

cynecx · 2020-02-10T15:38:42Z

~~I hope it's okay if I force-push now to address the merge-conflict? Because that will mess-up the review for sure xD.~~

Whatever, seems like the review boxed are marked outdated anyway.

jeehoonkang · 2020-02-12T01:09:50Z

Thanks!

bors r+

458: Fix unsoundness issues (mem::zeroed / ManuallyDrop -> MaybeUninit) r=jeehoonkang a=cynecx Several libs in crossbeam contain unsound code (eg. `Block::new`). This PR fixes them and requires a MSRV upgrade to 1.36. Co-authored-by: cynecx <me@cynecx.net>

bors · 2020-02-12T01:27:49Z

Build succeeded

continuous-integration/travis-ci/push

jonhoo · 2020-02-12T14:34:16Z

@jeehoonkang Nice, thanks! What's the plan for next release between this and #466 ? Would be good to land something soon to get those out.

474: Patch release for all crossbeam crates. r=jeehoonkang a=jonhoo This bumps the patch version for all sub-crates and the top-level crate and updates the changelogs with references to PRs where applicable. As far as I can tell, there are no breaking changes. The top-level crate has not change at all, though I bumped `rand` to `0.7` in all the crates, which I guess technically requires a patch bump if we do another release. Fixes #473. Closes #472. Closes #468. Closes #409. Not sure if this also solves #347 by virtue of #458? Co-authored-by: Jon Gjengset <jon@thesquareplanet.com>

This adds one new dependency, maybe-uninit, which is brought in by crossbeam-channel[1]. This is to apparently fix some unsound code without bumping the MSRV. Since ripgrep uses the latest stable release of Rust, the maybe-uninit crate should compile down to nothing and just re-export std's `MaybeUninit` type. [1] - crossbeam-rs/crossbeam#458

774: Reduce unsafe code in array queue/bounded channel r=taiki-e a=taiki-e Before #458, since destructors could be called on elements, the correct way was to convert the Vec to a pointer, save it, and then [deallocate it as a Vec with zero length](https://github.com/crossbeam-rs/crossbeam/blob/28ad2b7e015832b47db7e389dd9ebce3e94b3adb/crossbeam-channel/src/flavors/array.rs#L549-L552). However, after #458, destructors of the elements wrapped by MaybeUninit are not called, so there is no need to save the Vec/boxed slice as a pointer. This removes the unsafe code that has caused several (potential) unsoundnesses in the past (#500, #533, #763). Co-authored-by: Taiki Endo <te316e89@gmail.com>

cynecx changed the title ~~Fix unsoundness issues (mem::{zeroed, uninitialized} / ManuallyDrop -> MaybeUninit)~~ Fix unsoundness issues (mem::zeroed / ManuallyDrop -> MaybeUninit) Jan 1, 2020

cynecx commented Jan 6, 2020

View reviewed changes

crossbeam-channel/src/flavors/array.rs Outdated Show resolved Hide resolved

cynecx commented Jan 6, 2020

View reviewed changes

crossbeam-queue/src/array_queue.rs Outdated Show resolved Hide resolved

cynecx marked this pull request as ready for review January 6, 2020 20:51

cuviper reviewed Jan 9, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

divergentdave mentioned this pull request Jan 24, 2020

Miri support spacejam/sled#937

Merged

jonhoo mentioned this pull request Jan 27, 2020

Add miri CI test jonhoo/flurry#37

Merged

jonhoo mentioned this pull request Jan 29, 2020

Memory leak from crossbeam_epoch::deferred::Deferred::new #464

Closed

jonhoo mentioned this pull request Feb 4, 2020

Prep for release of 0.1.0 jonhoo/flurry#60

Merged

cynecx added 6 commits February 10, 2020 16:32

channel: add missing drop_in_place for array channel flavour's msgs

dce3e11

queue: add missing drop_in_place for array_queue slot values

d78de60

drop_in_place cleanups

1a8cbe1

cleanup drop impl for ArrayQueue / Channel by eliminating one no-op d…

7de64fe

…rop_in_place

Use maybe-uninit crate to bridge backwards compatibility issues with …

b5525b7

…MaybeUninit

Cleanup some things (Removed stale comment and add semicolon to expre…

a86b3b6

…ss statement)

cynecx force-pushed the fix-unsoundness branch from 3832f5a to a86b3b6 Compare February 10, 2020 15:39

Fix formatting issues in crossbeam-queue/src/array_queue.rs

8abcc14

cynecx requested a review from jeehoonkang February 10, 2020 16:16

Use more precise unsafe scopes

8903df8

bors bot merged commit 8903df8 into crossbeam-rs:master Feb 12, 2020

This was referenced Feb 17, 2020

Publish new releases of crossbeam crates #473

Closed

Patch release for all crossbeam crates. #474

Merged

jonhoo mentioned this pull request Feb 17, 2020

Crossbeam channels trigger valgrind errors servo/servo#23077

Open

twe4ked mentioned this pull request Feb 19, 2020

Add basic HashSet implementation jonhoo/flurry#19

Merged

jonhoo mentioned this pull request May 19, 2020

Publish version 1.0 #503

Open

taiki-e mentioned this pull request Jan 22, 2022

Reduce unsafe code in array queue/bounded channel #774

Merged

taiki-e mentioned this pull request Feb 17, 2022

epoch: possible reads of uninitialized data #347

Closed

RalfJung mentioned this pull request May 10, 2022

add crossbeam advisories for incorrect (unsound) zeroed memory rustsec/advisory-db#1231

Merged

RalfJung mentioned this pull request Jul 11, 2022

Tracking issue for panics in mem::uninitialized/zeroed rust-lang/rust#66151

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unsoundness issues (mem::zeroed / ManuallyDrop -> MaybeUninit) #458

Fix unsoundness issues (mem::zeroed / ManuallyDrop -> MaybeUninit) #458

cynecx commented Jan 1, 2020 •

edited

ghost commented Jan 6, 2020

cynecx commented Jan 6, 2020

cynecx commented Jan 6, 2020 •

edited

cynecx commented Jan 6, 2020

cynecx commented Jan 9, 2020 •

edited

cynecx commented Jan 17, 2020

taiki-e commented Jan 20, 2020

spacejam commented Jan 27, 2020 •

edited

ghost commented Jan 27, 2020

spacejam commented Jan 28, 2020

cynecx commented Jan 29, 2020 •

edited

jonhoo commented Jan 31, 2020

jonhoo commented Feb 4, 2020

jeehoonkang commented Feb 6, 2020

jonhoo commented Feb 6, 2020

cynecx commented Feb 6, 2020 •

edited

cynecx commented Feb 6, 2020 •

edited

jonhoo commented Feb 6, 2020

cuviper commented Feb 6, 2020

cynecx commented Feb 10, 2020 •

edited

jeehoonkang commented Feb 12, 2020

bors bot commented Feb 12, 2020

jonhoo commented Feb 12, 2020

Fix unsoundness issues (mem::zeroed / ManuallyDrop -> MaybeUninit) #458

Fix unsoundness issues (mem::zeroed / ManuallyDrop -> MaybeUninit) #458

Conversation

cynecx commented Jan 1, 2020 • edited

ghost commented Jan 6, 2020

cynecx commented Jan 6, 2020

cynecx commented Jan 6, 2020 • edited

cynecx commented Jan 6, 2020

cynecx commented Jan 9, 2020 • edited

cynecx commented Jan 17, 2020

taiki-e commented Jan 20, 2020

spacejam commented Jan 27, 2020 • edited

ghost commented Jan 27, 2020

spacejam commented Jan 28, 2020

cynecx commented Jan 29, 2020 • edited

jonhoo commented Jan 31, 2020

jonhoo commented Feb 4, 2020

jeehoonkang commented Feb 6, 2020

jonhoo commented Feb 6, 2020

cynecx commented Feb 6, 2020 • edited

cynecx commented Feb 6, 2020 • edited

jonhoo commented Feb 6, 2020

cuviper commented Feb 6, 2020

cynecx commented Feb 10, 2020 • edited

jeehoonkang commented Feb 12, 2020

bors bot commented Feb 12, 2020

Build succeeded

jonhoo commented Feb 12, 2020

cynecx commented Jan 1, 2020 •

edited

cynecx commented Jan 6, 2020 •

edited

cynecx commented Jan 9, 2020 •

edited

spacejam commented Jan 27, 2020 •

edited

cynecx commented Jan 29, 2020 •

edited

cynecx commented Feb 6, 2020 •

edited

cynecx commented Feb 6, 2020 •

edited

cynecx commented Feb 10, 2020 •

edited