dtolnay / inventory

Typed distributed plugin registration
Apache License 2.0
986 stars 43 forks source link

Soundness: Modify new before it's visible in the registry, not after. #15

Closed MaulingMonkey closed 4 years ago

MaulingMonkey commented 4 years ago

Okay, so undefined behavior would be pretty hard to trigger, but it is possible (I think):

This would be pretty boneheaded thing to do, and won't return the full list of plugins, but I've seen worse in shipped code.

https://github.com/dtolnay/inventory/blob/31b0974e6ab749967ee3506d166302c5a138221c/src/lib.rs#L154-L165

new.next is set after new has been inserted into the registry.

The first commit at least moves the new.next assignment before the CAS makes it visible in the registry. I'm not 100% sure if this gets rid of the undefined behavior however - we technically still have a &mut Node<T> around. (I've also made Registry::submit take the Box instead of the reference so it's at least easier to locally reason about soundness...)

As such, the second commit switches to keeping a ptr::NonNull<T> around instead of a &'static Node<T>.

dtolnay commented 4 years ago

Published in 0.1.5.

MaulingMonkey commented 4 years ago

My pleasure, thanks for writing this crate in the first place 👍