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 38e2f3dcb7da18cbbcb40692a22a799871644822..0d21c05c35a1fa220d9fb974a5993e5735f593e5 100644 (file)
--- 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 9b5ca1919329a7b4fc84ab0e73301e620813b63d..9060124cfc027e7b2f26b039890774d6737f2007 100644 (file)
--- 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 (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();
+}