Stop using mem::zeroed for FFI

[joboet]

Quite a lot of stds FFI code uses mem::zeroed to create empty structures that are to be filled by FFI. E.g.:

rust/library/std/src/sys/pal/windows/time.rs

Lines 68 to 72 in d2f335d

	unsafe {
	let mut t: SystemTime = mem::zeroed();
	c::GetSystemTimePreciseAsFileTime(&mut t.t);
	t
	}

This is unnecessary, since the C code does not require the structures to be initialized (you wouldn't zero out structures in C either). Thus, this pattern just reduces performance, as it results in the initialization of potentially very large structures such as sockaddr_storage. We should get rid of this pattern and replace it with proper handling of uninitialized data through MaybeUninit.

Edit (after discussion below): In some instances one might decide to keep the zero-initialization behaviour, but I think this should still go through MaybeUninit::zeroed instead of mem::zeroed to make the point of initialization explicit (by introducing .assume_init() calls in the right places.

I'll probably do the network code myself (I want to clean some things up there anyway), but I'm happy to mentor you if you'd like to help with other instances such as the filesystem code (library/std/src/sys/pal/*/fs.rs). Just contact me here or on Zulip. The best way to find the pattern is probably by searching for mem::zeroed in library/std/src/sys.

[ChrisDenton]

I don't think GetSystemTimePreciseAsFileTime is the best example. FILETIME is a couple of u32s (aka a 4-byte aligned u64). You wouldn't usually bother with MaybeUninit for a u64.

But for bigger structs I do agree.

[the8472]

Afaik in the linux kernel zero-initialization for structs is the default as a security measure. And some APIs explicitly require it, e.g. cmsghdr.

[joboet]

I don't think GetSystemTimePreciseAsFileTime is the best example. FILETIME is a couple of u32s (aka a 4-byte aligned u64). You wouldn't usually bother with MaybeUninit for a u64.

But for bigger structs I do agree.

Fair point. I just wanted a very clear example of the pattern.

Edit: Also, I think using MaybeUninit even here is very much justified as it explicitly annotates when the structure has been filled with useful data.

[joboet]

Afaik in the linux kernel zero-initialization for structs is the default as a security measure.

I don't think this is necessary in our case. For one, there is an FFI and linkage unit boundary in our case, meaning the optimizer can't rely introduce any optimizations that rely on the initialization state of the structure. Also, I guess Linux probably does this to make sure that sensitive data is not accidentally leaked when copying out kernel memory to userspace. This isn't relevant for us as we assume the environment to be trusted.

And some APIs explicitly require it, e.g. cmsghdr.

Obviously we should evaluate the correctness individually for each instance of the pattern. But for e.g. pthread_t, I highly doubt that anyone in C-land would zero-initialize it.

[Noratrieb]

I think it makes sense to do for potentially big structs, but I think it should be carefully evaluated on a case-by-case basis, as using MaybeUninit makes the code more complicated.

[hanna-kruppe]

For one, there is an FFI and linkage unit boundary in our case, meaning the optimizer can't rely introduce any optimizations that rely on the initialization state of the structure.

This kind of argument worries me. C programmers have been proven wrong about it many times as LTO has become more commonplace. It’s generally true today with libcs written in C and compiled to machine code before Rust even enters the picture, but this can and quite possibly should change. There’s cross-language LTO, LLVM libc has the explicit goal of supporting LTO of libc code + application code, and there are even (highly experimental) projects implementing libc interfaces in Rust.

[the8472]

And the wobblyness of an uninit struct backed by MADV_FREE'd memory would leak through an FFI barrier. I.e. *foo == *foo wouldn't hold. Of course the C code shouldn't do that without initializing the data in the first place and the UB would be clearly C's fault, but it's additional evidence that the optimization barrier argument is insufficient.

[joboet]

I think it makes sense to do for potentially big structs, but I think it should be carefully evaluated on a case-by-case basis, as using MaybeUninit makes the code more complicated.

I'd argue that the additional complexity is a good thing: Having to write .assume_init() makes you think twice about whether you've actually filled in the data. So even if we want to keep zero-initialization in some cases, I'd argue it still should be done with MaybeUninit::zeroed instead of mem::zeroed.

Of course the C code shouldn't do that without initializing the data in the first place and the UB would be clearly C's fault

Exactly. The question is how much we trust the C code to be correct. I'd argue that in nearly all of the cases inside std, misbehaving C code is both highly unlikely (because these are APIs used by everyone, e.g. GetSystemInfo on Windows) and would be unsound anyway (e.g. in the case of GetSystemInfo because the result is used with NonZero::new_unchecked, so if the structure is not filled, even the zero-initialization would lead to undefined behaviour).

[the8472]

because the result is used with NonZero::new_unchecked

This actually bit us on linux #115868. The API guarantees that it's non-zero but older kernels had a bug where it failed to uphold that guarantee. On the C side that's probably just a correctness issue, not UB. We elevated it UB by trusting the API and feeding it into nonzero.

The question is how much we trust the C code to be correct.

Not just the code but also the API. When talking about OS APIs we play a game of telephone from the internal interfaces to the public API to the libc/windows-sys translations.

Having to write .assume_init() makes you think twice about whether you've actually filled in the data. So even if we want to keep zero-initialization in some cases, I'd argue it still should be done with MaybeUninit::zeroed instead of mem::zeroed.

On the OS side it's usually not UB though afaik. Passing a zero-initialized struct to an OS api will just result in an error return, not UB because they don't have any non-zero types and check the pointers because they don't trust userspace.

So zeroed() is kind of like calling new(). You get a valid but perhaps useless struct.

[ChrisDenton]

We do have to trust the C API to some extent. The number of unsound things that it can do (or provoke us to do) is infinite but even if you restrict it to the set of things that can be done by accident then it's still a large surface area that we don't want to be defensively programming against. For example, writing to memory that's supposed to only be read can happen purely by accident. Or scribbling over our stack due to an offset bug. We can react to known bugs but we can't reasonably defend against everything.

That's not to say we should 100% trust the C API but there is a trade-off and we do have to draw a line somewhere.

[the8472]

I'm not saying we must zero everything. Imo it's just a conservative choice that might lessen the impact of some subset of bugs. So I don't like framing this as "stop using mem::zeroed".
If there are cases where there are perf benefits, sure, fine. If it's just some tiny struct that's dwarfed by the FFI or syscall overhead then I don't see the benefit.

[joboet]

I'm not saying we must zero everything. Imo it's just a conservative choice that might lessen the impact of some subset of bugs. So I don't like framing this as "stop using mem::zeroed".
If there are cases where there are perf benefits, sure, fine. If it's just some tiny struct that's dwarfed by the FFI or syscall overhead then I don't see the benefit.

Yes, fair enough.

On the OS side it's usually not UB though afaik. Passing a zero-initialized struct to an OS api will just result in an error return, not UB because they don't have any non-zero types and check the pointers because they don't trust userspace.

So zeroed() is kind of like calling new(). You get a valid but perhaps useless struct.

This isn't about undefined behaviour, but rather about easier correctness. As the system APIs don't return Result and aren't annotated with #[must_use], it's quite easy to forget to check results. Having to write .assume_init() means you have to think (and hopefully write a comment) about why the structure must have been filled at that point. So I think replacing mem::zeroed with MaybeUninit is worthwhile in general. We can obviously debate whether MaybeUninit::zeroed or MaybeUninit::uninit should be used for each instance individually. I've updated the issue to that effect.

[the8472]

Using an unsafe wrapper for something that's not a safety issue seems misguided and an unnecessary burden on those who review the code.

[xizheyin]

Hi. Is there any prerequisite knowledge or background required for me to try my hand at this issue? This is my first attempt at this project. What do I need to do besides checking out the development guide?

[joboet]

Not much, really, besides checking your code for the relevant targets with ./x check --target ... (e.g. x86_64-unknown-linux-gnu or x86_64-pc-windows-msvc) and formatting your code with ./x fmt before every commit. I guess searching for the man-page of the relevant C functions to see whether they require the data to be initialized would be a good idea as well.

As for which instance of the pattern to address, I'd recommend cases where either sockaddr_storage or pthread_attr_t is used – as you can see from the discussion above, there is not that much consensus within the library team on whether to make this change for smaller structures. These two should however be uncontroversial.

[xizheyin]

@rustbot claim

[xizheyin]

Hey. I'm not sure I found your id correctly inside zulip, so I came here to ask a question.
As far as I understand it, it's finding where a relatively large data structure like sockaddr_storage or pthread_attr_t is used in library/std/src/sys and replacing mem::zeroed with MaybeUninit::zeroed. is that right?
For example,

let mut uninit: MaybeUninit<u32> = MaybeUninit::uninit();
// first init
unsafe {
    std::ptr::write(uninit.as_mut_ptr(), 42);
}
// now call assume_init
let value = unsafe { uninit.assume_init() };

And I found these uses for mem::zeroed():

xxx@pc:~/rust$ grep -r "mem::zeroed()" /home/xxx/rust/library/std/src/sys
/home/xxx/rust/library/std/src/sys/net/connection/socket.rs:        let mut option_value: T = mem::zeroed();
/home/xxx/rust/library/std/src/sys/net/connection/socket.rs:        let mut storage: c::sockaddr_storage = mem::zeroed();
/home/xxx/rust/library/std/src/sys/net/connection/socket.rs:            let mut hints: c::addrinfo = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/net/connection/socket.rs:        let mut storage: c::sockaddr_storage = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/net/connection/socket/unix.rs:        let mut storage: libc::sockaddr_storage = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/net/connection/socket/unix.rs:            let mut arg: libc::accept_filter_arg = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/net/connection/socket/solid.rs:        let mut storage: netc::sockaddr_storage = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/net/connection/socket/wasip2.rs:        let mut storage: netc::sockaddr_storage = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/net/connection/socket/hermit.rs:        let mut storage: netc::sockaddr_storage = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/net/connection/socket/windows.rs:        let mut data: c::WSADATA = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/hermit/fs.rs:        let mut stat_val: stat_struct = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/hermit/fs.rs:        let mut stat_val: stat_struct = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/hermit/fs.rs:        let mut stat_val: stat_struct = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/teeos/thread.rs:        let mut native: libc::pthread_t = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/teeos/thread.rs:        let mut attr: libc::pthread_attr_t = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/linux/pidfd.rs:        let mut siginfo: libc::siginfo_t = unsafe { crate::mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/linux/pidfd.rs:        let mut siginfo: libc::siginfo_t = unsafe { crate::mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/process/process_unix.rs:                    let mut action: libc::sigaction = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/process/process_unix.rs:            let mut cmsg: Cmsg = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/process/process_unix.rs:            let mut msg: libc::msghdr = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/process/process_unix.rs:            let mut cmsg: Cmsg = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/process/process_unix.rs:            let mut msg: libc::msghdr = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/fs.rs:        let mut buf: libc::statx = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/fs.rs:        let mut stat: stat64 = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/fs.rs:            let mut ret = DirEntry { entry: mem::zeroed(), dir: Arc::clone(&self.inner) };
/home/xxx/rust/library/std/src/sys/pal/unix/fs.rs:        let mut stat: stat64 = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/fs.rs:        let mut stat: stat64 = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/fs.rs:                let mut attrlist: libc::attrlist = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/fs.rs:        let mut stat: stat64 = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/fs.rs:        let mut stat: stat64 = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/stack_overflow.rs:            let mut action: sigaction = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/stack_overflow.rs:        let mut action: sigaction = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/stack_overflow.rs:        let mut stack = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/stack_overflow.rs:        let mut current_stack: libc::stack_t = crate::mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/stack_overflow.rs:        let mut current_stack: libc::stack_t = crate::mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/stack_overflow.rs:        let mut attr: libc::pthread_attr_t = crate::mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/stack_overflow.rs:        let mut attr: libc::pthread_attr_t = crate::mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/thread.rs:        let mut native: libc::pthread_t = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/thread.rs:        let mut attr: libc::pthread_attr_t = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/thread.rs:                let mut set: libc::cpu_set_t = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/thread.rs:                let mut set: libc::cpuset_t = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/unix/thread.rs:                let mut sinfo: libc::system_info = crate::mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/unix/pipe.rs:    let mut fds: [libc::pollfd; 2] = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/wasi/thread.rs:                let mut native: libc::pthread_t = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/wasi/thread.rs:                let mut attr: libc::pthread_attr_t = unsafe { mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/wasi/thread.rs:                let mut event: wasi::Event = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/time.rs:            let mut t: SystemTime = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:            let mut wfd = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:            let mut overlapped: c::OVERLAPPED = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:            let mut overlapped = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:            let mut overlapped = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:            let mut info: c::BY_HANDLE_FILE_INFORMATION = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:                let mut attr_tag: c::FILE_ATTRIBUTE_TAG_INFO = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:            let mut info: c::FILE_BASIC_INFO = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:            let mut info: c::FILE_STANDARD_INFO = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:                let mut attr_tag: c::FILE_ATTRIBUTE_TAG_INFO = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:            let mut info: c::FILE_BASIC_INFO = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:        let mut wfd: c::WIN32_FIND_DATAW = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/fs.rs:                let mut wfd: c::WIN32_FIND_DATAW = mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/thread.rs:        let mut sysinfo: c::SYSTEM_INFO = crate::mem::zeroed();
/home/xxx/rust/library/std/src/sys/pal/windows/pipe.rs:        let mut overlapped: c::OVERLAPPED = unsafe { crate::mem::zeroed() };
/home/xxx/rust/library/std/src/sys/pal/windows/pipe.rs:        let mut overlapped: Box<c::OVERLAPPED> = unsafe { Box::new(mem::zeroed()) };
/home/xxx/rust/library/std/src/sys/pal/windows/pipe.rs:            let overlapped = Box::new(unsafe { mem::zeroed() });

[xizheyin]

MaybeUninit::zeroed seems to have the same performance overhead as mem::zeroed, should I use MaybeUminit::uinit instead of MaybeUninit::zeroed? @joboet

[slanterns]

If I read it correctly:

• For sockaddr_storage and pthread_attr_t, replace zeroing by MaybeUninit::uninit to reduce the performance overhead;
• For cases other than above-mentioned ones, we may also replace it by MaybeUninit::zeroed later for clearer semantic.

[joboet]

If I read it correctly:

* For `sockaddr_storage` and `pthread_attr_t`, replace zeroing by `MaybeUninit::uninit` to reduce the performance overhead;

* For cases other than above-mentioned ones, we may also replace it by `MaybeUninit::zeroed` later for clearer semantic.

Yes, that's right.

[safinaskar]

This actually bit us on linux #115868. The API guarantees that it's non-zero but older kernels had a bug where it failed to uphold that guarantee. On the C side that's probably just a correctness issue, not UB. We elevated it UB by trusting the API and feeding it into nonzero.

We should never assume that kernel/OS has any "common sense" behavior. And of course, we should not base our safety guarantees on OS correctness. For example, let's consider Linux's getcwd syscall. This syscall returns absolute path to current working directory. So, result is always starts with /. Right? Right?! No! If your current working directory is located on some mount, which is lazy unmounted and thus absent from current mount namespace, then getcwd will start with (unreachable) (or something like that) instead of /. As well as I understand, this is not documented at all. And Linux is full of such things. So: assume any kind of weirdness from OS interfaces.

Update 2025-02-14: this affects raw Linux syscall, but not glibc's getcwd wrapper. And yes, this is documented in getcwd manpage

[safinaskar]

Please, close this issue without changing anything!

(Text below will discuss both Linux and Windows syscalls.)

For many interfaces it is recommended or even required to zero structures before using. For example, let's talk about Linux's openat2. Manpage for openat2 ( https://manpages.debian.org/unstable/manpages-dev/openat2.2.en.html ) explicitly says:

Any future extensions to openat2() will be implemented as new fields appended to the open_how structure, with a zero value in a new field resulting in the kernel behaving as though that extension field was not present. Therefore, the caller must zero-fill this structure on initialization. (See the "Extensibility" section of the NOTES for more detail on why this is necessary.)

Moreover, zero-initializing structs is so easy and natural in C, that documentation writers sometimes do not even bother explicitly saying you that you should zero-initialize everything. For example, here is article Brauner wrote about newly introduced Linux syscalls: https://brauner.io/2024/12/16/list-all-mounts.html . As well as I understand, Brauner also is author of actual implementations of these syscalls, so he definitely knows what he writing. Notice this line in his example code: struct mnt_ns_info info = {};. This is how zero-initialization looks like in modern C code. (Contrast this with scary-looking unsafe { zeroed() }.)

Okay, why Brauner wrote this? Simply out of habit? Or because this is actually required? I don't know. Even if this is requirement, I think it is very easy for Brauner to forget to say this explicitly. Because zero-initializing is so common in C that people usually don't mention explicitly, that it is required.

Now let's talk about Windows. Here is Windows tutorial: https://learn.microsoft.com/en-us/windows/win32/learnwin32/creating-a-window . Notice line WNDCLASS wc = { };. This is zero-initializing. Is it required? I don't know. Zero-initializing is so natural in C (well, okay, C++ in this case), that author doesn't bother explaining it.

In WinAPI people usually zero-initialize everything. Unfortunately I cannot find a link, which will explain this. But I remember that in good old Delphi days it was commonplace to zero-initialize everything when you call WinAPI. Delphi used Pascal as its language. And yes, zero-initializing was totally unidiomatic and wordy in Pascal, similarly to how zeroed() is unidiomatic, scary, unsafe and wordy in Rust. Nonetheless, everybody zero-initialized everything. I don't know whether this was requirement. But this was common practice.

Okay, so. Zero-initializing is good default. Sometimes it is requirement. And if you don't know whether it is required, then, please, zero-initialize. It is safer option. This is harmless. This is faster than syscall overhead.

Mass-replacing zeroed with other things can introduce bugs. Please, don't do this. Just close this issue without action.

Even if you really want to replace something, then this should be some function, which doesn't do any syscall (and thus you can actually save some time by replacing zeroed with uninitialized) and you must be totally sure that your edit is correct.

So, again: just close this. If you don't want to close, then at least mark the issue as "hard", not as "easy". (Note: "E-hard" description doesn't say "rustc contributing experience", it just says "experience", so I assume general programming experience. This issue definitely needs a lot of system programming experience.)

@rustbot label: -E-easy

@rustbot label -E-easy

@rustbot label: +E-hard

(rustbot often ignores me, I don't know whether these commands will work.)

[joboet]

I'm not advocating to change cases where structs are filled by std, that's obviously a bad idea. I meant cases like pthread_attr_t where an opaque structure is initialized by calling pthread_attr_init – C really can't rely on the attr being initialized because it allows re-initializing thread attributes that were previously destroyed (hence the previous data would still be visible). But it's apparent that the scope of this issue is too large and that it's misleading, so I'm closing it. I still think #136826 is worthwhile – if you think that that too is too risky, please leave a comment there.