Missing UB in data race involving socketpair

[FrankReh]

While looking at the situation where the last epoll changes for clock sync made it over sync, test cases were not being resolved as expected as the code was made less generous.

I think this showed a different problem in Miri that hadn't been reported yet.

Here is the problem demonstrated with the libc-socketpair in Miri, no epoll.

With this diff, the miri test fails in test_race3, indicating UB: data race. That's expected.

An exhibition of the problem, I think, is that test_race2 should also have a UB: data race reported, but it does not. And I believe it does not because the main thread was synced to the clock of the sub-thread, and that clock also covered the write to the global after what should have been the synchronizing event. The synchronizing event was only meant to build a happens-before relationship.

diff --git a/tests/pass-dep/libc/libc-socketpair.rs b/tests/pass-dep/libc/libc-socketpair.rs
index 64819e576..b38be4b92 100644
--- a/tests/pass-dep/libc/libc-socketpair.rs
+++ b/tests/pass-dep/libc/libc-socketpair.rs
@@ -10,6 +10,8 @@ fn main() {
     test_socketpair();
     test_socketpair_threaded();
     test_race();
+    test_race2();
+    test_race3();
 }
 
 fn test_socketpair() {
@@ -136,3 +138,63 @@ fn test_race() {
     thread::yield_now();
     thread1.join().unwrap();
 }
+
+fn test_race2() {
+    static mut VAL: u8 = 0;
+    let mut fds = [-1, -1];
+    let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) };
+    assert_eq!(res, 0);
+    let thread1 = thread::spawn(move || {
+
+        let data = "a".as_bytes().as_ptr();
+        let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 1) };
+        assert_eq!(res, 1);
+
+
+        unsafe { VAL = 1 };
+    });
+    thread::yield_now();
+
+    let mut buf: [u8; 1] = [0; 1];
+    let res: i32 = unsafe {
+        libc::read(fds[1], buf.as_mut_ptr().cast(), buf.len() as libc::size_t)
+            .try_into()
+            .unwrap()
+    };
+    assert_eq!(res, 1);
+    assert_eq!(buf, "a".as_bytes());
+
+    unsafe { assert_eq!(VAL, 1) };
+
+    thread1.join().unwrap();
+}
+
+fn test_race3() {
+    static mut VAL: u8 = 0;
+    // let mut fds = [-1, -1];
+    // let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) };
+    // assert_eq!(res, 0);
+    let thread1 = thread::spawn(move || {
+
+        // let data = "a".as_bytes().as_ptr();
+        // let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 1) };
+        // assert_eq!(res, 1);
+
+
+        unsafe { VAL = 1 };
+    });
+    thread::yield_now();
+
+    // let mut buf: [u8; 1] = [0; 1];
+    // let res: i32 = unsafe {
+    //     libc::read(fds[1], buf.as_mut_ptr().cast(), buf.len() as libc::size_t)
+    //         .try_into()
+    //         .unwrap()
+    // };
+    // assert_eq!(res, 1);
+    // assert_eq!(buf, "a".as_bytes());
+
+    unsafe { assert_eq!(VAL, 1) };
+
+    thread1.join().unwrap();
+}

[FrankReh]

Where is the miri shim for writing to the global static? One way I see fixing the problem that I am perceiving would be to increment the clock when the thread writes to the global static - I have to assume it isn't being incremented given the symptoms of miri test cases that do and do not report UB: data race.

[FrankReh]

Given more time to think about it, looking for where the global static is being written is wrong.

I would content that release_clock would work as intended if the value being shared did not reflect the incremented value.
It should share the value before it was incremented. release_clock should still increment the value.

So change the return type to an owned clock, or keep a prev_clock for just this purpose of passing a reference. But one way or the other, return the clock value before the thread's clock was incremented.

With that change, all the old miri self tests still pass, plus the two new ones I showed above both report UB: data race, as they should.

[RalfJung]

Okay so the test case is this, with preemption disabled:

use std::thread;

fn main() {
    static mut VAL: u8 = 0;
    let mut fds = [-1, -1];
    let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) };
    assert_eq!(res, 0);
    let thread1 = thread::spawn(move || {

        let data = "a".as_bytes().as_ptr();
        let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 1) };
        assert_eq!(res, 1);

        unsafe { VAL = 1 };
    });
    thread::yield_now();

    let mut buf: [u8; 1] = [0; 1];
    let res: i32 = unsafe {
        libc::read(fds[1], buf.as_mut_ptr().cast(), buf.len() as libc::size_t)
            .try_into()
            .unwrap()
    };
    assert_eq!(res, 1);
    assert_eq!(buf, "a".as_bytes());

    unsafe { assert_eq!({ VAL }, 1) };

    thread1.join().unwrap();
}

I agree this should be UB. Good catch!

The odd thing is that this, which is almost equivalent, does report UB:

use std::sync::atomic::*;
use std::thread;

fn main() {
    static mut VAL: u8 = 0;
    static SYNC: AtomicBool = AtomicBool::new(false);

    let thread1 = thread::spawn(move || {
        SYNC.store(true, Ordering::Release);

        unsafe { VAL = 1 };
    });
    thread::yield_now();

    assert!(SYNC.load(Ordering::Acquire));

    unsafe { assert_eq!({ VAL }, 1) };

    thread1.join().unwrap();
}

[FrankReh]

Thanks. So I'm going to guess that there is an increment of the sub-thread's clock after the clock is synced. I'll use your working-properly test case and figure out the difference.

Yes, preemption is disabled for all these.

[FrankReh]

