@@ -82,4 +82,73 @@ TEST(BytesTest, TestCompare) {
8282 ASSERT_LT (*bytes1, *bytes2);
8383 ASSERT_FALSE (*bytes1 < *bytes1);
8484}
85+
86+ // Test to verify that move assignment correctly handles memory and prevents double-free.
87+ // Before the fix, the old implementation used memcpy + destructor which caused:
88+ // 1. The target's original memory was freed in destructor
89+ // 2. After memcpy, both source and target pointed to same memory
90+ // 3. When source was "reset" via placement new, it became empty
91+ // 4. But if move assignment was called again on the same target, the memcpy'd
92+ // pointer would be freed again (double-free) or memory accounting would be wrong.
93+ TEST (BytesTest, TestMoveAssignmentNoDoubleFree) {
94+ auto pool = paimon::GetMemoryPool ();
95+
96+ // Create three Bytes objects on stack
97+ Bytes a (" aaaa" get ()); // 4 bytes
98+ Bytes b (" bb" get ()); // 2 bytes
99+ Bytes c (" cccccc" get ()); // 6 bytes
100+ ASSERT_EQ (12 , pool->CurrentUsage ()); // 4 + 2 + 6 = 12
101+
102+ // First move: b = std::move(a)
103+ // Should free b's original memory (2 bytes), transfer a's memory to b
104+ b = std::move (a);
105+ ASSERT_EQ (10 , pool->CurrentUsage ()); // 4 + 6 = 10 (b's 2 bytes freed)
106+ ASSERT_EQ (" aaaa" std::string (b.data (), b.size ()));
107+ // Moved-from object is expected to be empty by Bytes' contract.
108+ ASSERT_EQ (nullptr , a.data ()); // NOLINT(bugprone-use-after-move, clang-analyzer-cplusplus.Move)
109+ ASSERT_EQ (0 , a.size ()); // NOLINT(bugprone-use-after-move, clang-analyzer-cplusplus.Move)
110+
111+ // Second move: b = std::move(c)
112+ // Should free b's current memory (4 bytes from a), transfer c's memory to b
113+ // This is where the old implementation would cause issues:
114+ // - Old code would call destructor on b, freeing the 4 bytes
115+ // - Then memcpy c into b, making b point to c's 6-byte buffer
116+ // - Memory accounting would be wrong because Free was called on wrong data
117+ b = std::move (c);
118+ ASSERT_EQ (6 , pool->CurrentUsage ()); // Only c's 6 bytes remain (now owned by b)
119+ ASSERT_EQ (" cccccc" std::string (b.data (), b.size ()));
120+ // Moved-from object is expected to be empty by Bytes' contract.
121+ ASSERT_EQ (nullptr , c.data ()); // NOLINT(bugprone-use-after-move, clang-analyzer-cplusplus.Move)
122+ ASSERT_EQ (0 , c.size ()); // NOLINT(bugprone-use-after-move, clang-analyzer-cplusplus.Move)
123+
124+ // Self-assignment should be safe. Use an alias to avoid -Wself-move.
125+ Bytes* self = &b;
126+ b = std::move (*self);
127+ ASSERT_EQ (6 , pool->CurrentUsage ());
128+ ASSERT_EQ (" cccccc" std::string (b.data (), b.size ()));
129+ }
130+
131+ // Test move assignment with heap-allocated Bytes to verify no double-free
132+ // when combining unique_ptr semantics with move assignment
133+ TEST (BytesTest, TestMoveAssignmentHeapAllocated) {
134+ auto pool = paimon::GetMemoryPool ();
135+
136+ auto bytes1 = Bytes::AllocateBytes (" hello" get ()); // 5 bytes + sizeof(Bytes)
137+ auto bytes2 = Bytes::AllocateBytes (" world!" get ()); // 6 bytes + sizeof(Bytes)
138+ size_t expected = 5 + 6 + 2 * sizeof (Bytes);
139+ ASSERT_EQ (expected, pool->CurrentUsage ());
140+
141+ // Move the content of bytes1 into bytes2's Bytes object
142+ // This should free "world!" (6 bytes) and transfer "hello" ownership
143+ *bytes2 = std::move (*bytes1);
144+ expected = 5 + 2 * sizeof (Bytes); // "world!" freed, "hello" transferred
145+ ASSERT_EQ (expected, pool->CurrentUsage ());
146+ ASSERT_EQ (" hello" std::string (bytes2->data (), bytes2->size ()));
147+ ASSERT_EQ (nullptr , bytes1->data ());
148+
149+ // Reset bytes2, which should free "hello"
150+ bytes2.reset ();
151+ expected = sizeof (Bytes); // Only bytes1's empty Bytes struct remains
152+ ASSERT_EQ (expected, pool->CurrentUsage ());
153+ }
85154} // namespace paimon::test
0 commit comments