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>
(cherry picked from commit aaa0bf7ce8a31a84fb95ad1c01dbdb0752be9543)

diff --git a/src/rgw/driver/rados/rgw_rados.h b/src/rgw/driver/rados/rgw_rados.h
index 7a67bb73fe539b92532f891b808cadceb7d2b6bf..98f357529007e1f9a226bb35cbf25e3c3126bd1d 100644 (file)
--- a/src/rgw/driver/rados/rgw_rados.h
+++ b/src/rgw/driver/rados/rgw_rados.h
@@ -196,11 +196,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;
@@ -214,6 +209,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 2c6c45ad99ca5a1c1d2086b763c9d02f24a020da..bc9b9c6b4496a16eb6c9a819b55e36a9cc494624 100644 (file)
--- a/src/test/rgw/CMakeLists.txt
+++ b/src/test/rgw/CMakeLists.txt
@@ -322,6 +322,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 (file)
index 0000000..3fb1232
--- /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();
+}