Varargs are completely unchecked if passed as generics

[joshuabogue]

I am trying to call the function DftiSetValue from the Intel Math Library. The function has the layout MKL_LONG DftiSetValue(DFTI_DESCRIPTOR_HANDLE, enum DFTI_CONFIG_PARAM, ...), the first is an 8 byte pointer (windows 64 bit), the second is an enum which is 4 bytes long, and the last parameter is a floating point number.

I can call this function from c++ just fine as a float or double, but cannot call this from rust without making it a double. This is with the enum being treated as 32 bit int, and I get an error if I try to call it with a u4 from the ux package. According to the documentation this function should be a float because I am using floating point fft precision.

I have attached some code that shows the fft being called with some sample values and you can see that one time it is called and returns all zeros which is incorrect and the second time it is called it comes back correctly with non-zero values.

Example.zip

[Mark-Simulacrum]

cc @dlrobertson @eddyb

[eddyb]

According to the documentation this function should be a float because I am using floating point fft precision.

float is automatically promoted to double when passed via varargs, AFAIK.
I forget if we do this automatically for you.

[dlrobertson]

From the C Standard.

The type of an argument in a variable argument list will never be an integer type smaller than int, nor will it ever be float.

And indeed if you were to attempt to use va_arg with a float with gcc you would get something like the following.

$ gcc -std=c11 -o ./test test.c
In file included from test.c:2:
test.c: In function ‘add’:
test.c:10:25: warning: ‘float’ is promoted to ‘double’ when passed through ‘...’
   10 |         f += va_arg(ap, float);
      |                         ^
test.c:10:25: note: (so you should pass ‘double’ not ‘float’ to ‘va_arg’)
test.c:10:25: note: if this code is reached, the program will abort
$ ./test
Illegal instruction

It would be trivial to automatically promote floats to double like we do for integers smaller than an int. I'd rather make programmers explicitly change their use of VaList::arg instead of automatically promoting, but this has caused some confusion, so it might be worth adding.

[Enselic]

Triage: Would be great if someone could produce a minimized reproducer of this problem. Right now the .zip contains almost 600 lines of Rust code.

[workingjubilee]

I see the problem.

When the type is an instantiated one, we do check to see if it is "C-variadic safe", and demand people correctly pass the argument, casting if necessary. The problem is that when the type is still generic, we can't check that until it is monomorphized.

[workingjubilee]

This code should not compile:

//@ compile-flags: --edition 2021
use core::ffi;

extern "C" {
    fn printf(ptr: *const ffi::c_char, ...) -> ffi::c_int;
}

fn generic_printf<T>(c: &ffi::CStr, arg: T) {
    unsafe { printf(c.as_ptr(), arg) };
}

fn main() {
    generic_printf(c"%d", 2u8);
    generic_printf(c"%f", 3.333_f32);
    generic_printf(c"%s", vec![6, 2, 8, 3, 1, 8, 5, 3, 0]);
}

[workingjubilee]

@apiraino This is a compiler bug.

[workingjubilee]

This is a miscompilation.

[workingjubilee]

There is no actual stdlib API involved, and there's barely anything for T-lang to decide aside from possibly some details on how we mitigate the fallout. This is T-compiler's to fix.

[workingjubilee]

What C does

• C does not support passing the following primitive argument types in variable arguments:
  • bool, c_char, c_uchar, c_schar
  • c_short, c_ushort
  • c_float
• unsupported types may be passed as source arguments, but they are in reality coerced via the default argument promotions up to c_int or c_double
  • c_int: bool, c_char, c_schar, c_uchar
  • c_double: c_float
• c_long, size_t, and other bigger Standard C integers work like you expect
• it is absolutely, positively, UB to then do va_arg¹ on the other side trying to obtain any of these unsupported types, because what was actually passed were the promoted types².
• as a reminder, C enums are just integers, so they get promoted as integers do
• C also supports passing these argument types through variable arguments³:
  • structs
  • data pointers

What rustc doesn't do

• because the types in this program are genericized, they seem to elude MissingCastForVariadicArg, which attempts to catch them, as discovering the instantiated types must wait until monomorphization
• it seems we don't expect these types to make it to the backend, so we may or may not handle the argument promotions correctly⁴.
• Clang performs these translations in its front end (because this is not really about ABI, it's C source code semantics), making the integers become i32 (at least, on a target that has i32 for c_int), and converting the float to a double, so LLVM probably cannot be expected to correctly handle this case for any unsupported types.
• see this Godbolt for comparison of effects, the main result that differs in visible consequence is the float, all the other types seem to have interestingly "normal" effects, but it would not surprise me if these were all de facto UB in LLVMIR and we are mostly getting lucky to only miscompile one of these types. https://rust.godbolt.org/z/5oPsxzvrd

My opinion

These programs seem to have no specified behavior in Rust. Rust does not promote the arguments via default argument promotions because there is no such notion in the language⁵. This is not part of the ABI spec, in my opinion, but a detail of C source semantics. This, however, violates the C Standard's intent and implementation, which are the only meaningful spec we have for C's varargs.

These calls thus seem to be UB, but within unsafe, thus it is technically permissible. But we lower to LLVMIR that has no clear semantics, either. Combined with the fact that this UB is statically detectable, I believe it is incorrect to compile a program with calls to C varargs with unsupported types.

Afterword

Interestingly, we do not lint on this program that passes repr(Rust) types to C varargs⁶:

use std::ffi::{c_char, c_int};
extern "C" {
    fn printf(c: *const c_char, ...) -> c_int;
}

fn main() {
    let allocated_cstring = c", but it is still UB\n".to_owned();
    unsafe { printf(c"this kinda makes sense%s".as_ptr(), allocated_cstring) };
    
    let allocated_repr_rust = vec!["lol what"];
    unsafe { printf(c"but this doesn't: %s".as_ptr(), allocated_repr_rust) };
}

Footnotes

1. how C programs "unpack" an argument from the variable arguments, equivalent to VaList::arg ↩

2. oddly, if the value was within 0..=c_int::MAX, it is specifically okay to use va_arg for c_int when you "meant" c_uint, and vice versa. ↩

3. it's currently unclear to me what the full story is for unions and function pointers, due to some oddities in the spec of va_arg, but I believe they in practice operate like structs and data pointers ↩

4. they seem to be passed through "as-is" to LLVMIR but with zeroext and signext annotations as appropriate. I have no idea if that's correct semantically according to LLVM and the LangRef is unclear on that. ↩

5. I don't really think rustc should be expected to do these argument promotions, either, as proposing such immediately opens a huge can of worms about what should be done for types C doesn't even have⁷. These promotions also, frankly, perpetuate misunderstandings, as this very bug report reveals. ↩

6. This is funny to me, given the improper_ctypes lint, for instance, is... very aggressive. This entire thing raises the question, to me, of "Who is Rust's support for calling variable arguments for? Is it for people who want to merely call FFI surfaces that C definitely supports? Or is it for doing strange things that C... hmm... doesn't technically prohibit?" ↩

7. If we did want to fix this by doing the promotions anyways, that seems like something that could be accomplished via library code. Then rustc might have to learn something like "call this function on this unsafe trait, implicitly", but that seems more useful in general. Then if someone implements that trait incorrectly, the compiler team can shrug at them and say "what did you expect?" ↩

[workingjubilee]

I've figured out enough to know about what needs to be implemented to at least close the miscompilation part soon, it's now just a matter of finding the right place to slot in the necessary operations before the call.