From d8a22a281cc20dc58f1da62be02048622392da05 Mon Sep 17 00:00:00 2001 From: Folkert de Vries Date: Wed, 21 May 2025 14:21:33 +0200 Subject: [PATCH] limit impls of `VaArgSafe` to just types that are actually safe 8 and 16-bit integers are subject to upcasting in C, and hence are not reliably safe. users should perform their own casting and deal with the consequences --- library/core/src/ffi/mod.rs | 2 +- library/core/src/ffi/va_list.rs | 70 ++++++++++++------- library/std/src/ffi/mod.rs | 2 +- .../c-link-to-rust-va-list-fn/checkrust.rs | 12 ++-- 4 files changed, 52 insertions(+), 34 deletions(-) diff --git a/library/core/src/ffi/mod.rs b/library/core/src/ffi/mod.rs index 288d0df0d05bb..0bc98e2ea8645 100644 --- a/library/core/src/ffi/mod.rs +++ b/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", diff --git a/library/core/src/ffi/va_list.rs b/library/core/src/ffi/va_list.rs index 1ad8038cbf6eb..f12bd289f27aa 100644 --- a/library/core/src/ffi/va_list.rs +++ b/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 Sealed for *mut T {} + impl 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 sealed_trait::VaArgSafe for *mut T {} -unsafe impl sealed_trait::VaArgSafe for *const T {} +unsafe impl VaArgSafe for *mut T {} +unsafe impl VaArgSafe for *const T {} impl<'f> VaListImpl<'f> { /// Advance to the next arg. #[inline] - pub unsafe fn arg(&mut self) -> T { + pub unsafe fn arg(&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(ap: &mut VaListImpl<'_>) -> T; +unsafe fn va_arg(ap: &mut VaListImpl<'_>) -> T; diff --git a/library/std/src/ffi/mod.rs b/library/std/src/ffi/mod.rs index 567916099101b..024cb71b915d7 100644 --- a/library/std/src/ffi/mod.rs +++ b/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, diff --git a/tests/run-make/c-link-to-rust-va-list-fn/checkrust.rs b/tests/run-make/c-link-to-rust-va-list-fn/checkrust.rs index bcaef33344e51..7e4344f1c69b2 100644 --- a/tests/run-make/c-link-to-rust-va-list-fn/checkrust.rs +++ b/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::() == -1); - continue_if!(ap.arg::() == 'A' as c_char); - continue_if!(ap.arg::() == '4' as c_char); - continue_if!(ap.arg::() == ';' as c_char); + continue_if!(ap.arg::() == 'A' as c_int); + continue_if!(ap.arg::() == '4' as c_int); + continue_if!(ap.arg::() == ';' as c_int); continue_if!(ap.arg::() == 0x32); continue_if!(ap.arg::() == 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::().floor() == 3.14f64.floor()); continue_if!(ap.arg::() == 12); - continue_if!(ap.arg::() == 'a' as c_char); + continue_if!(ap.arg::() == 'a' as c_int); continue_if!(ap.arg::().floor() == 6.18f64.floor()); continue_if!(compare_c_str(ap.arg::<*const c_char>(), "Hello")); continue_if!(ap.arg::() == 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::().floor() == 6.28f64.floor()); continue_if!(ap.arg::() == 16); - continue_if!(ap.arg::() == 'A' as c_char); + continue_if!(ap.arg::() == '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::().floor() == 3.14f64.floor()); continue_if!(ap.arg::() == 12); - continue_if!(ap.arg::() == 'A' as c_char); + continue_if!(ap.arg::() == 'A' as c_int); continue_if!(ap.arg::() == 1); 0 }