rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.19k stars 12.56k forks source link

Delegation incorrectly assumes `VaListImpl<'_>` from `...` will correctly pass-through to `...` #127443

Open workingjubilee opened 2 months ago

workingjubilee commented 2 months ago

To quote @petrochenkov in https://github.com/rust-lang/rust/pull/127401#issuecomment-2211775528

Delegation is supposed to copy the wrapper function signature from the target function, and then just pass all the parameters to the target function in the body. I assume that applies to VaListImpl or whatever is hiding under the ... in C-like variadics as well. If passing ... through requires some additional actions, the delegation should do those actions (or report an error), but now it's not doing them. That said, Rust should make passing ... through "just work" if it's technically possible, even if C requires some additional dance for that.

Unfortunately, ... and va_list are different types for good reason: one is arguments that have not been "discovered" yet in the function, whereas va_list is a structure that describes how to acquire those arguments, and may be acquired by rolling some or all of those arguments into the structure at the moment of its creation.

This is why, in C, there are both of these functions.

int printf ( const char * format, ... );
int vprintf ( const char * format, va_list arg );

They are not duplicates of each other, and attempting to treat them as such is a common source of UB in C programs written by people foolish enough to meddle with variadic functions in C. ( So, most of them, considering the existence of printf. )

When we receive arguments into ... using feature(c_variadic), Rust currently implicitly creates a VaListImpl<'_>, which approximately equates to C's va_list. We do this implicitly rather than requiring someone to call va_start. We are not guaranteed to know, as far as I am aware, how many arguments were passed or their shape. Some ABIs pass the necessary knowledge as a hidden parameter. But others don't. If we could control the ABI, of course, we would have the ability to do so every time, but this feature is about interfacing with C ABIs.

This interaction of features is thus currently incorrect and is undefined behavior even if a function with ... arguments is correctly called by Rust, as long as it passes through a layer of delegation.

What @petrochenkov describes is desirable, but I expect the answer will be "technically impossible, even over a reasonable subset of our supported platforms". I will do the necessary research either way. But until we confirm this, or apply the necessary changes to support delegating unsafe extern "C" fn(...), or redesign feature(c_variadic), whatever it would take to fix this, the following code must be rejected by the compiler:

//@ edition:2018
//@ aux-crate:fn_header_aux=fn-header-aux.rs

#![feature(c_variadic)]
#![feature(fn_delegation)]
#![allow(incomplete_features)]

mod to_reuse {
    pub unsafe extern "C" fn variadic_fn(n: usize, mut args: ...) {}
}

reuse to_reuse::variadic_fn;
reuse fn_header_aux::variadic_fn_extern;

fn main() {
    unsafe {
        variadic_fn(0);
        variadic_fn(0, 1);
        variadic_fn_extern(0);
        variadic_fn_extern(0, 1);
    }
    let _: unsafe extern "C" fn(usize, ...) = variadic_fn;
    let _: unsafe extern "C" fn(usize, ...) = variadic_fn_extern;
}

This bug is currently live in the most recent Rust nightlies.

rustc --version --verbose:

rustc 1.81.0-nightly (524d806c6 2024-07-05)
binary: rustc
commit-hash: 524d806c62a82ecc0cf8634b94997ae506f4d6f9
commit-date: 2024-07-05
host: x86_64-unknown-linux-gnu
release: 1.81.0-nightly
LLVM version: 18.1.7
workingjubilee commented 2 months ago

If we wish to support delegating to an unsafe extern "C" fn delegatee(...), then it would be simplest if, instead of entering a function call and then making another function call immediately, the function call in question actually resolves to calling the function delegatee during code generation. I think.