Rollup merge of #141341 - folkertdev:limit-VaArgSafe-impls, r=workingjubilee

limit impls of `VaArgSafe` to just types that are actually safe

tracking issue: #44930

Retrieving 8- or 16-bit integer arguments from a `VaList` is not safe, because such types are subject to upcasting. See #61275 (comment) for more detail.

This PR also makes the instances of `VaArgSafe` visible in the documentation, and uses a private sealed trait to make sure users cannot create additional impls of `VaArgSafe`, which would almost certainly cause UB.

r? `@workingjubilee`

Author: matthiaskrgr

library/core/src/ffi/mod.rs

@@ -28,7 +28,7 @@ pub mod c_str;
     issue = "44930",
     reason = "the `c_variadic` feature has not been properly tested on all supported platforms"
 )]
-pub use self::va_list::{VaList, VaListImpl};
+pub use self::va_list::{VaArgSafe, VaList, VaListImpl};
 
 #[unstable(
     feature = "c_variadic",

library/core/src/ffi/va_list.rs

@@ -223,39 +223,57 @@ impl<'a, 'f: 'a> DerefMut for VaList<'a, 'f> {
     }
 }
 
-// The VaArgSafe trait needs to be used in public interfaces, however, the trait
-// itself must not be allowed to be used outside this module. Allowing users to
-// implement the trait for a new type (thereby allowing the va_arg intrinsic to
-// be used on a new type) is likely to cause undefined behavior.
-//
-// FIXME(dlrobertson): In order to use the VaArgSafe trait in a public interface
-// but also ensure it cannot be used elsewhere, the trait needs to be public
-// within a private module. Once RFC 2145 has been implemented look into
-// improving this.
-mod sealed_trait {
-    /// Trait which permits the allowed types to be used with [super::VaListImpl::arg].
-    pub unsafe trait VaArgSafe {}
-}
+mod sealed {
+    pub trait Sealed {}
 
-macro_rules! impl_va_arg_safe {
-    ($($t:ty),+) => {
-        $(
-            unsafe impl sealed_trait::VaArgSafe for $t {}
-        )+
-    }
+    impl Sealed for i32 {}
+    impl Sealed for i64 {}
+    impl Sealed for isize {}
+
+    impl Sealed for u32 {}
+    impl Sealed for u64 {}
+    impl Sealed for usize {}
+
+    impl Sealed for f64 {}
+
+    impl<T> Sealed for *mut T {}
+    impl<T> Sealed for *const T {}
 }
 
-impl_va_arg_safe! {i8, i16, i32, i64, usize}
-impl_va_arg_safe! {u8, u16, u32, u64, isize}
-impl_va_arg_safe! {f64}
+/// Trait which permits the allowed types to be used with [`VaListImpl::arg`].
+///
+/// # Safety
+///
+/// This trait must only be implemented for types that C passes as varargs without implicit promotion.
+///
+/// In C varargs, integers smaller than [`c_int`] and floats smaller than [`c_double`]
+/// are implicitly promoted to [`c_int`] and [`c_double`] respectively. Implementing this trait for
+/// types that are subject to this promotion rule is invalid.
+///
+/// [`c_int`]: core::ffi::c_int
+/// [`c_double`]: core::ffi::c_double
+pub unsafe trait VaArgSafe: sealed::Sealed {}
+
+// i8 and i16 are implicitly promoted to c_int in C, and cannot implement `VaArgSafe`.
+unsafe impl VaArgSafe for i32 {}
+unsafe impl VaArgSafe for i64 {}
+unsafe impl VaArgSafe for isize {}
+
+// u8 and u16 are implicitly promoted to c_int in C, and cannot implement `VaArgSafe`.
+unsafe impl VaArgSafe for u32 {}
+unsafe impl VaArgSafe for u64 {}
+unsafe impl VaArgSafe for usize {}
+
+// f32 is implicitly promoted to c_double in C, and cannot implement `VaArgSafe`.
+unsafe impl VaArgSafe for f64 {}
 
-unsafe impl<T> sealed_trait::VaArgSafe for *mut T {}
-unsafe impl<T> sealed_trait::VaArgSafe for *const T {}
+unsafe impl<T> VaArgSafe for *mut T {}
+unsafe impl<T> VaArgSafe for *const T {}
 
 impl<'f> VaListImpl<'f> {
     /// Advance to the next arg.
     #[inline]
-    pub unsafe fn arg<T: sealed_trait::VaArgSafe>(&mut self) -> T {
+    pub unsafe fn arg<T: VaArgSafe>(&mut self) -> T {
         // SAFETY: the caller must uphold the safety contract for `va_arg`.
         unsafe { va_arg(self) }
     }
@@ -317,4 +335,4 @@ unsafe fn va_copy<'f>(dest: *mut VaListImpl<'f>, src: &VaListImpl<'f>);
 /// argument `ap` points to.
 #[rustc_intrinsic]
 #[rustc_nounwind]
-unsafe fn va_arg<T: sealed_trait::VaArgSafe>(ap: &mut VaListImpl<'_>) -> T;
+unsafe fn va_arg<T: VaArgSafe>(ap: &mut VaListImpl<'_>) -> T;

library/std/src/ffi/mod.rs

@@ -172,7 +172,7 @@ pub use core::ffi::c_void;
               all supported platforms",
     issue = "44930"
 )]
-pub use core::ffi::{VaList, VaListImpl};
+pub use core::ffi::{VaArgSafe, VaList, VaListImpl};
 #[stable(feature = "core_ffi_c", since = "1.64.0")]
 pub use core::ffi::{
     c_char, c_double, c_float, c_int, c_long, c_longlong, c_schar, c_short, c_uchar, c_uint,

tests/run-make/c-link-to-rust-va-list-fn/checkrust.rs

@@ -30,9 +30,9 @@ pub unsafe extern "C" fn check_list_0(mut ap: VaList) -> usize {
 #[no_mangle]
 pub unsafe extern "C" fn check_list_1(mut ap: VaList) -> usize {
     continue_if!(ap.arg::<c_int>() == -1);
-    continue_if!(ap.arg::<c_char>() == 'A' as c_char);
-    continue_if!(ap.arg::<c_char>() == '4' as c_char);
-    continue_if!(ap.arg::<c_char>() == ';' as c_char);
+    continue_if!(ap.arg::<c_int>() == 'A' as c_int);
+    continue_if!(ap.arg::<c_int>() == '4' as c_int);
+    continue_if!(ap.arg::<c_int>() == ';' as c_int);
     continue_if!(ap.arg::<c_int>() == 0x32);
     continue_if!(ap.arg::<c_int>() == 0x10000001);
     continue_if!(compare_c_str(ap.arg::<*const c_char>(), "Valid!"));
@@ -43,7 +43,7 @@ pub unsafe extern "C" fn check_list_1(mut ap: VaList) -> usize {
 pub unsafe extern "C" fn check_list_2(mut ap: VaList) -> usize {
     continue_if!(ap.arg::<c_double>().floor() == 3.14f64.floor());
     continue_if!(ap.arg::<c_long>() == 12);
-    continue_if!(ap.arg::<c_char>() == 'a' as c_char);
+    continue_if!(ap.arg::<c_int>() == 'a' as c_int);
     continue_if!(ap.arg::<c_double>().floor() == 6.18f64.floor());
     continue_if!(compare_c_str(ap.arg::<*const c_char>(), "Hello"));
     continue_if!(ap.arg::<c_int>() == 42);
@@ -55,7 +55,7 @@ pub unsafe extern "C" fn check_list_2(mut ap: VaList) -> usize {
 pub unsafe extern "C" fn check_list_copy_0(mut ap: VaList) -> usize {
     continue_if!(ap.arg::<c_double>().floor() == 6.28f64.floor());
     continue_if!(ap.arg::<c_int>() == 16);
-    continue_if!(ap.arg::<c_char>() == 'A' as c_char);
+    continue_if!(ap.arg::<c_int>() == 'A' as c_int);
     continue_if!(compare_c_str(ap.arg::<*const c_char>(), "Skip Me!"));
     ap.with_copy(
         |mut ap| {
@@ -75,7 +75,7 @@ pub unsafe extern "C" fn check_varargs_0(_: c_int, mut ap: ...) -> usize {
 pub unsafe extern "C" fn check_varargs_1(_: c_int, mut ap: ...) -> usize {
     continue_if!(ap.arg::<c_double>().floor() == 3.14f64.floor());
     continue_if!(ap.arg::<c_long>() == 12);
-    continue_if!(ap.arg::<c_char>() == 'A' as c_char);
+    continue_if!(ap.arg::<c_int>() == 'A' as c_int);
     continue_if!(ap.arg::<c_longlong>() == 1);
     0
 }