https://github.com/vgvassilev updated https://github.com/llvm/llvm-project/pull/200888
>From 1733f723b2ab1c07638ff25a256c81d2fc7f0c87 Mon Sep 17 00:00:00 2001 From: Vassil Vassilev <[email protected]> Date: Mon, 1 Jun 2026 14:38:37 +0000 Subject: [PATCH] [clang-repl] Fix Value's move ctor releasing storage on construction Value::Value(Value &&) called Release() on the just-moved-into storage, decrementing the refcount to zero on the only remaining reference. Subsequent reads -- including ~Value() running clear(), which calls Release() a second time on the now-freed allocation -- hit use-after-free. The move should transfer the existing reference: the source clears IsManuallyAlloc so its destructor will not Release, and *this assumes ownership of the same refcount. Neither side needs to Retain or Release to keep the count correct. Add a regression test exercising move-construction, move-assignment, and follow-on copy-construction on a K_PtrOrObj Value. AddressSanitizer catches the bug without the fix. --- clang/lib/Interpreter/Value.cpp | 7 +-- .../unittests/Interpreter/InterpreterTest.cpp | 50 +++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/clang/lib/Interpreter/Value.cpp b/clang/lib/Interpreter/Value.cpp index b985361ed748a..eb0dcc7f7ff2e 100644 --- a/clang/lib/Interpreter/Value.cpp +++ b/clang/lib/Interpreter/Value.cpp @@ -177,9 +177,10 @@ Value::Value(Value &&RHS) noexcept { Data = RHS.Data; ValueKind = std::exchange(RHS.ValueKind, K_Unspecified); IsManuallyAlloc = std::exchange(RHS.IsManuallyAlloc, false); - - if (IsManuallyAlloc) - ValueStorage::getFromPayload(getPtr())->Release(); + // The move transfers ownership of the storage refcount from RHS to *this; + // RHS.IsManuallyAlloc is now false so its dtor won't Release, leaving + // *this as the sole owner of the existing reference. No Retain/Release is + // needed -- the physical RefCnt is unchanged across the move. } Value &Value::operator=(const Value &RHS) { diff --git a/clang/unittests/Interpreter/InterpreterTest.cpp b/clang/unittests/Interpreter/InterpreterTest.cpp index 2df29b3d5def5..ab2b588591d56 100644 --- a/clang/unittests/Interpreter/InterpreterTest.cpp +++ b/clang/unittests/Interpreter/InterpreterTest.cpp @@ -450,6 +450,56 @@ TEST_F(InterpreterTest, ValueSetRawBitsCopiesByteCount) { std::memcpy(Buf, &Src, sizeof(Src)); V2.setRawBits(Buf); EXPECT_EQ(V2.getLongLong(), Src); + +// Regression: Value's move ctor and move-assign must transfer ownership of +// the manually-allocated storage without changing the storage refcount. +// Earlier the move ctor called Release() on the just-moved-into storage, +// double-releasing on the next read. +TEST_F(InterpreterTest, ValueMoveSemantics) { + std::vector<const char *> Args = {"-fno-sized-deallocation"}; + std::unique_ptr<Interpreter> Interp = createInterpreter(Args); + + llvm::cantFail( + Interp->ParseAndExecute("struct MoveT { int v = 7; ~MoveT() {} };")); + + // Move-construct: source becomes empty, destination owns the storage. + Value Src; + llvm::cantFail(Interp->ParseAndExecute("MoveT{}", &Src)); + ASSERT_EQ(Src.getKind(), Value::K_PtrOrObj); + ASSERT_TRUE(Src.isManuallyAlloc()); + void *Payload = Src.getPtr(); + + Value Moved(std::move(Src)); + EXPECT_EQ(Moved.getKind(), Value::K_PtrOrObj); + EXPECT_TRUE(Moved.isManuallyAlloc()); + EXPECT_EQ(Moved.getPtr(), Payload); + EXPECT_EQ(Src.getKind(), Value::K_Unspecified); + EXPECT_FALSE(Src.isManuallyAlloc()); + + // Move-assign over a populated Value: previous storage released, new + // storage adopted with refcount unchanged. + Value Other; + llvm::cantFail(Interp->ParseAndExecute("MoveT{}", &Other)); + Other = std::move(Moved); + EXPECT_EQ(Other.getKind(), Value::K_PtrOrObj); + EXPECT_EQ(Other.getPtr(), Payload); + EXPECT_EQ(Moved.getKind(), Value::K_Unspecified); + + // Copy-construct still works (Retain bumps refcount; both share storage). + Value Copy(Other); + EXPECT_EQ(Copy.getKind(), Value::K_PtrOrObj); + EXPECT_EQ(Copy.getPtr(), Payload); + EXPECT_EQ(Other.getPtr(), Payload); + + // Force destruction order Copy -> Other -> Interp inside the test body so + // any latent corruption from a buggy move surfaces here. Pre-fix the move + // ctor leaves Other holding a dangling pointer; the subsequent Release in + // ~Copy / ~Other reads or asserts on freed memory. Without explicit + // teardown the abort happened during global cleanup, after gtest already + // recorded the test as OK. + Copy.clear(); + Other.clear(); + Interp.reset(); } TEST_F(InterpreterTest, TranslationUnit_CanonicalDecl) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
