[clang] [clang-repl] Fix Value's move ctor releasing storage on construction (PR #200888)

[Vassil Vassilev via cfe-commits]

https://github.com/vgvassilev updated https://github.com/llvm/llvm-project/pull/200888

>From 864031fd233ecf9b39affd3d9441e3a5f48f7aee Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassilev at gmail.com>
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 | 51 +++++++++++++++++++
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Interpreter/Value.cpp b/clang/lib/Interpreter/Value.cpp
index d4c9d51ffcb61..bbb58278b63ae 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 9ff9092524d21..1465f4d36337e 100644
--- a/clang/unittests/Interpreter/InterpreterTest.cpp
+++ b/clang/unittests/Interpreter/InterpreterTest.cpp
@@ -421,6 +421,57 @@ TEST_F(InterpreterTest, Value) {
   EXPECT_STREQ(prettyPrint.c_str(), "(D) (One) : unsigned int 1\n");
 }
 
+// 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);