From 6c8bbae6828ddd06e21a5cffa9a17d9936c8409c Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 25 Mar 2026 12:30:26 -0700 Subject: [PATCH] Delete C++ move operations for non-copyable Clone types Previously, Crubit would generate C++ move constructors for Rust types that implemented `Clone` but not `Copy` by deferring them to `MoveCtorStyle::Copy`. This would silently invoke the C++ copy constructor (and underlying Rust `Clone::clone`) instead of performing an actual move. By itself, this is not harmful at all. This caused severe memory leaks when crossing the FFI boundary within TansmuteAbi, however. For example, when `crubit::TransmuteAbi` decodes bytes into a C++ object, it takes the raw bytes from Rust and attempts to `std::move` them into ownership. Because the move constructors were falling back to deep copies, the new C++ object was a copied clone, and the original raw bytes (owning things like `Box`) were abandoned without `Drop::drop` ever being called. This CL removes `MoveCtorStyle::Copy` so that C++ move operations are explicitly deleted (`= delete`) for non-trivially movable `Clone` types. This correctly turns silent deep-copy memory leaks into compile-time errors. PiperOrigin-RevId: 889377273 --- .../database/adt_core_bindings.rs | 3 -- .../generate_bindings_test.rs | 7 ++-- cc_bindings_from_rs/generate_bindings/lib.rs | 5 --- .../test/known_traits/drop/drop.rs | 3 -- .../test/known_traits/drop/drop_test.cc | 34 ++++++------------- .../tuple_structs/tuple_structs_cc_api.h | 5 +++ 6 files changed, 19 insertions(+), 38 deletions(-) diff --git a/cc_bindings_from_rs/generate_bindings/database/adt_core_bindings.rs b/cc_bindings_from_rs/generate_bindings/database/adt_core_bindings.rs index 9e8ff7656..fde0783ac 100644 --- a/cc_bindings_from_rs/generate_bindings/database/adt_core_bindings.rs +++ b/cc_bindings_from_rs/generate_bindings/database/adt_core_bindings.rs @@ -100,7 +100,4 @@ pub enum MoveCtorStyle { Default, // The type is Default (and Unpin) and can be moved using MemSwap and std::move. MemSwap, - // The type cannot be moved but has a copy constructor and assignment operator that are used in - // lieu of the move constructor and assignment operator. - Copy, } diff --git a/cc_bindings_from_rs/generate_bindings/generate_bindings_test.rs b/cc_bindings_from_rs/generate_bindings/generate_bindings_test.rs index 951d1fec9..69f53e924 100644 --- a/cc_bindings_from_rs/generate_bindings/generate_bindings_test.rs +++ b/cc_bindings_from_rs/generate_bindings/generate_bindings_test.rs @@ -1747,10 +1747,9 @@ fn test_format_item_struct_with_custom_drop_and_no_default_and_clone(test_src: & } ); - // Implicit, but not `=default`-ed move constructor and move assignment - // operator. - assert_cc_not_matches!(main_api.tokens, quote! { TypeUnderTest(TypeUnderTest&&) }); - assert_cc_not_matches!(main_api.tokens, quote! { operator=(TypeUnderTest&&) }); + // Explicitly deleted move constructor and move assignment operator. + assert_cc_matches!(main_api.tokens, quote! { TypeUnderTest(TypeUnderTest&&) = delete; }); + assert_cc_matches!(main_api.tokens, quote! { operator=(TypeUnderTest&&) = delete; }); // No definition of a custom move constructor nor move assignment operator. assert_cc_not_matches!(result.cc_details.tokens, quote! { TypeUnderTest(TypeUnderTest&&) },); assert_cc_not_matches!(result.cc_details.tokens, quote! { operator=(TypeUnderTest&&) },); diff --git a/cc_bindings_from_rs/generate_bindings/lib.rs b/cc_bindings_from_rs/generate_bindings/lib.rs index ff2b4e9c0..a698236f0 100644 --- a/cc_bindings_from_rs/generate_bindings/lib.rs +++ b/cc_bindings_from_rs/generate_bindings/lib.rs @@ -1214,8 +1214,6 @@ fn has_move_ctor_and_assignment_operator<'tcx>( let is_unpin = self_ty.is_unpin(tcx, typing_env); if has_default_ctor && is_unpin { Some(MoveCtorStyle::MemSwap) - } else if db.has_copy_ctor_and_assignment_operator(def_id, self_ty).is_some() { - Some(MoveCtorStyle::Copy) } else { None } @@ -1234,9 +1232,6 @@ fn generate_move_ctor_and_assignment_operator<'tcx>( let adt_cc_name = &core.cc_short_name; let qualified_adt_name = &core.cc_fully_qualified_name; match db.has_move_ctor_and_assignment_operator(core.def_id, core.self_ty) { - // We rely on the copy constructor and assignment operator to handle the move - // operations. - Some(MoveCtorStyle::Copy) => Ok(ApiSnippets::default()), None => { bail!( "C++ move operations are unavailable for this type. See \ diff --git a/cc_bindings_from_rs/test/known_traits/drop/drop.rs b/cc_bindings_from_rs/test/known_traits/drop/drop.rs index b3cd67d8e..d20562211 100644 --- a/cc_bindings_from_rs/test/known_traits/drop/drop.rs +++ b/cc_bindings_from_rs/test/known_traits/drop/drop.rs @@ -165,9 +165,6 @@ pub mod drop_impl_with_clone { self.field = i; } } - impl DropImplWithClone { - pub fn take_by_value(_: Self) {} - } } /// Test for `Drop` support when: diff --git a/cc_bindings_from_rs/test/known_traits/drop/drop_test.cc b/cc_bindings_from_rs/test/known_traits/drop/drop_test.cc index 96a39c734..e19636830 100644 --- a/cc_bindings_from_rs/test/known_traits/drop/drop_test.cc +++ b/cc_bindings_from_rs/test/known_traits/drop/drop_test.cc @@ -120,9 +120,9 @@ TYPED_TEST(CustomDropWithDefaultTest, ByValue) { TEST(DropTest, DropImplWithClone) { using TypeUnderTest = drop::drop_impl_with_clone::DropImplWithClone; static_assert(!std::is_default_constructible_v); - static_assert(std::is_move_constructible_v); + static_assert(!std::is_move_constructible_v); static_assert(!std::is_trivially_move_constructible_v); - static_assert(std::is_move_assignable_v); + static_assert(!std::is_move_assignable_v); static_assert(!std::is_trivially_move_assignable_v); static_assert(std::is_destructible_v); static_assert(!std::is_trivially_destructible_v); @@ -151,9 +151,10 @@ TEST(DropTest, DropImplWithClone) { // We expect the move to be implemented in terms of copy, so we expect // a corresponding increase in clone counters. - TypeUnderTest s2(std::move(s)); + TypeUnderTest s2(s); EXPECT_EQ(123, s2.get_int()); - EXPECT_EQ(123, s.get_int()); // NOLINT(bugprone-use-after-move) + EXPECT_EQ(123, s.get_int()); // Since it's no longer movable, we must + // invoke copy explicitly. EXPECT_EQ(1, drop::counters::get_clone_count()); EXPECT_EQ(0, drop::counters::get_drop_count()); } @@ -169,10 +170,10 @@ TEST(DropTest, DropImplWithClone) { EXPECT_EQ(0, drop::counters::get_clone_from_count()); EXPECT_EQ(0, drop::counters::get_drop_count()); - // We expect the move to be implemented in terms of copy, so we expect - // a corresponding increase in clone counters. - s2 = std::move(s1); - EXPECT_EQ(123, s1.get_int()); // NOLINT(bugprone-use-after-move) + // We expect the explicit move to trigger a compiler error, so we verify + // copy assignment works instead. + s2 = s1; + EXPECT_EQ(123, s1.get_int()); EXPECT_EQ(123, s2.get_int()); EXPECT_EQ(0, drop::counters::get_clone_count()); EXPECT_EQ(1, drop::counters::get_clone_from_count()); @@ -182,8 +183,8 @@ TEST(DropTest, DropImplWithClone) { // (it would lead to aliasing-related UB in Rust when `self` and `source` // point to the same object). #pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wself-move" - s2 = std::move(s2); +#pragma clang diagnostic ignored "-Wself-assign-overloaded" + s2 = s2; #pragma clang diagnostic pop EXPECT_EQ(123, s1.get_int()); EXPECT_EQ(123, s2.get_int()); @@ -194,19 +195,6 @@ TEST(DropTest, DropImplWithClone) { EXPECT_EQ(0, drop::counters::get_clone_count()); EXPECT_EQ(1, drop::counters::get_clone_from_count()); EXPECT_EQ(2, drop::counters::get_drop_count()); - - // Testing pass by value. - drop::counters::reset_counts(); - { - TypeUnderTest s = TypeUnderTest::create_from_int(123); - EXPECT_EQ(0, drop::counters::get_clone_count()); - // Clones twice: once into the parameter, once again from the thunk to Rust. - // Both are destroyed. - TypeUnderTest::take_by_value(std::move(s)); - EXPECT_EQ(2, drop::counters::get_clone_count()); - EXPECT_EQ(2, drop::counters::get_drop_count()); - } - EXPECT_EQ(3, drop::counters::get_drop_count()); } TEST(DropTest, DropImplWithNothingElse) { diff --git a/cc_bindings_from_rs/test/structs/tuple_structs/tuple_structs_cc_api.h b/cc_bindings_from_rs/test/structs/tuple_structs/tuple_structs_cc_api.h index 56ee79b4e..c49321807 100644 --- a/cc_bindings_from_rs/test/structs/tuple_structs/tuple_structs_cc_api.h +++ b/cc_bindings_from_rs/test/structs/tuple_structs/tuple_structs_cc_api.h @@ -41,6 +41,11 @@ struct CRUBIT_INTERNAL_RUST_TYPE( // Drop::drop ~CloneNoDefault(); + // C++ move operations are unavailable for this type. See + // http://crubit.rs/rust/movable_types for an explanation of Rust types that + // are C++ movable. + CloneNoDefault(CloneNoDefault&&) = delete; + ::tuple_structs::CloneNoDefault& operator=(CloneNoDefault&&) = delete; // Clone::clone CloneNoDefault(const CloneNoDefault&);