dynstack icon indicating copy to clipboard operation
dynstack copied to clipboard

Guard macro against unsafe misuse

Open y21 opened this issue 3 years ago • 0 comments

It's currently possible to misuse the dyn_push! macro in various ways. This PR tries to prevent the ones I found:

  • $stack:expr metavariable is expanded in unsafe block. This allows the user to write unsafe code without an unsafe block because of the hidden unsafe block in the macro:
dyn_push!(&mut (*std::ptr::null_mut::<DynStack<dyn Debug>>()), 1);
  • There are no checks in place that $stack is actually a DynStack. A user can pass their own struct with an unsafe push method and the macro will call it.
struct Foo;
impl Foo {
  pub unsafe fn push(&mut self, _: *mut ()) {
    std::hint::unreachable_unchecked()
  }
}
let mut t = Foo;
dyn_push!(t, ());

I've tried to add comments for most of the changes I've made. I don't know if this is the best way, let me know if you have any ideas for improving it.

The call to identify() from the newly introduced sealed MustBeDynStack trait returns &mut DynStack. Even if the user creates a struct with an identity method, they still have to return a &mut DynStack and that means the call to s.push in the macro is guaranteed to be DynStack::push. The reason why it doesn't just do something like let s: &mut DynStack = $stack; (without the trait) is because that would break if you call it as dyn_push!(stack);, and similarily let s: &mut DynStack = &mut $stack breaks if called as dyn_push!(&mut stack). Calling identity as a method will let autoref figure out whether to insert &mut or not and will allow both as before.

y21 avatar Aug 10 '22 00:08 y21