librbd: fix segfault when removing non-existent group

Removing a non-existent group triggers a segfault in
librbd::mirror::GroupGetInfoRequest::send(). The issue is caused by a missing
return after finish(), which allows execution to fall through into
GroupGetInfoRequest::get_id() and access invalid memory.

Also, makesure to ignore ENOENT throughout the method Group::remove()
except at cls_client::dir_get_id()

Credits to Ilya Dryomov <idryomov@gmail.com>

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

diff --git a/qa/workunits/rbd/rbd_mirror_group_simple.sh b/qa/workunits/rbd/rbd_mirror_group_simple.sh
index fcbb8d52a0aaff096690fc8904b9b68afc378d74..a2ea0192d16f26ed8d9251637a3f566a1791850a 100755 (executable)
--- a/qa/workunits/rbd/rbd_mirror_group_simple.sh
+++ b/qa/workunits/rbd/rbd_mirror_group_simple.sh
@@ -1358,6 +1358,23 @@ test_empty_group()
   check_daemon_running "${secondary_cluster}"
 }
 
+# test removal of a non existing group
+declare -a test_remove_non_existing_group_1=("${CLUSTER2}" ${pool0} "group_blabla")
+
+test_remove_non_existing_group_scenarios=1
+
+test_remove_non_existing_group()
+{
+  local primary_cluster=$1
+  local pool=$2
+  local non_existing_group=$3
+
+  # command fails with the following message now
+  # rbd: remove error: (2) No such file or directory
+  # 2025-09-26T14:21:54.194+0530 7f94208f3d00 -1 librbd::api::Group: remove: error getting the group id: (2) No such file or directory
+  expect_failure "error getting the group id" rbd --cluster=${primary_cluster} group remove ${pool}/${non_existing_group}
+}
+
 #check that omap keys have been correctly deleted
 declare -a test_empty_group_omap_keys_1=("${CLUSTER2}" "${CLUSTER1}" "${pool0}")
 
@@ -3732,6 +3749,7 @@ run_all_tests()
 {
   run_test_all_scenarios test_empty_group
   run_test_all_scenarios test_empty_groups
+  run_test_all_scenarios test_remove_non_existing_group
   # This next test requires support for dynamic groups TODO
   # run_test_all_scenarios test_mirrored_group_remove_all_images
   # This next test also requires dynamic groups - TODO enable

diff --git a/src/librbd/api/Group.cc b/src/librbd/api/Group.cc
index 2264ad9b6f4b8a5022fbdaf433681d2e5cc93b27..6c46a81d414a0ef25fbdb557f327237bc15cf0f4 100644 (file)
--- a/src/librbd/api/Group.cc
+++ b/src/librbd/api/Group.cc
@@ -395,8 +395,9 @@ int Group<I>::remove(librados::IoCtx& io_ctx, const char *group_name)
   std::string group_id;
   int r = cls_client::dir_get_id(&io_ctx, RBD_GROUP_DIRECTORY,
                                 std::string(group_name), &group_id);
-  if (r < 0 && r != -ENOENT) {
-    lderr(cct) << "error getting id of group" << dendl;
+  if (r < 0) {
+    lderr(cct) << "error getting the group id: "
+               << cpp_strerror(r) << dendl;
     return r;
   }
 
@@ -417,7 +418,7 @@ int Group<I>::remove(librados::IoCtx& io_ctx, const char *group_name)
   }
 
   r = Mirror<I>::group_disable(io_ctx, group_name, false);
-  if (r < 0 && r != -EOPNOTSUPP) {
+  if (r < 0 && r != -ENOENT && r != -EOPNOTSUPP) {
     lderr(cct) << "failed to disable mirroring: " << cpp_strerror(r) << dendl;
     return r;
   }

diff --git a/src/librbd/mirror/GroupGetInfoRequest.cc b/src/librbd/mirror/GroupGetInfoRequest.cc
index 25997d2987f397aa204b40ac536e691e69000cc1..96936bb0e63242204b63aac21643cab2bcac7f65 100644 (file)
--- a/src/librbd/mirror/GroupGetInfoRequest.cc
+++ b/src/librbd/mirror/GroupGetInfoRequest.cc
@@ -29,6 +29,7 @@ void GroupGetInfoRequest<I>::send() {
   if (m_group_name.empty() && m_group_id.empty()) {
     lderr(cct) << "both group name and group id cannot be empty" << dendl;
     finish(-EINVAL);
+    return;
   }
 
   if (m_group_id.empty()) {