Pin and Unsafe usage code review

code review
4

Your code is unsound. As a minimal example, it causes this safe program to produce a segmentation fault when run:

use compose_rt::{Composer, Recomposer};
use std::mem;

fn main() {
    let group = |cx: &mut Composer, cached: bool| {
        cx.group(
            |_| "Hello, World!",
            |cx| {
                if cached {
                    let mut recomp = Recomposer::new();
                    mem::swap(cx, recomp.composer());
                }
            },
            |_| false,
            |node| assert_eq!(*node, "Hello, World!"),
            |_| {},
        );
    };
    let mut recomp = Recomposer::new();
    group(recomp.composer(), false);
    recomp.finalize();
    group(recomp.composer(), true);
}

This is why unsafe casts should never be used to extend the lifetime of references. Without the pointer cast, the compiler gives an error about this:

error[E0499]: cannot borrow `*self` as mutable more than once at a time
   --> /home/lm978/compose-rt/compose-rt/src/composer.rs:243:42
    |
222 |         let cached = self.tape.get_mut(cursor).map(|s| {
    |                      ------------------------- first mutable borrow occurs here
...
243 |                         let c = children(self);
    |                                          ^^^^ second mutable borrow occurs here
244 |                         apply_children(node, c);
    |                                        ---- first borrow later used here

(In fact, this compiler error is what enabled me to design the example above: it shows exactly where the issue occurs.) Therefore, I would recommend taking the negligible performance hit from a RefCell, to prevent errors like this. unsafe blocks should only be used when one is absolutely sure that the preconditions can never be violated; the ideal scenario is when the unsafety is contained within a single function that does not call anything else.

1 Like