Functions taking pointers without lifetimes are unsafe. PiperOrigin-RevId: 423186690
diff --git a/rs_bindings_from_cc/src_code_gen.rs b/rs_bindings_from_cc/src_code_gen.rs index 99a2778..b6250a5 100644 --- a/rs_bindings_from_cc/src_code_gen.rs +++ b/rs_bindings_from_cc/src_code_gen.rs
@@ -210,15 +210,23 @@ let param_idents = func.params.iter().map(|p| make_ident(&p.identifier.identifier)).collect_vec(); - let param_types = func + let param_type_kinds = func .params .iter() .map(|p| { - format_rs_type(&p.type_.rs_type, ir, &lifetime_to_name).with_context(|| { - format!("Failed to format type for parameter {:?} on {:?}", p, func) + RsTypeKind::new(&p.type_.rs_type, ir).with_context(|| { + format!("Failed to process type of parameter {:?} on {:?}", p, func) }) }) .collect::<Result<Vec<_>>>()?; + let param_types = param_type_kinds + .iter() + .map(|t| { + t.format(ir, &lifetime_to_name) + .with_context(|| format!("Failed to format parameter type {:?} on {:?}", t, func)) + }) + .collect::<Result<Vec<_>>>()?; + let is_unsafe = param_type_kinds.iter().any(|p| matches!(p, RsTypeKind::Pointer { .. })); let maybe_record: Option<&Record> = func.member_func_metadata.as_ref().map(|meta| meta.find_record(ir)).transpose()?; @@ -270,8 +278,7 @@ } 2 => { // TODO(lukasza): Do something smart with move constructor. - let param_rs_type_kind = RsTypeKind::new(&func.params[1].type_.rs_type, ir)?; - if param_rs_type_kind.is_shared_ref_to(record) { + if param_type_kinds[1].is_shared_ref_to(record) { // Copy constructor if should_derive_clone(record) { return Ok(None); @@ -308,7 +315,7 @@ .map(|(ident, type_)| quote! { #ident : #type_ }) .collect_vec(); let mut lifetimes = func.lifetime_params.iter().collect_vec(); - let mut maybe_first_api_param = func.params.get(0); + let mut maybe_first_api_param = param_type_kinds.get(0); if func.name == UnqualifiedIdentifier::Constructor { return_type_fragment = quote! { -> Self }; @@ -320,21 +327,22 @@ // TODO(lukasza): Avoid incorrectly trimming the lifetimes when a // lifetime of `__this` is also used in another parameter: // fn constructor<'a>(__this: &'a mut Self, x: &'a i32) - let maybe_first_lifetime = - maybe_first_api_param.unwrap().type_.rs_type.lifetime_args.first(); + // TODO(lukasza): Should be able to guarantee presence of the + // lifetime once skipping generating unsafe constructor bindings. + let maybe_first_lifetime = func.params[0].type_.rs_type.lifetime_args.first(); if let Some(no_longer_needed_lifetime_id) = maybe_first_lifetime { lifetimes.retain(|l| l.id != *no_longer_needed_lifetime_id); } // Rebind `maybe_first_api_param` to the next param after `__this`. - maybe_first_api_param = func.params.get(1); + maybe_first_api_param = param_type_kinds.get(1); } // Change `__this: &'a SomeStruct` into `&'a self` if needed. if format_first_param_as_self { let first_api_param = maybe_first_api_param .ok_or_else(|| anyhow!("No parameter to format as 'self': {:?}", func))?; - let self_decl = RsTypeKind::new(&first_api_param.type_.rs_type, ir)? + let self_decl = first_api_param .format_as_self_param_for_instance_method(func, ir, &lifetime_to_name) .with_context(|| { format!("Failed to format as `self` param: {:?}", first_api_param) @@ -346,7 +354,16 @@ } let func_body = match &func.name { - UnqualifiedIdentifier::Identifier(_) | UnqualifiedIdentifier::Destructor => { + UnqualifiedIdentifier::Identifier(_) => { + let mut body = quote! { crate::detail::#thunk_ident( #( #thunk_args ),* ) }; + // Only need to wrap everything in an `unsafe { ... }` block if + // the *whole* api function is safe. + if !is_unsafe { + body = quote! { unsafe { #body } }; + } + body + } + UnqualifiedIdentifier::Destructor => { quote! { unsafe { crate::detail::#thunk_ident( #( #thunk_args ),* ) } } } UnqualifiedIdentifier::Constructor => { @@ -368,9 +385,22 @@ } }; - let pub_ = match impl_kind { - ImplKind::None | ImplKind::Struct => quote! { pub }, - ImplKind::Trait(_) => quote! {}, + let (pub_, unsafe_) = match impl_kind { + ImplKind::None | ImplKind::Struct => ( + quote! { pub }, + if is_unsafe { + quote! {unsafe} + } else { + quote! {} + }, + ), + ImplKind::Trait(_) => ( + quote! {}, + // TODO(b/214244223): Correctly handle `is_unsafe` when + // generating trait impls (treat destructors as safe, skip + // bindings for constructors and things like PartialEq). + quote! {}, + ), }; let lifetimes = lifetimes.into_iter().map(|l| format_lifetime_name(&l.name)); @@ -378,7 +408,7 @@ quote! { #[inline(always)] - #pub_ fn #func_name #generic_params( #( #api_params ),* ) #return_type_fragment { + #pub_ #unsafe_ fn #func_name #generic_params( #( #api_params ),* ) #return_type_fragment { #func_body } } @@ -425,7 +455,7 @@ if param_types.is_empty() || func.params.is_empty() { bail!("Constructors should have at least one parameter (__this)"); } - param_types[0] = RsTypeKind::new(&func.params[0].type_.rs_type, ir)? + param_types[0] = param_type_kinds[0] .format_as_this_param_for_constructor_thunk(ir, &lifetime_to_name) .with_context(|| { format!("Failed to format `__this` param for a thunk: {:?}", func.params[0]) @@ -1660,8 +1690,8 @@ rs_api, quote! { #[inline(always)] - pub fn Deref(p: *const *mut i32) -> *mut i32 { - unsafe { crate::detail::__rust_thunk___Z5DerefPKPi(p) } + pub unsafe fn Deref(p: *const *mut i32) -> *mut i32 { + crate::detail::__rust_thunk___Z5DerefPKPi(p) } } ); @@ -1705,8 +1735,8 @@ rs_api, quote! { #[inline(always)] - pub fn f(str: *const i8) { - unsafe { crate::detail::__rust_thunk___Z1fPKc(str) } + pub unsafe fn f(str: *const i8) { + crate::detail::__rust_thunk___Z1fPKc(str) } } ); @@ -2053,8 +2083,7 @@ // functions yet, except in the case of overloaded constructors with a // single parameter. let ir = ir_from_cc( - r#" - void f(); + r#" void f(); void f(int i); struct S1 final { void f(); @@ -2083,7 +2112,7 @@ // But we can import member functions that have the same name as a free // function. - assert_rs_matches!(rs_api, quote! {pub fn f(__this: *mut S2)}); + assert_rs_matches!(rs_api, quote! {pub unsafe fn f(__this: *mut S2)}); // We can also import overloaded single-parameter constructors. assert_rs_matches!(rs_api, quote! {impl From<i32> for S3});
diff --git a/rs_bindings_from_cc/test/function/no_elided_lifetimes/no_elided_lifetimes.cc b/rs_bindings_from_cc/test/function/no_elided_lifetimes/no_elided_lifetimes.cc new file mode 100644 index 0000000..966f194 --- /dev/null +++ b/rs_bindings_from_cc/test/function/no_elided_lifetimes/no_elided_lifetimes.cc
@@ -0,0 +1,15 @@ +// Part of the Crubit project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "rs_bindings_from_cc/test/function/no_elided_lifetimes/no_elided_lifetimes.h" + +namespace { + +const int* g_int_ptr = nullptr; + +} // namespace + +void StorePointer(const int& int_ref) { g_int_ptr = &int_ref; } + +int ReadStoredPointer() { return *g_int_ptr; }
diff --git a/rs_bindings_from_cc/test/function/no_elided_lifetimes/no_elided_lifetimes.h b/rs_bindings_from_cc/test/function/no_elided_lifetimes/no_elided_lifetimes.h new file mode 100644 index 0000000..df3a730 --- /dev/null +++ b/rs_bindings_from_cc/test/function/no_elided_lifetimes/no_elided_lifetimes.h
@@ -0,0 +1,23 @@ +// Part of the Crubit project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#ifndef CRUBIT_RS_BINDINGS_FROM_CC_TEST_FUNCTION_NO_ELIDED_LIFETIMES_NO_ELIDED_LIFETIMES_H_ +#define CRUBIT_RS_BINDINGS_FROM_CC_TEST_FUNCTION_NO_ELIDED_LIFETIMES_NO_ELIDED_LIFETIMES_H_ + +// The two functions below help test interactions between safe Rust code +// and C++ code without lifetime annotations. Here, `StorePointer` is not +// annotated with lifetimes (and therefore can stash a pointer to `int_ref` +// in a global variable, asking callers of `ReadStoredPointer` to "be careful"). +// +// The function-under-test below takes `int_ref` as `const int&`, because: +// 1) In presence of lifetimes (no lifetimes below) references would +// become references in the generated Rust bindings and we care +// mostly about safety of using Rust references. +// 2) We want to test the simplest possible scenario that shows the unsafety +// problem. Therefore we test with `int` rather than with a struct. +// NOLINTNEXTLINE(google3-readability-pass-trivial-by-value) +void StorePointer(const int& int_ref); +int ReadStoredPointer(); + +#endif // CRUBIT_RS_BINDINGS_FROM_CC_TEST_FUNCTION_NO_ELIDED_LIFETIMES_NO_ELIDED_LIFETIMES_H_
diff --git a/rs_bindings_from_cc/test/function/no_elided_lifetimes/test.rs b/rs_bindings_from_cc/test/function/no_elided_lifetimes/test.rs new file mode 100644 index 0000000..ad41db5 --- /dev/null +++ b/rs_bindings_from_cc/test/function/no_elided_lifetimes/test.rs
@@ -0,0 +1,36 @@ +// Part of the Crubit project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#[cfg(test)] +mod tests { + use no_elided_lifetimes::*; + + #[test] + fn test_store_pointer() { + let mut boxed_int = Box::new(123); + + // Without (human-, or machine-verified) lifetime annotations, passing a + // pointer (or reference) across FFI boundary is unsafe. The call to + // `StorePointer` should *not* be possible without an `unsafe` block. + // + // Note that if `StorePointer` function was *not* marked as `unsafe`, + // then Rust Clippy would warn about the code below having an + // "unnecessary `unsafe` block". Seeing such Clippy warning would + // indicate a regression. + unsafe { + StorePointer(boxed_int.as_mut()); + } + assert_eq!(123, ReadStoredPointer()); + + *boxed_int.as_mut() = 456; + assert_eq!(456, ReadStoredPointer()); + + // The commented-out `ReadStoredPointer()` would dereference a dangling + // pointer, resulting in Undefined Behavior (UB). In normal builds, UB + // might result in `ReadStoredPointer()` returning 0. In ASan builds, + // the UB would be caught and reported as an error. + drop(boxed_int); + //assert_eq!(456, ReadStoredPointer()); + } +}
diff --git a/rs_bindings_from_cc/test/golden/no_elided_lifetimes_rs_api.rs b/rs_bindings_from_cc/test/golden/no_elided_lifetimes_rs_api.rs index c3ddf51..e49f424 100644 --- a/rs_bindings_from_cc/test/golden/no_elided_lifetimes_rs_api.rs +++ b/rs_bindings_from_cc/test/golden/no_elided_lifetimes_rs_api.rs
@@ -12,8 +12,8 @@ pub type __builtin_ms_va_list = *mut u8; #[inline(always)] -pub fn free_function(p1: *mut i32) -> *mut i32 { - unsafe { crate::detail::__rust_thunk___Z13free_functionRi(p1) } +pub unsafe fn free_function(p1: *mut i32) -> *mut i32 { + crate::detail::__rust_thunk___Z13free_functionRi(p1) } #[derive(Clone, Copy)] @@ -29,15 +29,15 @@ impl S { #[inline(always)] - pub fn const_method(__this: *const S, p1: *mut i32, p2: *mut i32) -> *mut i32 { - unsafe { crate::detail::__rust_thunk___ZNK1S12const_methodERiS0_(__this, p1, p2) } + pub unsafe fn const_method(__this: *const S, p1: *mut i32, p2: *mut i32) -> *mut i32 { + crate::detail::__rust_thunk___ZNK1S12const_methodERiS0_(__this, p1, p2) } } impl S { #[inline(always)] - pub fn method(__this: *mut S, p1: *mut i32, p2: *mut i32) -> *mut i32 { - unsafe { crate::detail::__rust_thunk___ZN1S6methodERiS0_(__this, p1, p2) } + pub unsafe fn method(__this: *mut S, p1: *mut i32, p2: *mut i32) -> *mut i32 { + crate::detail::__rust_thunk___ZN1S6methodERiS0_(__this, p1, p2) } } @@ -72,8 +72,8 @@ // Parameter type 'struct S &&' is not supported #[inline(always)] -pub fn take_pointer(p: *mut i32) { - unsafe { crate::detail::__rust_thunk___Z12take_pointerPi(p) } +pub unsafe fn take_pointer(p: *mut i32) { + crate::detail::__rust_thunk___Z12take_pointerPi(p) } // CRUBIT_RS_BINDINGS_FROM_CC_TEST_GOLDEN_NO_ELIDED_LIFETIMES_H_