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

Remove unsound custom Cell #158

Merged
merged 3 commits into from
Jul 19, 2020
Merged

Remove unsound custom Cell #158

merged 3 commits into from
Jul 19, 2020

Conversation

Shnatsel
Copy link
Contributor

Right now the custom cell is unsound because Cell::get_mut() uses Rc::as_ref() to obtain a reference to the underlying data, which does not distinguish between mutable and immutable references, and so does not guarantee uniqueness. Thus it is possible to obtain several mutable references to the same memory location by calling Cell::get_mut() repeatedly:

let mycell = Cell::new(vec![1,2,3]);
let ref1 = mysell.get_mut();
let ref2 = mysell.get_mut(); // obtained a second mutable reference; UB starts here

This unsoundness can even be triggered from the public API (see PoC that fails MIRI). It may result in pretty much arbitrary memory corruption, most likely a use-after-free.

This PR removes the unsound custom cell and replaces it with Rc<RefCell<T>. This reverts the code to the way it was before the introduction of the custom cell in 20b03a4
I expect performance to take a slight hit, but the difference not to be noticeable outside of microbenchmarks. cargo bench shows no difference between this branch and master.

The code is heavily based on the patch by @Nemo157 https://gist.github.com/Nemo157/b495b84b5b2f3134d19ad71ba5002066, all I did was resolve conflicts with latest master.

This PR supersedes #113 which has been stuck for 6 months and didn't fix the problem entirely, since the unsound custom cell was still in use.

@robjtede robjtede mentioned this pull request Jul 19, 2020
15 tasks
@codecov
Copy link

codecov bot commented Jul 19, 2020

Codecov Report

Merging #158 into master will decrease coverage by 0.25%.
The diff coverage is 73.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
- Coverage   61.59%   61.34%   -0.26%     
==========================================
  Files          80       79       -1     
  Lines        5028     5011      -17     
==========================================
- Hits         3097     3074      -23     
- Misses       1931     1937       +6     
Impacted Files Coverage Δ
actix-service/src/apply_cfg.rs 0.00% <0.00%> (ø)
actix-service/src/lib.rs 42.42% <ø> (ø)
actix-service/src/and_then.rs 88.75% <100.00%> (-2.50%) ⬇️
actix-service/src/and_then_apply_fn.rs 88.31% <100.00%> (-1.30%) ⬇️
actix-service/src/then.rs 88.76% <100.00%> (-2.25%) ⬇️
actix-server/src/lib.rs 50.00% <0.00%> (-16.67%) ⬇️
actix-rt/src/system.rs 57.89% <0.00%> (-2.64%) ⬇️
actix-utils/src/inflight.rs 92.68% <0.00%> (-2.44%) ⬇️
actix-service/src/map_err.rs 81.39% <0.00%> (-2.33%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61176f6...bc17dc1. Read the comment docs.

@robjtede robjtede requested review from a team and removed request for a team July 19, 2020 15:31
@robjtede
Copy link
Member

Thanks so much for expiditing this. Can you mention fix in the changelog.

@Shnatsel
Copy link
Contributor Author

Can you mention fix in the changelog.

Done.

@robjtede robjtede requested review from a team July 19, 2020 19:11
@Shnatsel
Copy link
Contributor Author

Error message from failed check is:

Unable to update registry https://github.com/rust-lang/crates.io-index

So looks like it's just flaky and needs to be retried.

@Shnatsel
Copy link
Contributor Author

CI is green now.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks a lot!

@Shnatsel
Copy link
Contributor Author

Thanks for the quick review and merge!

Please let me know once a version incorporating these changes is released. I'd like to open a RustSec advisory so that anyone interested in security would know to update.

@Shnatsel Shnatsel deleted the cellless branch July 19, 2020 23:01
@Shnatsel
Copy link
Contributor Author

Oh wait, there's another copy if this Cell implementation in the repo. I'll open an issue about that one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants