Unsound impl Debug for <union> (in src/unix/linux_like/android/mod.rs)

[anforowicz]

Description of the soundness issue

c0b1ccf has added pub union __c_anonymous_ifr_ifru to src/unix/linux_like/android/mod.rs, together with impl ::fmt::Debug for __c_anonymous_ifr_ifru - see:

libc/src/unix/linux_like/android/mod.rs

Lines 990 to 1008 in 40741ba

	impl ::fmt::Debug for __c_anonymous_ifr_ifru {
	fn fmt(&self, f: &mut ::fmt::Formatter) -> ::fmt::Result {
	f.debug_struct("ifr_ifru")
	.field("ifru_addr", unsafe { &self.ifru_addr })
	.field("ifru_dstaddr", unsafe { &self.ifru_dstaddr })
	.field("ifru_broadaddr", unsafe { &self.ifru_broadaddr })
	.field("ifru_netmask", unsafe { &self.ifru_netmask })
	.field("ifru_hwaddr", unsafe { &self.ifru_hwaddr })
	.field("ifru_flags", unsafe { &self.ifru_flags })
	.field("ifru_ifindex", unsafe { &self.ifru_ifindex })
	.field("ifru_metric", unsafe { &self.ifru_metric })
	.field("ifru_mtu", unsafe { &self.ifru_mtu })
	.field("ifru_map", unsafe { &self.ifru_map })
	.field("ifru_slave", unsafe { &self.ifru_slave })
	.field("ifru_newname", unsafe { &self.ifru_newname })
	.field("ifru_data", unsafe { &self.ifru_data })
	.finish()
	}
	}

The implementation of Debug::fmt seems unsound - when displaying a union where only 2 bytes have been initialized (e.g. when only the c_short-sized ifru_flags field has been initialized) the fmt method will also try to access other (potentially uninitialized) bytes of a union (e.g. the ifru_newname field takes 16 bytes).

The above impl seems unsound, because using the above impl, a safe Rust code can trigger Undefined Behavior:

• MIRI flags this behavior as Undefined Behavior - see the repro steps below
• https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html says that Undefined Behavior may be exhibited by "producing an invalid value, even in private fields and locals" and says that "an integer [...] obtained from uninitialized memory" is an invalid value. (OTOH, I note that the reference also says that "the only cases in which reading uninitialized memory is permitted are inside unions" - I don't understand if this is in conflict with the other UB definitions here.)

Steps to reproduce

$ cat Cargo.toml
[package]
name = "libc-debug-impl-soundness"
version = "0.1.0"
edition = "2021"

[dependencies]
libc = { version = "0.2.152", features = ["extra_traits"] }

$ cat src/main.rs
use libc::__c_anonymous_ifr_ifru as ifru;
fn main() {
    let i = ifru { ifru_flags: 123 };
    dbg!(i);
}

$ cargo miri run
...
[src/main.rs:4:5] i = ifr_ifru {
    ifru_addr: sockaddr {
        sa_family: 123,
        sa_data: [
error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
   --> /usr/local/google/home/lukasza/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/num.rs:471:5
    |
471 | /     impl_Display!(
472 | |         i8, u8, i16, u16, i32, u32, i64, u64, usize, isize
473 | |             as u64 via to_u64 named fmt_u64
474 | |     );
    | |_____^ using uninitialized data, but this operation requires initialized memory
    |
    = 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
...

Other notes

The pattern of using unsafe to display all fields may affect unions other than just __c_anonymous_ifr_ifru. For example, the impl Debug for __c_anonymous_ifc_ifcu follows a similar pattern, although in this case both fields have the same size. OTOH I haven't reviewed other cases that can be found by the following code search: https://github.com/search?q=repo%3Arust-lang%2Flibc+%2Ffield.*unsafe.*self%2F&type=code

AFAICT all fields of the union begin at offset 0 - the union seems to be #[repr(C)] although this is not obvious because that attribute is injected by the s_no_extra_traits macro.

This issue has been found during a security audit while updating libc to a newer version - see https://chromium-review.googlesource.com/c/chromium/src/+/5178771/comment/39aaef1c_033c399c/

[JohnTitor]

We track this topic in #3248, closing in favor of that. Thank you for reporting anyway!