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

Audit the sentinel node for Unsafe Code Guidelines Violations #97

Open
Gankra opened this issue Aug 16, 2019 · 5 comments

Comments

@Gankra
Copy link
Contributor

commented Aug 16, 2019

As discussed in #96, LinkedHashMap is some very old code that uses some old patterns that the UCG really doesn't like.

@Gankra

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Whoops accidentally hit submit. Copying my comment from the PR:


Yikes. Ok so this is definitely an improvement but I think more needs to be done to make this rigorously correct. In particular it is very easy to accidentally assert that the K and V exist transiently, leading to technical but unlikely to matter UB.

I believe the correct fix here is a more subtle and frankly very annoying, in the same vein as these fixes we did for BTreeMap and LinkedList.


Extra info: we allocate a sentinel ("guard") node for the map with an uninitialized K/V, which is a standard doubly-linked-list trick to make things a bit simpler by basically making it a big ring, with this sentinel joining the two ends. The problem is that we don't have a type-level distinction between "a node" and "a node that has a K/V pair", so if you ever access the sentinel with a reference, we are, potentially (grey area in the UCG), implicitly asserting that the K/V is initialized to valid values (impossible for us to do).

Note that our situation isn't exactly the same as BTreeMap: it could never even allocate space for K/V, as every BTreeMap shares this one statically allocated node. This meant a reference to a Node was partially dangling, instead of "just" pointing at uninitialized memory. The former is more obviously busted in the eyes of the UCG, as I understand it.

As BTreeMap established, the "correct" fix for this is to have two types, Node and ValueNode, where everything points to Node and you only cast to ValueNode when you know you're not looking at the sentinel.

Alternatively, we can just very carefully avoid ever constructing &Node or &mut Node if it's ever possible that we're pointing at the sentinel (basically only manipulating it through a raw pointer). This is potentially possible!

Alternatively Alternatively, we can wrap the K/V in a MaybeUninit, which to me feels like a "bad solution" but honestly might just be the best one? It'll mostly just add lots of random noise anytime we want to look at the key and value, which admittedly so would the BTreeMap solution, just "higher up" in the logic.

@Gankra

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

fun unsafe evil for @RalfJung's catalogue

@Gankra

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

As fun trivia, although this sounds like a case where maybe the absence of &raw would be relevant, I think this perfectly dodges around the problem, as:

  • we have no interest in actually initializing the sentinel
  • we get a raw pointer to the sentinel directly from alloc
  • normal nodes just use Box::new
@RalfJung

This comment has been minimized.

Copy link

commented Aug 16, 2019

My personal stanza on this is we should allow references to uninitialized memory. But we'll likely have to make an exception for references to uninhabited types (including things like (u32, !)), because optimizations.

If K or V is !, is it possible to ever hit a code path that creates a node reference?

@Gankra

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

No, the empty map doesn't allocate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.