"test_rc_cyclic_with_one_ref" fails in Miri

[RalfJung]

In the latest test suite run in Miri, test_rc_cyclic_with_one_ref failed. This test was added in f03d0b3 as part of #75994. The cyclic Arc tests work fine, so either the tests are different or something about the Rc implementation is.

The error is "error: Undefined Behavior: not granting access to tag because incompatible item is protected: [Unique for <74172040> (call 24024731)]", which means that some memory is deallocated while a mutable reference to it exists that is kept alive because it was passed as an argument to some function that is still running.

Cc @mental32

[mental32]

Thanks for notifying me @RalfJung

The tests do differ with the by the last two lines (in test_rc_cyclic_with_one_ref) where Rc has:

    assert_eq!(one_ref.inner.strong_count(), 2);
    assert_eq!(one_ref.inner.weak_count(), 1);

Arc has:

    assert_eq!(Arc::strong_count(&one_ref), 2);
    assert_eq!(Arc::weak_count(&one_ref), 1);

I can't see where in the implementations Rc::new_cyclic vs Arc::new_cyclic (which Rc::new_cyclic was based on) behavior differs so I'm led to believe that this difference in the tests is causing the fail.

What is the procedure for patching this? should I simply open a PR with the suspected fix and mention this issue?

[RalfJung]

The tests are entirely safe code though, so that's not where the bug is. The bug is necessarily somewhere around unsafe code.

I'll try to investigate more, but won't get around to that before Friday.

[carbotaniuman]

I did some digging, and my guess would be this method used in the Drop impl for Arc:

    pub unsafe fn get_mut_unchecked(this: &mut Self) -> &mut T {
        // We are careful to *not* create a reference covering the "count" fields, as
        // this would alias with concurrent access to the reference counts (e.g. by `Weak`).
        unsafe { &mut (*this.ptr.as_ptr()).data }
    }

Rc just does ptr::drop_in_place(self.ptr.as_mut());, which creates a reference covering the count fields. This was probably never tested because it would've been impossible to create such a struct OneRef without interior mutability.

[RalfJung]

Ah I had missed that you were already working on a fix and now I have developed my own. :/ Yours is probably better though, so let's go with that. In the future, please make a note in the issue when you have a PR, so that others know -- there are no notifications for pingbacks.

[carbotaniuman]

Ah I had missed that you were already working on a fix and now I have developed my own. :/ Yours is probably better though, so let's go with that. In the future, please make a note in the issue when you have a PR, so that others know -- there are no notifications for pingbacks.

Ah sorry about that, still new to contributing here, just left a comment :( In the future, would pinging be acceptable?

[RalfJung]

Sure you could also ping me on the PR, though if you just write a brief note here in the issue everyone subscribed will see it. :)

And it's not a big deal, don't worry. I probably also should have re-read the issue before trying to fix the problem myself.