False positive when warning about use of uninit plain bytes in std::sync::CondVar::new()

[5225225]

Reproduction code:

fn main() {
    let _ = std::sync::Condvar::new();
}

Run with MIRIFLAGS="-Zmiri-check-number-validity" cargo miri run

Error:

error: Undefined Behavior: type validation failed: encountered uninitialized bytes, but expected initialized plain (non-pointer) bytes
  --> /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/condvar.rs:64:17
   |
64 |         let r = libc::pthread_condattr_destroy(attr.as_mut_ptr());
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, but expected initialized plain (non-pointer) bytes
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
           
   = note: inside `std::sys::unix::condvar::Condvar::init` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/condvar.rs:64:17
   = note: inside `std::sys_common::condvar::Condvar::new` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/condvar.rs:20:18
   = note: inside `std::sync::Condvar::new` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sync/condvar.rs:127:26
note: inside `main` at src/main.rs:2:13
  --> src/main.rs:2:13
   |
2  |     let _ = std::sync::Condvar::new();
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:123:18
   = note: inside closure at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:145:18
   = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:259:13
   = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:406:40
   = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:370:19
   = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:133:14
   = note: inside closure at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:128:48
   = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:406:40
   = note: inside `std::panicking::r#try::<isize, [closure@std::rt::lang_start_internal::{closure#2}]>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:370:19
   = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:133:14
   = note: inside `std::rt::lang_start_internal` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:128:20
   = note: inside `std::rt::lang_start::<()>` at /home/jess/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:144:17

The relevant bit of stdlib code seems to be

pub unsafe fn init(&mut self) {
    use crate::mem::MaybeUninit;
    let mut attr = MaybeUninit::<libc::pthread_condattr_t>::uninit();
    let r = libc::pthread_condattr_init(attr.as_mut_ptr());
    assert_eq!(r, 0);
    let r = libc::pthread_condattr_setclock(attr.as_mut_ptr(), libc::CLOCK_MONOTONIC);
    assert_eq!(r, 0);
    let r = libc::pthread_cond_init(self.inner.get(), attr.as_ptr());
    assert_eq!(r, 0);
    let r = libc::pthread_condattr_destroy(attr.as_mut_ptr());
    assert_eq!(r, 0);
}

where miri doesn't see that pthread_condattr_init(attr.as_mut_ptr()) makes pthread_condattr_destroy(attr.as_mut_ptr()) not UB.

[5225225]

Relevant bit of code seems to be

miri/src/shims/posix/foreign_items.rs

Lines 334 to 343 in adc26a3

	"pthread_condattr_init" => {
	let &[ref attr] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
	let result = this.pthread_condattr_init(attr)?;
	this.write_scalar(Scalar::from_i32(result), dest)?;
	}
	"pthread_condattr_destroy" => {
	let &[ref attr] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
	let result = this.pthread_condattr_destroy(attr)?;
	this.write_scalar(Scalar::from_i32(result), dest)?;
	}

Looks fairly simple to implement, so I'll claim this.

[5225225]

The man page says

The pthread_condattr_destroy() function shall destroy a condition variable attributes object; the object becomes, in effect, uninitialized.

Does this mean we should write uninit to the target of the pointer? (To protect against double destroy).

That, and double init is... probably not okay? It says "Results are undefined if pthread_condattr_init() is called specifying an already initialized attr attributes object", but I'm not sure if I should take that to mean "it is UB to do that".

[oli-obk]

Does this mean we should write uninit to the target of the pointer?

Technically yes, but we aren't as thorough with this as we should be. Technically a mir::Operand::Move should also uninitialize the moved from place, but we don't.

If it's not too much trouble and since the docs say so, go right ahead and overwrite with uninit

[RalfJung]

Good catch! This regression was introduced with #1904, looks like our test suite does not cover enough of the destroy functions.

Thanks for the report and fix. ❤️