Let me point out though, without knowing why it works correctly with the AtomicBool, that by moving the clock increment to after the value that is cloned to be returned by release_clock, all the miri tests continued to pass, but also the test case I came up with passed. So if there are AtomicBool tests in the miri suite, they continue to work.

As you had seen in other threads, another hack way to make things pass was to call release_clock twice. The point was that the clock was
incremented after an earlier clock had been synced.

[FrankReh]

It's hard for me to trace the AtomicBool. I haven't worked with the intrinsics before. I don't see a release_clock being called in that area of the code.

[RalfJung]

Atomic loads and stores are handled here:

miri/src/intrinsics/atomic.rs

Lines 126 to 155 in 124c7ad

	fn atomic_load(
	&mut self,
	args: &[OpTy<'tcx>],
	dest: &MPlaceTy<'tcx>,
	atomic: AtomicReadOrd,
	) -> InterpResult<'tcx> {
	let this = self.eval_context_mut();
	let [place] = check_arg_count(args)?;
	let place = this.deref_pointer(place)?;
	// Perform atomic load.
	let val = this.read_scalar_atomic(&place, atomic)?;
	// Perform regular store.
	this.write_scalar(val, dest)?;
	interp_ok(())
	}
	fn atomic_store(&mut self, args: &[OpTy<'tcx>], atomic: AtomicWriteOrd) -> InterpResult<'tcx> {
	let this = self.eval_context_mut();
	let [place, val] = check_arg_count(args)?;
	let place = this.deref_pointer(place)?;
	// Perform regular load.
	let val = this.read_scalar(val)?;
	// Perform atomic store
	this.write_scalar_atomic(val, &place, atomic)?;
	interp_ok(())
	}

The relevant clock handling is here:

miri/src/concurrency/data_race.rs

Lines 1313 to 1355 in 124c7ad

	/// Update the data-race detector for an atomic read occurring at the
	/// associated memory-place and on the current thread.
	fn validate_atomic_load(
	&self,
	place: &MPlaceTy<'tcx>,
	atomic: AtomicReadOrd,
	) -> InterpResult<'tcx> {
	let this = self.eval_context_ref();
	this.validate_atomic_op(
	place,
	atomic,
	AccessType::AtomicLoad,
	move |memory, clocks, index, atomic| {
	if atomic == AtomicReadOrd::Relaxed {
	memory.load_relaxed(&mut *clocks, index, place.layout.size)
	} else {
	memory.load_acquire(&mut *clocks, index, place.layout.size)
	}
	},
	)
	}
	/// Update the data-race detector for an atomic write occurring at the
	/// associated memory-place and on the current thread.
	fn validate_atomic_store(
	&mut self,
	place: &MPlaceTy<'tcx>,
	atomic: AtomicWriteOrd,
	) -> InterpResult<'tcx> {
	let this = self.eval_context_mut();
	this.validate_atomic_op(
	place,
	atomic,
	AccessType::AtomicStore,
	move |memory, clocks, index, atomic| {
	if atomic == AtomicWriteOrd::Relaxed {
	memory.store_relaxed(clocks, index, place.layout.size)
	} else {
	memory.store_release(clocks, index, place.layout.size)
	}
	},
	)
	}

[FrankReh]

I don't think that level of Miri is even using release_clock. I see clocks being passed around and their clock index too, and logic that seems straight out of the paper - but I did not see where a clock is incremented - I'll look again. I think it's easy to show the same problem with eventfd, so I'm writing a test case to prove it to myself. Then we can be sure it isn't just something about unnamed pipe.

[FrankReh]

Yes, same problem with this test using eventfd. The assert against VAL_B being 11 should be caught as UB but it is not. Unless I apply my release_clock patch and then it is flagged as UB.

//@only-target: linux
// test_race depends on a deterministic schedule.
//@compile-flags: -Zmiri-preemption-rate=0

// These are artifacts from the file I copied from.
// FIXME(static_mut_refs): Do not allow `static_mut_refs` lint
#![allow(static_mut_refs)]

use std::thread;

fn main() {
    test_race();
}

fn read_bytes<const N: usize>(fd: i32, buf: &mut [u8; N]) -> i32 {
    let res: i32 = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), N).try_into().unwrap() };
    return res;
}

fn write_bytes<const N: usize>(fd: i32, data: [u8; N]) -> i32 {
    let res: i32 =
        unsafe { libc::write(fd, data.as_ptr() as *const libc::c_void, N).try_into().unwrap() };
    return res;
}

fn test_race() {
    static mut VAL_A: u8 = 0;
    static mut VAL_B: u8 = 10;
    let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC;
    let fd = unsafe { libc::eventfd(0, flags) };
    let thread1 = thread::spawn(move || {
        let mut buf: [u8; 8] = [0; 8];
        let res = read_bytes(fd, &mut buf);
        // read returns number of bytes has been read, which is always 8.
        assert_eq!(res, 8);
        let counter = u64::from_ne_bytes(buf);
        assert_eq!(counter, 1);
        unsafe { assert_eq!(VAL_A, 1) };
        unsafe { assert_eq!(VAL_B, 11) };
    });
    unsafe { VAL_A = 1 };
    let data: [u8; 8] = 1_u64.to_ne_bytes();
    let res = write_bytes(fd, data);
    // write returns number of bytes written, which is always 8.
    assert_eq!(res, 8);
    unsafe { VAL_B = 11 };
    thread::yield_now();
    thread1.join().unwrap();
}

[RalfJung]

So change the return type to an owned clock, or keep a prev_clock for just this purpose of passing a reference. But one way or the other, return the clock value before the thread's clock was incremented.

That is indeed the issue! I screwed this up in 38700bf, not realizing that I changed it from "work with clock before increment" to "work with clock after increment". oops

I'll look into fixing this.