From: Ilya Dryomov <idryomov@gmail.com>
Date: Tue, 11 Jun 2024 16:10:47 +0000 (+0200)
Subject: librbd: diff-iterate shouldn't crash on an empty byte range
X-Git-Tag: v20.0.0~1707^2~1
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=a3441a701963fef155fdda7d8f949d73bae26b69;p=ceph.git

librbd: diff-iterate shouldn't crash on an empty byte range

Commit 0b5ba5fedf70 ("librbd/object_map: add support for ranged
diff-iterate") introduced a regression for the case when whole_object
parameter is set to true.  Despite DiffRequest being called into and
another DiffIterate potentially being spawned recursively, an empty
byte range previously happened to make it.

Bail on an empty byte range early just like we have always done on an
empty snap id range (i.e. when start and end versions are the same).

Fixes: https://tracker.ceph.com/issues/66418
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---

diff --git a/src/librbd/api/DiffIterate.cc b/src/librbd/api/DiffIterate.cc
index 9b400debd13a..8ddcabd7c0b2 100644
--- a/src/librbd/api/DiffIterate.cc
+++ b/src/librbd/api/DiffIterate.cc
@@ -312,13 +312,13 @@ int DiffIterate<I>::execute() {
   if (from_snap_id == CEPH_NOSNAP) {
     return -ENOENT;
   }
-  if (from_snap_id == end_snap_id) {
+  if (from_snap_id > end_snap_id) {
+    return -EINVAL;
+  }
+  if (from_snap_id == end_snap_id || m_length == 0) {
     // no diff.
     return 0;
   }
-  if (from_snap_id >= end_snap_id) {
-    return -EINVAL;
-  }
 
   int r;
   bool fast_diff_enabled = false;
diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc
index 5e82beb039d4..d4f0396ec3d1 100644
--- a/src/test/librbd/test_librbd.cc
+++ b/src/test/librbd/test_librbd.cc
@@ -8529,23 +8529,59 @@ TYPED_TEST(DiffIterateTest, DiffIterateUnalignedSmall)
   {
     librbd::RBD rbd;
     librbd::Image image;
-    int order = 0;
+    int order = 22;
     std::string name = this->get_temp_image_name();
-    ssize_t size = 10 << 20;
+    ssize_t data_end = 8 << 20;
 
-    ASSERT_EQ(0, create_image_pp(rbd, ioctx, name.c_str(), size, &order));
+    ASSERT_EQ(0, create_image_pp(rbd, ioctx, name.c_str(),
+                                 data_end + (2 << 20), &order));
     ASSERT_EQ(0, rbd.open(ioctx, image, name.c_str(), NULL));
 
     ceph::bufferlist bl;
-    bl.append(std::string(size, '1'));
-    ASSERT_EQ(size, image.write(0, size, bl));
+    bl.append(std::string(data_end, '1'));
+    ASSERT_EQ(data_end, image.write(0, data_end, bl));
 
     std::vector<diff_extent> extents;
+    ASSERT_EQ(0, image.diff_iterate2(NULL, 0, 0, true,
+                                     this->whole_object, vector_iterate_cb,
+                                     &extents));
+    ASSERT_EQ(0u, extents.size());
+
     ASSERT_EQ(0, image.diff_iterate2(NULL, 5000005, 1234, true,
                                      this->whole_object, vector_iterate_cb,
                                      &extents));
     ASSERT_EQ(1u, extents.size());
     ASSERT_EQ(diff_extent(5000005, 1234, true, 0), extents[0]);
+    extents.clear();
+
+    ASSERT_EQ(0, image.diff_iterate2(NULL, data_end - 1, 0, true,
+                                     this->whole_object, vector_iterate_cb,
+                                     &extents));
+    ASSERT_EQ(0u, extents.size());
+
+    ASSERT_EQ(0, image.diff_iterate2(NULL, data_end - 1, 1, true,
+                                     this->whole_object, vector_iterate_cb,
+                                     &extents));
+    ASSERT_EQ(1u, extents.size());
+    ASSERT_EQ(diff_extent(data_end - 1, 1, true, 0), extents[0]);
+    extents.clear();
+
+    ASSERT_EQ(0, image.diff_iterate2(NULL, data_end - 1, 2, true,
+                                     this->whole_object, vector_iterate_cb,
+                                     &extents));
+    ASSERT_EQ(1u, extents.size());
+    ASSERT_EQ(diff_extent(data_end - 1, 1, true, 0), extents[0]);
+    extents.clear();
+
+    ASSERT_EQ(0, image.diff_iterate2(NULL, data_end, 0, true,
+                                     this->whole_object, vector_iterate_cb,
+                                     &extents));
+    ASSERT_EQ(0u, extents.size());
+
+    ASSERT_EQ(0, image.diff_iterate2(NULL, data_end, 1, true,
+                                     this->whole_object, vector_iterate_cb,
+                                     &extents));
+    ASSERT_EQ(0u, extents.size());
 
     ASSERT_PASSED(this->validate_object_map, image);
   }
@@ -8580,6 +8616,20 @@ TYPED_TEST(DiffIterateTest, DiffIterateUnaligned)
     ASSERT_EQ(diff_extent(8376263, 12345, true, 0), extents[0]);
     ASSERT_EQ(diff_extent(8388608, 4194304, true, 0), extents[1]);
     ASSERT_EQ(diff_extent(12582912, 54321, true, 0), extents[2]);
+    extents.clear();
+
+    // length is clipped up to end
+    ASSERT_EQ(0, image.diff_iterate2(NULL, size - 1, size, true,
+                                     this->whole_object, vector_iterate_cb,
+                                     &extents));
+    ASSERT_EQ(1u, extents.size());
+    ASSERT_EQ(diff_extent(size - 1, 1, true, 0), extents[0]);
+    extents.clear();
+
+    // offset past end
+    ASSERT_EQ(-EINVAL, image.diff_iterate2(NULL, size, size, true,
+                                           this->whole_object,
+                                           vector_iterate_cb, &extents));
 
     ASSERT_PASSED(this->validate_object_map, image);
   }