Generated `WithRef` implementations can’t be sound when interior mutability is at play

[steffahn]

In other words, you can’t just copy a value to a different place, and then expose that copy by-immutable-reference to untrusted (safe) user code, because immutable references could have mutable access behind interior mutability.

use soapy::{Soa, Soapy, WithRef};
use std::cell::RefCell;

#[derive(Soapy)]
struct Foo(RefCell<String>);

fn main() {
    let mut x = Soa::<Foo>::new();
    x.push(Foo(RefCell::new(String::from("Hello World"))));

    let r = x.get(0).unwrap();

    // mem::take gets ownership and modifies the `String`, however these modifications are only happening
    // to the local copy via `ptr::read` in the `unsafe` implementation of `WithRef`, so the original `String`
    // keeps ownership, too
    let s1: String =
        r.with_ref(|copy_of_refcell| std::mem::take::<String>(&mut copy_of_refcell.0.borrow_mut()));

    // let's just do it again, because copy-paste is easy
    let s2: String =
        r.with_ref(|copy_of_refcell| std::mem::take::<String>(&mut copy_of_refcell.0.borrow_mut()));

    println!("{s1} and {s2} again?"); // ==>> "Hello World and Hello World again?"
    drop(s1);
    println!("freed!");
    drop(s2); // double-free
    println!("freed again!");

    // (triple-free, once `x` is dropped)
}

a workaround of “just write back the changes” of course can’t work either, as that isn’t thread-safe, whilst Soa promises Sync.

I don’t have any immediate ideas on how to keep the nice implementations of Hash and Eq and the like.

[tim-harding]

Shoot, that's quite inconvenient. Thanks for bringing it to my attention. This issue seems trickier to address than the other one you brought up. Maybe a better API would be something like map and map_idx that takes a value and returns a new value to replace it. Hopefully the compiler could elide things when no mutation occurs.

[tim-harding]

I replaced WithRef with AsSoaRef, which is a similar idea except that instead of creating &T from Ref<T>, it creates Ref<T> from T. Instead of falling back on T's implementation of common traits, there is now an additional soa_derive attribute for the Soapy derive that allows for deriving those traits on the generated types.

[simonsan]

Does it mean, that this library is sound now and can be used in production?

[tim-harding]

I think so, assuming @steffahn uncovered all instances of unsoundness in his initial audit (thanks again, I learned a lot from your review). Still, I'll be going back through all the unsafe blocks for another round of self-auditing, adding more documentation and tests, and hopefully reducing the amount of unsafe in general.

[steffahn]

I don’t want to claim to have done any sort of complete audit of the API’s soundness. I just looked around until I had found an issue, and a bit more until finding another.

On the other hand, it sounds like @tim-harding is trying to be cautious of avoiding soundness issues, running tests with miri, etc… so you probably/realistically wouldn’t have had issues even before the changes[^1] if you weren’t “using it wrong” (though that’s the power of Rust, we needn’t blame the user for using something “wrong”, we have better criteria [“uses safe code”] for that).

[^1]: Except that the fixes involved breaking change to the API, so that would have been the real impact to users here, most likely.

[simonsan]

Thanks! 🚀