Dubious use of unsafe in runtime::store::JsValueStore.replace()

[tomoverlund]

This is another extraction from #5370. This is the potential problem: "C2. Unsoundness in JsValueStore::replace — mutates data through a shared Arc" as written in that issue.

This is the code in question: core/runtime/src/store/mod.rs:

unsafe fn replace(&mut self, other: ValueStoreInner) {
    let ptr = Arc::as_ptr(&self.0).cast_mut();

    assert!(!ptr.is_null());
    unsafe {
        assert!(
            matches!(*ptr, ValueStoreInner::Empty),
            "ValueStoreInner must be empty."
        );

        *ptr = other;
    }
}

It has the following comment justifying the unsafe block:

 /// # SAFETY
 /// This should only be done if the inner content is [`ValueStoreInner::Empty`]
 /// and only by the creator of the current [`JsValueStore`]. We enforce the first
 /// rule at runtime (and will panic), and the second rule by requiring a mutable
 /// reference. This is still unsafe and relies on unsafe pointer access.

This is the essence of Claude's complaint (see #5370 for complete details):

Mutating data behind a shared Arc(which is &T, not &mut T) is undefined behaviour even when single-threaded; the assert!(matches!(*ptr, ValueStoreInner::Empty)) does not rescue soundness because the other live clones currently observe Empty but their later Deref operations will read Object/Array/Map through a non-UnsafeCell shared reference.

Note the crucial point that even in a single-threaded context, this is still undefined behavior. I asked ChatGPT about this audit, and it says, in part:

The concern raised in that audit is generally valid. The key issue is not threading; it's Rust's aliasing model.

and:

...writing through a raw pointer obtained from Arc::as_ptr() is mutating memory that is currently aliased by shared references. That is exactly the kind of operation Rust declares UB.

I don't have code to demonstrate this fails, and it is tested indirectly via message/tests.rs, but that doesn't mean the code is valid or won't fail in all scenarios (optimizations for a particular architecture, future optimizations, etc). Ideally, the code in question would be replaced with safe code.

In my ChatGPT conversation, I asked for alternatives:

The replace() function is trying to create a store with potential recursive data, and then never mutate it after. What would be the recommended way to achieve this safely without unnecessary overhead?

and it replied, in part:

This is a classic problem: you need to construct a graph that may contain cycles, but you want the final structure to be immutable.

The usual solution is not to mutate through a shared Arc, but instead to introduce a temporary construction phase where mutation is permitted, and then "freeze" the structure.

with the following options:

1. Build with Arc::new_cyclic
2. Two-phase construction with OnceLock
3. Two-phase construction with UnsafeCell (lowest overhead)
4. Arena + freeze (often the cleanest)

with the concluding recommendation:

OnceLock is almost certainly the closest drop-in replacement that preserves the existing algorithm while restoring soundness. The runtime cost is usually negligible compared to deserializing recursive structures.

But do read the chat link for all the details with pros and cons.

If unsafe is absolutely needed for performance, then it should be the appropriate unsafe, though I would caution against unsafe code in general WITHOUT A BENCHMARK TO SHOW IT'S NEEDED. Sorry for the all-caps, but unsafe is a nasty business, and we really should be avoiding premature optimization. Related issue: #4181.