Author: Vassil Vassilev Date: 2026-06-06T18:19:13+03:00 New Revision: 4706906ba256bfbb3590ae1eb05ef9cd4b92421a
URL: https://github.com/llvm/llvm-project/commit/4706906ba256bfbb3590ae1eb05ef9cd4b92421a DIFF: https://github.com/llvm/llvm-project/commit/4706906ba256bfbb3590ae1eb05ef9cd4b92421a.diff LOG: [clang-repl] Fix Value's move ctor releasing storage on construction (#200888) 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. Added: Modified: clang/lib/Interpreter/Value.cpp clang/unittests/Interpreter/InterpreterTest.cpp Removed: ################################################################################ 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..6a461590ed12e 100644 --- a/clang/unittests/Interpreter/InterpreterTest.cpp +++ b/clang/unittests/Interpreter/InterpreterTest.cpp @@ -452,6 +452,57 @@ TEST_F(InterpreterTest, ValueSetRawBitsCopiesByteCount) { 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) { std::vector<const char *> Args; std::unique_ptr<Interpreter> Interp = createInterpreter(Args); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
