From c2b2ba572aa94a27fbecbe25fd16345c92a5879e Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Tue, 11 Jun 2024 18:10:47 +0200 Subject: [PATCH] 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 (cherry picked from commit a3441a701963fef155fdda7d8f949d73bae26b69) --- src/librbd/api/DiffIterate.cc | 8 ++--- src/test/librbd/test_librbd.cc | 60 +++++++++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/librbd/api/DiffIterate.cc b/src/librbd/api/DiffIterate.cc index 8d19542b3f3b1..4655f643ba686 100644 --- a/src/librbd/api/DiffIterate.cc +++ b/src/librbd/api/DiffIterate.cc @@ -312,13 +312,13 @@ int DiffIterate::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 f4b4b3907bfe4..8d138cdec3432 100644 --- a/src/test/librbd/test_librbd.cc +++ b/src/test/librbd/test_librbd.cc @@ -7071,23 +7071,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 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); } @@ -7122,6 +7158,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); } -- 2.39.5