osd/ECBackend: fix iterator invalidation in omap_get

The removed_ranges erase loop in omap_get() called erase(out_it->first),
which invalidates out_it, then did ++out_it on the dead iterator (UB).
When adjacent keys were erased this skipped keys that should have been
removed:

    Expected: { "k01", "k05", "k06" }
    Actual:   { "k01", "k03", "k05", "k06" }

Extract the loop into a static remove_keys_in_ranges() helper so it can
be unit tested without an ObjectStore, and rewrite it as a range erase:
removed_ranges is a sorted map of non-overlapping [start, end) ranges.

Fixes: https://tracker.ceph.com/issues/78177
Signed-off-by: Sun Yuechi <sunyuechi@iscas.ac.cn>

diff --git a/src/osd/ECBackend.cc b/src/osd/ECBackend.cc
index d12ab9d8fc5656c36493cbeb9a0632ddc2b92896..37da9bc594bc6be6758e482e5f10f5301f06fbd9 100644 (file)
--- a/src/osd/ECBackend.cc
+++ b/src/osd/ECBackend.cc
@@ -1796,11 +1796,7 @@ int ECBackend::omap_get(
   }
 
   // Remove keys in removed_ranges
-  for (auto out_it = out->begin(); out_it != out->end(); ++out_it) {
-    if (should_be_removed(removed_ranges, out_it->first)) {
-      out->erase(out_it->first);
-    }
-  }
+  remove_keys_in_ranges(removed_ranges, out);
 
   // Apply updates in update_map
   for (const auto &[key, val_opt] : update_map) {
@@ -1875,3 +1871,12 @@ bool ECBackend::should_be_removed(
   // No ranges contain the key, return false
   return false;
 }
+
+void ECBackend::remove_keys_in_ranges(
+  const std::map<std::string, std::optional<std::string>>& removed_ranges,
+  std::map<std::string, ceph::buffer::list>* out) {
+  for (const auto& [start, end] : removed_ranges) {
+    out->erase(out->lower_bound(start),
+               end ? out->lower_bound(*end) : out->end());
+  }
+}

diff --git a/src/osd/ECBackend.h b/src/osd/ECBackend.h
index a5f013a0bb38a976700bbc41e88ff191091f35a5..30e1c2db5374c078128b62fcc527614ba7ba4d99 100644 (file)
--- a/src/osd/ECBackend.h
+++ b/src/osd/ECBackend.h
@@ -456,4 +456,9 @@ public:
     const std::map<std::string, std::optional<std::string>>& removed_ranges,
     const std::string_view key
   );
+
+  static void remove_keys_in_ranges(
+    const std::map<std::string, std::optional<std::string>>& removed_ranges,
+    std::map<std::string, ceph::buffer::list>* out
+  );
 };

diff --git a/src/test/osd/test_ec_omap_journal.cc b/src/test/osd/test_ec_omap_journal.cc
index 58cb193c0cf7408ed4a8bd8dd22f61cc24f9fdf5..45d4425aba471b3eb624954fbdc84a79b98c896e 100644 (file)
--- a/src/test/osd/test_ec_omap_journal.cc
+++ b/src/test/osd/test_ec_omap_journal.cc
@@ -17,6 +17,7 @@
 #include "test/unit.cc"
 
 #include "osd/ECOmapJournal.h"
+#include "osd/ECBackend.h"
 #include "common/dout.h"
 
 class MockDoutPrefixProvider : public DoutPrefixProvider {
@@ -1450,4 +1451,30 @@ TEST(ecomapjournal, has_omap_updates_after_remove_entry)
   
   // Should not have updates after removing all entries
   ASSERT_FALSE(journal.has_omap_updates(test_hoid));
-}
\ No newline at end of file
+}
+
+// A removed range spanning several adjacent keys erases all of them.
+TEST(ecbackend_remove_keys_in_ranges, removes_contiguous_block)
+{
+  std::map<std::string, ceph::buffer::list> out;
+  for (std::string_view k : {"k01", "k02", "k03", "k04", "k05", "k06"}) {
+    out[std::string(k)].append(k);
+  }
+  std::map<std::string, std::optional<std::string>> removed = {
+    {"k02", "k05"},  // -> erases k02, k03, k04
+  };
+
+  ECBackend::remove_keys_in_ranges(removed, &out);
+
+  std::vector<std::string> remaining;
+  for (const auto &[k, v] : out) {
+    remaining.push_back(k);
+  }
+  EXPECT_EQ((std::vector<std::string>{"k01", "k05", "k06"}), remaining);
+
+  // Surviving values must be untouched.
+  ASSERT_TRUE(out.contains("k05"));
+  ceph::buffer::list k05_bl;
+  k05_bl.append("k05");
+  EXPECT_TRUE(out["k05"].contents_equal(k05_bl));
+}