Various soundness bugs related to `MaybeUninit::uninit`, partial initialization, then `.assume_init()`

[tgross35]

This is similar to what is reported in #2693, but more general.

• nix/src/mqueue.rs

  Lines 111 to 121 in ba63e90

  	let mut attr = mem::MaybeUninit::<libc::mq_attr>::uninit();
  	unsafe {
  	let p = attr.as_mut_ptr();
  	(*p).mq_flags = mq_flags;
  	(*p).mq_maxmsg = mq_maxmsg;
  	(*p).mq_msgsize = mq_msgsize;
  	(*p).mq_curmsgs = mq_curmsgs;
  	MqAttr {
  	mq_attr: attr.assume_init(),
  	}
  	}

  : mq_attr has a private field so the struct cannot possibly be initialized.
• https://github.com/nix-rust/nix/blob/ba63e90362e24dcebbf96783da668647ceace21c/src/sys/signalfd.rs#L115C51-L127: signalfd_siginfo has padding fields

• nix/src/spawn.rs

  Lines 25 to 31 in ba63e90

  	let mut attr = mem::MaybeUninit::uninit();
  	let res = unsafe { libc::posix_spawnattr_init(attr.as_mut_ptr()) };
  	Errno::result(res)?;
  	let attr = unsafe { attr.assume_init() };
  	Ok(PosixSpawnAttr { attr })

  and other places in that file: posix_spawnattr_t has padding fields

• nix/src/sys/select.rs

  Line 33 in ba63e90

  let mut fdset = mem::MaybeUninit::uninit();

  : the contents of fdset are not public API and may contain padding fields

• nix/src/sys/stat.rs

  Line 215 in ba63e90

  let mut dst = mem::MaybeUninit::uninit();

  : stat contains padding

The first example is the most problematic, that UB caused a (fairly innocuous) issue in Fedora's CI rust-lang/libc#4892 (comment). This happened to be fixed in the latest libc, but that was due to a change in internal details that shouldn't be relied upon.

In general, it's really not ever robust to use MaybeUninit::uninit with libc types if .assume_init() will be called. For anything that has padding fields now or may add fields in the future, it's easy to wind up in a situation where a system libc/kernel is only partially initializing the type and assume_init creates UB. I doubt the cost of zeroing will be noticeable when there is usually a syscall overhead.

(It's fine to leave the type wrapped in MaybeUninit and only access useful fields via raw pointers, same as in C, but that's not ergonomic.)

[xtqqczze]

@tgross35 Why do you think the following is unsound?

nix/src/sys/select.rs

Lines 33 to 40 in ba63e90

	let mut fdset = mem::MaybeUninit::uninit();
	unsafe {
	libc::FD_ZERO(fdset.as_mut_ptr());
	Self {
	set: fdset.assume_init(),
	_fd: std::marker::PhantomData,
	}
	}

[tgross35]

I just missed the FD_ZERO call; you are right, that one should be fine.

[xtqqczze]

I’m not an expert on Rust soundness, but most of the examples given for types “containing padding fields” look reasonable; the only one that seems questionable is the siginfo case:

nix/src/sys/signalfd.rs

Lines 115 to 127 in ba63e90

	let mut buffer = mem::MaybeUninit::<siginfo>::uninit();
	let size = mem::size_of_val(&buffer);
	let res = Errno::result(unsafe {
	libc::read(self.0.as_raw_fd(), buffer.as_mut_ptr().cast(), size)
	})
	.map(|r| r as usize);
	match res {
	Ok(x) if x == size => Ok(Some(unsafe { buffer.assume_init() })),
	Ok(_) => unreachable!("partial read on signalfd"),
	Err(Errno::EAGAIN) => Ok(None),
	Err(error) => Err(error),
	}

.

[tgross35]

Good point, that one looks good as well. signalfd_siginfo does have padding fields, but read must be writing them so it isn't a problem.

[xtqqczze]

I think this is the only one from the description that is unsound:

nix/src/mqueue.rs

Lines 111 to 121 in ba63e90

	let mut attr = mem::MaybeUninit::<libc::mq_attr>::uninit();
	unsafe {
	let p = attr.as_mut_ptr();
	(*p).mq_flags = mq_flags;
	(*p).mq_maxmsg = mq_maxmsg;
	(*p).mq_msgsize = mq_msgsize;
	(*p).mq_curmsgs = mq_curmsgs;
	MqAttr {
	mq_attr: attr.assume_init(),
	}
	}

[tgross35]

Functions related to posix_spawnattr_t are as well I believe; that type has padding, and I don't believe we can rely on posix_spawnattr_init to initialize the padding fields.

[xtqqczze]

Functions related to posix_spawnattr_t are as well I believe; that type has padding, and I don't believe we can rely on posix_spawnattr_init to initialize the padding fields.

We need libc 0.2.178 to fix this UB, see rust-lang/libc#4609. Currently libc is pinned at 0.2.175.

[tgross35]

You can fix it now by using MaybeUninit::zeroed() rather than MaybeUninit::uninit.

That is my original point: these are bugs in this library, not bugs in libc. libc is doing some things to smooth this out for the future, but we can't cover every case. Even with libc switching to padding there typically isn't any guarantee that a libcall initializes all fields, so it could still be wrong unless you use zeroed.

[xtqqczze]

We need libc 0.2.178 to fix this UB, see rust-lang/libc#4609. Currently libc is pinned at 0.2.175.

@tgross35 oh, I see you reviewed and merged that PR 😀

[xtqqczze]

Functions related to posix_spawnattr_t are as well I believe; that type has padding, and I don't believe we can rely on posix_spawnattr_init to initialize the padding fields.

Actually, libc was bumped to 0.2.180 very recently (#2724), so initializing posix_spawnattr_t with posix_spawnattr_init is now sound.

[tgross35]

My above point still stands: these are very internal things, there isn't any guarantee that posix_spawnattr_init is actually initializing all fields. It probably is, but why risk it? MaybeUninit::zeroed completely avoids this class of potential bugs.

[xtqqczze]

@tgross35 Initializing all fields with the default value is part of the contract for posix_spawnattr_init, as per POSIX.1.

[tgross35]

"The posix_spawnattr_init() function shall initialize a spawn attributes object attr with the default value for all of the individual attributes used by the implementation." is talking about attributes, not fields. There can be unused fields that don't get initialized.

Again - is it really easier to do all this low level digging across all platforms rather than using zeroed and being absolutely certain? I don't understand the motivation to stick with a known footgun.

[xtqqczze]

MaybeUninit::zeroed completely avoids this class of potential bugs.

MaybeUninit::zeroed does not eliminate this class of bugs. There’s no guarantee that an all-zero byte pattern represents a valid value of some type T, and its contract does not guarantee that padding bytes will be zeroed.