From ab5e0e11fc53baaca88b8f1acd44dde44692507a Mon Sep 17 00:00:00 2001 From: xylaaaaa <2392805527@qq.com> Date: Mon, 26 Jan 2026 20:11:11 +0800 Subject: [PATCH 1/5] Fix Bytes move assignment --- src/paimon/common/memory/bytes.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/paimon/common/memory/bytes.cpp b/src/paimon/common/memory/bytes.cpp index 6b2b6c9b..8a5f6d7f 100644 --- a/src/paimon/common/memory/bytes.cpp +++ b/src/paimon/common/memory/bytes.cpp @@ -59,9 +59,16 @@ Bytes& Bytes::operator=(Bytes&& other) noexcept { if (&other == this) { return *this; } - this->~Bytes(); - std::memcpy(this, &other, sizeof(*this)); - new (&other) Bytes(); + if (data_ != nullptr) { + assert(pool_); + pool_->Free(data_, size_); + } + pool_ = other.pool_; + data_ = other.data_; + size_ = other.size_; + other.pool_ = nullptr; + other.data_ = nullptr; + other.size_ = 0; return *this; } From 925565b7e3d88500a8369dabd9db617651bd682a Mon Sep 17 00:00:00 2001 From: xylaaaaa <2392805527@qq.com> Date: Wed, 28 Jan 2026 11:35:27 +0800 Subject: [PATCH 2/5] add test --- src/paimon/common/memory/bytes_test.cpp | 66 +++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/src/paimon/common/memory/bytes_test.cpp b/src/paimon/common/memory/bytes_test.cpp index 419042ea..d669456e 100644 --- a/src/paimon/common/memory/bytes_test.cpp +++ b/src/paimon/common/memory/bytes_test.cpp @@ -82,4 +82,70 @@ TEST(BytesTest, TestCompare) { ASSERT_LT(*bytes1, *bytes2); ASSERT_FALSE(*bytes1 < *bytes1); } + +// Test to verify that move assignment correctly handles memory and prevents double-free. +// Before the fix, the old implementation used memcpy + destructor which caused: +// 1. The target's original memory was freed in destructor +// 2. After memcpy, both source and target pointed to same memory +// 3. When source was "reset" via placement new, it became empty +// 4. But if move assignment was called again on the same target, the memcpy'd +// pointer would be freed again (double-free) or memory accounting would be wrong. +TEST(BytesTest, TestMoveAssignmentNoDoubleFree) { + auto pool = paimon::GetMemoryPool(); + + // Create three Bytes objects on stack + Bytes a("aaaa", pool.get()); // 4 bytes + Bytes b("bb", pool.get()); // 2 bytes + Bytes c("cccccc", pool.get()); // 6 bytes + ASSERT_EQ(12, pool->CurrentUsage()); // 4 + 2 + 6 = 12 + + // First move: b = std::move(a) + // Should free b's original memory (2 bytes), transfer a's memory to b + b = std::move(a); + ASSERT_EQ(10, pool->CurrentUsage()); // 4 + 6 = 10 (b's 2 bytes freed) + ASSERT_EQ("aaaa", std::string(b.data(), b.size())); + ASSERT_EQ(nullptr, a.data()); + ASSERT_EQ(0, a.size()); + + // Second move: b = std::move(c) + // Should free b's current memory (4 bytes from a), transfer c's memory to b + // This is where the old implementation would cause issues: + // - Old code would call destructor on b, freeing the 4 bytes + // - Then memcpy c into b, making b point to c's 6-byte buffer + // - Memory accounting would be wrong because Free was called on wrong data + b = std::move(c); + ASSERT_EQ(6, pool->CurrentUsage()); // Only c's 6 bytes remain (now owned by b) + ASSERT_EQ("cccccc", std::string(b.data(), b.size())); + ASSERT_EQ(nullptr, c.data()); + ASSERT_EQ(0, c.size()); + + // Self-assignment should be safe + b = std::move(b); + ASSERT_EQ(6, pool->CurrentUsage()); + ASSERT_EQ("cccccc", std::string(b.data(), b.size())); +} + +// Test move assignment with heap-allocated Bytes to verify no double-free +// when combining unique_ptr semantics with move assignment +TEST(BytesTest, TestMoveAssignmentHeapAllocated) { + auto pool = paimon::GetMemoryPool(); + + auto bytes1 = Bytes::AllocateBytes("hello", pool.get()); // 5 bytes + sizeof(Bytes) + auto bytes2 = Bytes::AllocateBytes("world!", pool.get()); // 6 bytes + sizeof(Bytes) + size_t expected = 5 + 6 + 2 * sizeof(Bytes); + ASSERT_EQ(expected, pool->CurrentUsage()); + + // Move the content of bytes1 into bytes2's Bytes object + // This should free "world!" (6 bytes) and transfer "hello" ownership + *bytes2 = std::move(*bytes1); + expected = 5 + 2 * sizeof(Bytes); // "world!" freed, "hello" transferred + ASSERT_EQ(expected, pool->CurrentUsage()); + ASSERT_EQ("hello", std::string(bytes2->data(), bytes2->size())); + ASSERT_EQ(nullptr, bytes1->data()); + + // Reset bytes2, which should free "hello" + bytes2.reset(); + expected = sizeof(Bytes); // Only bytes1's empty Bytes struct remains + ASSERT_EQ(expected, pool->CurrentUsage()); +} } // namespace paimon::test From 0df4771a9270fe95916c591686cba8262b858d14 Mon Sep 17 00:00:00 2001 From: xylaaaaa <2392805527@qq.com> Date: Tue, 3 Feb 2026 15:29:20 +0800 Subject: [PATCH 3/5] fix --- src/paimon/common/memory/bytes_test.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/paimon/common/memory/bytes_test.cpp b/src/paimon/common/memory/bytes_test.cpp index d669456e..7e2c00a5 100644 --- a/src/paimon/common/memory/bytes_test.cpp +++ b/src/paimon/common/memory/bytes_test.cpp @@ -119,8 +119,9 @@ TEST(BytesTest, TestMoveAssignmentNoDoubleFree) { ASSERT_EQ(nullptr, c.data()); ASSERT_EQ(0, c.size()); - // Self-assignment should be safe - b = std::move(b); + // Self-assignment should be safe. Use an alias to avoid -Wself-move. + Bytes* self = &b; + b = std::move(*self); ASSERT_EQ(6, pool->CurrentUsage()); ASSERT_EQ("cccccc", std::string(b.data(), b.size())); } From e1d4845f5eb6ae8148d39bfdbfa5bfbb65986592 Mon Sep 17 00:00:00 2001 From: xylaaaaa <2392805527@qq.com> Date: Tue, 3 Feb 2026 15:40:56 +0800 Subject: [PATCH 4/5] fix clang-format --- src/paimon/common/memory/bytes_test.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/paimon/common/memory/bytes_test.cpp b/src/paimon/common/memory/bytes_test.cpp index 7e2c00a5..b7b49782 100644 --- a/src/paimon/common/memory/bytes_test.cpp +++ b/src/paimon/common/memory/bytes_test.cpp @@ -94,9 +94,9 @@ TEST(BytesTest, TestMoveAssignmentNoDoubleFree) { auto pool = paimon::GetMemoryPool(); // Create three Bytes objects on stack - Bytes a("aaaa", pool.get()); // 4 bytes - Bytes b("bb", pool.get()); // 2 bytes - Bytes c("cccccc", pool.get()); // 6 bytes + Bytes a("aaaa", pool.get()); // 4 bytes + Bytes b("bb", pool.get()); // 2 bytes + Bytes c("cccccc", pool.get()); // 6 bytes ASSERT_EQ(12, pool->CurrentUsage()); // 4 + 2 + 6 = 12 // First move: b = std::move(a) @@ -131,8 +131,8 @@ TEST(BytesTest, TestMoveAssignmentNoDoubleFree) { TEST(BytesTest, TestMoveAssignmentHeapAllocated) { auto pool = paimon::GetMemoryPool(); - auto bytes1 = Bytes::AllocateBytes("hello", pool.get()); // 5 bytes + sizeof(Bytes) - auto bytes2 = Bytes::AllocateBytes("world!", pool.get()); // 6 bytes + sizeof(Bytes) + auto bytes1 = Bytes::AllocateBytes("hello", pool.get()); // 5 bytes + sizeof(Bytes) + auto bytes2 = Bytes::AllocateBytes("world!", pool.get()); // 6 bytes + sizeof(Bytes) size_t expected = 5 + 6 + 2 * sizeof(Bytes); ASSERT_EQ(expected, pool->CurrentUsage()); From b8f0b0438cb16c1c5e54256d0a5529a2139676f3 Mon Sep 17 00:00:00 2001 From: xylaaaaa <2392805527@qq.com> Date: Tue, 3 Feb 2026 16:45:31 +0800 Subject: [PATCH 5/5] fix --- src/paimon/common/memory/bytes_test.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/paimon/common/memory/bytes_test.cpp b/src/paimon/common/memory/bytes_test.cpp index b7b49782..40f8a8d3 100644 --- a/src/paimon/common/memory/bytes_test.cpp +++ b/src/paimon/common/memory/bytes_test.cpp @@ -104,8 +104,9 @@ TEST(BytesTest, TestMoveAssignmentNoDoubleFree) { b = std::move(a); ASSERT_EQ(10, pool->CurrentUsage()); // 4 + 6 = 10 (b's 2 bytes freed) ASSERT_EQ("aaaa", std::string(b.data(), b.size())); - ASSERT_EQ(nullptr, a.data()); - ASSERT_EQ(0, a.size()); + // Moved-from object is expected to be empty by Bytes' contract. + ASSERT_EQ(nullptr, a.data()); // NOLINT(bugprone-use-after-move, clang-analyzer-cplusplus.Move) + ASSERT_EQ(0, a.size()); // NOLINT(bugprone-use-after-move, clang-analyzer-cplusplus.Move) // Second move: b = std::move(c) // Should free b's current memory (4 bytes from a), transfer c's memory to b @@ -116,8 +117,9 @@ TEST(BytesTest, TestMoveAssignmentNoDoubleFree) { b = std::move(c); ASSERT_EQ(6, pool->CurrentUsage()); // Only c's 6 bytes remain (now owned by b) ASSERT_EQ("cccccc", std::string(b.data(), b.size())); - ASSERT_EQ(nullptr, c.data()); - ASSERT_EQ(0, c.size()); + // Moved-from object is expected to be empty by Bytes' contract. + ASSERT_EQ(nullptr, c.data()); // NOLINT(bugprone-use-after-move, clang-analyzer-cplusplus.Move) + ASSERT_EQ(0, c.size()); // NOLINT(bugprone-use-after-move, clang-analyzer-cplusplus.Move) // Self-assignment should be safe. Use an alias to avoid -Wself-move. Bytes* self = &b;