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

It is unsound to accept arbitrary user-constructed Guards #46

Closed
jonhoo opened this issue Jan 30, 2020 · 1 comment
Closed

It is unsound to accept arbitrary user-constructed Guards #46

jonhoo opened this issue Jan 30, 2020 · 1 comment

Comments

@jonhoo
Copy link
Owner

jonhoo commented Jan 30, 2020

As @Amanieu points out in #44 (comment), the current API which lets users supply their own Guard references to our methods is unsound. Specifically, a user can do the following:

use crossbeam_epoch::*;
let map = HashMap::default();
map.insert(42, String::from("hello"), &epoch::pin());

let evil = crossbeam_epoch::Collector::new();
let evil = evil.register();
let oops = map.get(&42, &evil.guard());

map.remove(&42, &epoch::pin());
// at this point, the default collector is allowed to free `"hello"`
// since no-one has the global epoch pinned as far as it is aware.
// `oops` is tied to the lifetime of a Guard that is not a part of
// the same epoch group, and so can now be dangling.
// but we can still access it!
assert_eq!(oops, "hello");

Here is a sketch of a possible solution, largely inspired by @Amanieu's proposal + #45.

First, we add a Collector field to HashMap, and add a constructor that takes a Collector (this can be no_std). The default() and new() constructors use epoch::default_collector().clone() as their Collector. Then, we add a check to every method that takes a &Guard (which is essentially all of them at the moment) that checks that the given Guard's collector matches self.collector (see SkipList::check_guard). This should take care of the safety issues.

We then add a wrapper type that is tied to a single Guard and exposes all the methods (and some more convenience ones like Index) without a Guard argument. This is #45. It should also have two constructors. One that takes a user-supplied Guard (and checks it), and one that does not (and uses epoch::pin). There should be a method to extract the Guard again from the HashMapRef (since it needs to take ownership of it to also have a variant that uses epoch::pin).

@cuviper
Copy link
Collaborator

cuviper commented Jan 30, 2020

If we use a Cow-like enum, we can still just borrow a user's guard. (Not std::borrow::Cow directly since Guard can't "copy" with ToOwned.)

jonhoo added a commit that referenced this issue Jan 30, 2020
@jonhoo jonhoo closed this as completed in f0d7cf2 Jan 31, 2020
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

No branches or pull requests

2 participants