From: Oguzhan Ozmen <oozmen@bloomberg.net>
Date: Thu, 4 Dec 2025 00:53:02 +0000 (+0000)
Subject: RGW: remove custom copy constructor for RGWObjectCtx and enforce no copy/move
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=aaa0bf7ce8a31a84fb95ad1c01dbdb0752be9543;p=ceph.git

RGW: remove custom copy constructor for RGWObjectCtx and enforce no copy/move

The custom copy constructor allowed RGWObjectCtx to be copied without
locking the source, enabling concurrent modifications of objs_state
during std::_Rb_tree::_M_copy(). This could corrupt the map and cause
crashes during copy or destruction. Since RGWObjectCtx is not meant to
be copied or moved, remove the custom copy constructor and rely on the
type's natural non-copyable semantics, with static assertions enforcing
the contract.

Related to: https://tracker.ceph.com/issues/62063
Signed-off-by: Oguzhan Ozmen <oozmen@bloomberg.net>
---

diff --git a/src/rgw/driver/rados/rgw_rados.h b/src/rgw/driver/rados/rgw_rados.h
index 38e2f3dcb7d..0d21c05c35a 100644
--- a/src/rgw/driver/rados/rgw_rados.h
+++ b/src/rgw/driver/rados/rgw_rados.h
@@ -173,11 +173,6 @@ class RGWObjectCtx {
   std::map<rgw_obj, RGWObjStateManifest> objs_state;
 public:
   explicit RGWObjectCtx(rgw::sal::Driver* _driver) : driver(_driver) {}
-  RGWObjectCtx(RGWObjectCtx& _o) {
-    std::unique_lock wl{lock};
-    this->driver = _o.driver;
-    this->objs_state = _o.objs_state;
-  }
 
   rgw::sal::Driver* get_driver() {
     return driver;
@@ -191,6 +186,15 @@ public:
   void invalidate(const rgw_obj& obj);
 };
 
+// Enforce at compile time that RGWObjectCtx is neither copyable nor movable
+static_assert(!std::is_copy_constructible<RGWObjectCtx>::value,
+              "RGWObjectCtx must not be copy-constructed");
+static_assert(!std::is_copy_assignable<RGWObjectCtx>::value,
+              "RGWObjectCtx must not be copy-assigned");
+static_assert(!std::is_move_constructible<RGWObjectCtx>::value,
+              "RGWObjectCtx must not be move-constructed");
+static_assert(!std::is_move_assignable<RGWObjectCtx>::value,
+              "RGWObjectCtx must not be move-assigned");
 
 struct RGWRawObjState {
   rgw_raw_obj obj;
diff --git a/src/test/rgw/CMakeLists.txt b/src/test/rgw/CMakeLists.txt
index 9b5ca191932..9060124cfc0 100644
--- a/src/test/rgw/CMakeLists.txt
+++ b/src/test/rgw/CMakeLists.txt
@@ -334,6 +334,17 @@ add_executable(unittest_rgw_shard_io test_rgw_shard_io.cc)
 add_ceph_unittest(unittest_rgw_shard_io)
 target_link_libraries(unittest_rgw_shard_io ${rgw_libs} unit-main ${UNITTEST_LIBS})
 
+# unittest_rgw_object_ctx
+add_executable(unittest_rgw_object_ctx test_rgw_object_ctx.cc)
+add_ceph_unittest(unittest_rgw_object_ctx)
+target_include_directories(unittest_rgw_object_ctx
+  SYSTEM PRIVATE
+    ${CMAKE_SOURCE_DIR}/src/rgw
+    ${CMAKE_SOURCE_DIR}/src/rgw/driver/rados
+    ${CMAKE_SOURCE_DIR}/src/s3select/rapidjson/include
+)
+target_link_libraries(unittest_rgw_object_ctx ${rgw_libs})
+
 add_ceph_test(test-ceph-diff-sorted.sh
   ${CMAKE_CURRENT_SOURCE_DIR}/test-ceph-diff-sorted.sh)
 
diff --git a/src/test/rgw/test_rgw_object_ctx.cc b/src/test/rgw/test_rgw_object_ctx.cc
new file mode 100644
index 00000000000..3fb12325b1a
--- /dev/null
+++ b/src/test/rgw/test_rgw_object_ctx.cc
@@ -0,0 +1,75 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:nil -*-
+// vim: ts=8 sw=2 sts=2 expandtab ft=cpp
+
+#include <gtest/gtest.h>
+
+#include "rgw_rados.h"
+
+TEST(TestRGWObjectCtx, create_and_destroy_many)
+{
+  // Create and destroy many RGWObjectCtx instances and
+  // for each one exercise its API from multiple threads.
+  //
+  // If there are lifetime or concurrency bugs in use of RGWObjectCtx,
+  // this might help surfacing them (perhaps under ASan/TSan/valgrind runs).
+
+  constexpr int test_duration_secs = 10;
+  constexpr int num_concurrent_requests = 100;
+
+  std::atomic<bool> stop{false};
+  std::vector<std::thread> threads;
+
+  // Pre-generate objects handled by the object contexts
+  std::vector<rgw_obj> objs;
+  objs.reserve(num_concurrent_requests);
+  for (int i = 0; i < num_concurrent_requests; ++i) {
+    objs.emplace_back(
+        rgw_bucket("", "dummy_bucket" + std::to_string(i)),
+        rgw_obj_key("dummy_key" + std::to_string(i)));
+  }
+
+  // Spawn threads that repeatedly construct/destroy RGWObjectCtx objects
+  // and exercise its methods on random objects in random order
+  for (int t = 0; t < num_concurrent_requests; ++t) {
+    threads.emplace_back([&, t]() {
+      std::mt19937_64 rng{
+        static_cast<std::mt19937_64::result_type>(
+          std::chrono::steady_clock::now().time_since_epoch().count() + t
+        )
+      };
+      std::uniform_int_distribution<int> obj_dist(0, num_concurrent_requests - 1);
+      std::uniform_int_distribution<int> coin(0, 1);
+
+      while (!stop.load(std::memory_order_relaxed)) {
+        RGWObjectCtx ctx(nullptr);
+        const auto& o = objs[obj_dist(rng)];
+
+        // randomly choose which operations to do and in which order
+        // to simulate a real-life pattern.
+        if (coin(rng)) {
+          ctx.set_atomic(o, true);
+        }
+        if (coin(rng)) {
+          ctx.set_compressed(o);
+        }
+        if (coin(rng)) {
+          ctx.set_prefetch_data(o);
+        }
+        if (coin(rng)) {
+          (void)ctx.get_state(o);
+        }
+        if (coin(rng)) {
+          ctx.invalidate(o);
+        }
+        if (coin(rng)) {
+          (void) ctx.get_driver();
+        }
+      }
+    });
+  }
+
+  // Let the threads run for some time
+  std::this_thread::sleep_for(std::chrono::seconds(test_duration_secs));
+  stop.store(true);
+  for (auto& th : threads) th.join();
+}