Unsoundness in ghost-cursor

[noamtashma]

Here is a counterexample showing ghost-cursor is unsound:

fn ghost_cursor_counterexample() {
    GhostToken::new(|mut token| {
        let cell = GhostCell::new(Box::new(GhostCell::new(7)));
        let cursor = GhostCursor::new(&mut token, Some(&cell));
        let inner_cursor = cursor.move_into(|cell| Some(cell)).ok().unwrap();
        let (token, inner_cell_ref_opt) = inner_cursor.into_parts();
        let inner_cell_ref = inner_cell_ref_opt.unwrap();
        // Free the box
        *cell.borrow_mut(token) = Box::new(GhostCell::new(3));
        // Use-after-free!
        println!("{}", inner_cell_ref.borrow(token));
    });
}

This works by using the ghost-cursor in order to obtain a long-living reference in the internal GhostCell, without borrowing the token for the same lifetime. This wouldn't be possible ordinarily. Then, the token is free to be reused to access the outer GhostCell to drop the inner GhostCell.

I think there are two interesting changes to consider that might fix this problem:

• Removing the into_parts method.
• Ensuring that the cursor must outlive any reference that it gives out, even if they're ghost cells. This essentially means dropping the &GhostCell<T> output from the into_parts method.

Maybe even having it only return an immutable reference to the token will also be fine (like before This issue)?

[matthieu-m]

The only known usecase of into_parts I know of is my own in ghost-collections: https://github.com/matthieu-m/ghost-collections/blob/master/src/linked_list/cursor.rs#L105 . Of note, it does not require a mutable reference to the token.

I was double-checking the tests, and while they do check that the original token is still borrowed mutably, they do not use the fact that the newly returned token is a mutable reference either.

I believe the usecase in ghost-collections is legitimate, that is, it is legitimate to want the cursor functionality, to have both a concurrent read-only Cursor types and a mutable CursorMut type, and it is legitimate to downgrade a CursorMut into a Cursor, if only to be able to call a function accepting the latter after navigating (and mutating) using the former.

As such, I am none too keen on the first two suggestions (as is): removing into_parts, or removing part of its output, mean cutting out this piece of functionality. Unless, of course, equivalent functionality is added in another way.

I believe this only leaves us with two solutions:

• Reverting the change to into_parts, and going back to returning an immutable token, with a comment explaining the issue.
• Duplicating GhostCursor for immutable and mutable borrows of the token, and implementing the conversion internally, allowing us to remove into_parts whilst allowing ghost-collections to retain its cursors.

And, to be honest, I'd rather go with the former soonish, so as to yank unsound versions whilst providing a minimum effort upgrade.

[matthieu-m]

Fixed in 0242a51.

The release will wait for the ongoing work on fixing check_distinct.

[noamtashma]

I actully think it should be possible both variants. In other words, to also add a method 'into_token(self) -> &'a mut GhostToken<'brand>`.

[matthieu-m]

It certainly would be possible, but it may not be necessary: dropping the cursor allows to use the original token as it's no longer borrowed.

It could be added later as a convenience method.