librbd: fixed memory leak and use-after-free in group snap API

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/Operations.cc b/src/librbd/Operations.cc
index 4d71e3d319b5a6e240a0f7fc0723f945e79eb12a..1c052eb129a7e43e8981e4c8cd6b3f06096cbfe9 100644 (file)
--- a/src/librbd/Operations.cc
+++ b/src/librbd/Operations.cc
@@ -667,7 +667,7 @@ void Operations<I>::execute_resize(uint64_t size, bool allow_shrink, ProgressCon
 
 template <typename I>
 int Operations<I>::snap_create(const cls::rbd::SnapshotNamespace &snap_namespace,
-                              const char *snap_name) {
+                              const std::string& snap_name) {
   if (m_image_ctx.read_only) {
     return -EROFS;
   }
@@ -691,7 +691,7 @@ int Operations<I>::snap_create(const cls::rbd::SnapshotNamespace &snap_namespace
 
 template <typename I>
 void Operations<I>::snap_create(const cls::rbd::SnapshotNamespace &snap_namespace,
-                               const char *snap_name,
+                               const std::string& snap_name,
                                Context *on_finish) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 5) << this << " " << __func__ << ": snap_name=" << snap_name
@@ -751,7 +751,7 @@ void Operations<I>::execute_snap_create(const cls::rbd::SnapshotNamespace &snap_
 
 template <typename I>
 int Operations<I>::snap_rollback(const cls::rbd::SnapshotNamespace& snap_namespace,
-                                const char *snap_name,
+                                const std::string& snap_name,
                                  ProgressContext& prog_ctx) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 5) << this << " " << __func__ << ": snap_name=" << snap_name
@@ -831,7 +831,7 @@ void Operations<I>::execute_snap_rollback(const cls::rbd::SnapshotNamespace& sna
 
 template <typename I>
 int Operations<I>::snap_remove(const cls::rbd::SnapshotNamespace& snap_namespace,
-                              const char *snap_name) {
+                              const std::string& snap_name) {
   if (m_image_ctx.read_only) {
     return -EROFS;
   }
@@ -855,7 +855,7 @@ int Operations<I>::snap_remove(const cls::rbd::SnapshotNamespace& snap_namespace
 
 template <typename I>
 void Operations<I>::snap_remove(const cls::rbd::SnapshotNamespace& snap_namespace,
-                               const char *snap_name,
+                               const std::string& snap_name,
                                Context *on_finish) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 5) << this << " " << __func__ << ": snap_name=" << snap_name
@@ -1026,7 +1026,7 @@ void Operations<I>::execute_snap_rename(const uint64_t src_snap_id,
 
 template <typename I>
 int Operations<I>::snap_protect(const cls::rbd::SnapshotNamespace& snap_namespace,
-                               const char *snap_name) {
+                               const std::string& snap_name) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 5) << this << " " << __func__ << ": snap_name=" << snap_name
                 << dendl;
@@ -1121,7 +1121,7 @@ void Operations<I>::execute_snap_protect(const cls::rbd::SnapshotNamespace& snap
 
 template <typename I>
 int Operations<I>::snap_unprotect(const cls::rbd::SnapshotNamespace& snap_namespace,
-                                 const char *snap_name) {
+                                 const std::string& snap_name) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 5) << this << " " << __func__ << ": snap_name=" << snap_name
                 << dendl;

diff --git a/src/librbd/Operations.h b/src/librbd/Operations.h
index 2b0c725b981b02977954a3fc5ac930a6e92fb9a5..ff1238ff50e549e28711c7284730a29723318cfb 100644 (file)
--- a/src/librbd/Operations.h
+++ b/src/librbd/Operations.h
@@ -45,26 +45,25 @@ public:
                       Context *on_finish, uint64_t journal_op_tid);
 
   int snap_create(const cls::rbd::SnapshotNamespace &snap_namespace,
-                 const char *snap_name);
+                 const std::string& snap_name);
   void snap_create(const cls::rbd::SnapshotNamespace &snap_namespace,
-                  const char *snap_name,
-                  Context *on_finish);
+                  const std::string& snap_name, Context *on_finish);
   void execute_snap_create(const cls::rbd::SnapshotNamespace &snap_namespace,
                           const std::string &snap_name,
                           Context *on_finish,
                            uint64_t journal_op_tid, bool skip_object_map);
 
   int snap_rollback(const cls::rbd::SnapshotNamespace& snap_namespace,
-                   const char *snap_name,
+                   const std::string& snap_name,
                    ProgressContext& prog_ctx);
   void execute_snap_rollback(const cls::rbd::SnapshotNamespace& snap_namespace,
                             const std::string &snap_name,
                              ProgressContext& prog_ctx, Context *on_finish);
 
   int snap_remove(const cls::rbd::SnapshotNamespace& snap_namespace,
-                 const char *snap_name);
+                 const std::string& snap_name);
   void snap_remove(const cls::rbd::SnapshotNamespace& snap_namespace,
-                  const char *snap_name,
+                  const std::string& snap_name,
                   Context *on_finish);
   void execute_snap_remove(const cls::rbd::SnapshotNamespace& snap_namespace,
                           const std::string &snap_name,
@@ -76,13 +75,13 @@ public:
                            Context *on_finish);
 
   int snap_protect(const cls::rbd::SnapshotNamespace& snap_namespace,
-                  const char *snap_name);
+                  const std::string& snap_name);
   void execute_snap_protect(const cls::rbd::SnapshotNamespace& snap_namespace,
                            const std::string &snap_name,
                            Context *on_finish);
 
   int snap_unprotect(const cls::rbd::SnapshotNamespace& snap_namespace,
-                    const char *snap_name);
+                    const std::string& snap_name);
   void execute_snap_unprotect(const cls::rbd::SnapshotNamespace& snap_namespace,
                              const std::string &snap_name,
                              Context *on_finish);

diff --git a/src/librbd/api/Group.cc b/src/librbd/api/Group.cc
index 48c31dcb71d341de9aaaf03d91c6f9defe956d3f..2e622a691054655733c59b2ca632d840ea34e81e 100644 (file)
--- a/src/librbd/api/Group.cc
+++ b/src/librbd/api/Group.cc
@@ -204,8 +204,6 @@ int group_snap_remove_by_record(librados::IoCtx& group_ioctx,
   std::vector<librbd::IoCtx*> io_ctxs;
   std::vector<librbd::ImageCtx*> ictxs;
 
-  std::string image_snap_name;
-
   cls::rbd::SnapshotNamespace ne;
 
   ldout(cct, 20) << "Removing snapshots" << dendl;
@@ -252,8 +250,6 @@ int group_snap_remove_by_record(librados::IoCtx& group_ioctx,
 
   for (int i = 0; i < snap_count; ++i) {
     ImageCtx *ictx = ictxs[i];
-    ldout(cct, 20) << "Removing individual snapshot with name: " <<
-      image_snap_name << dendl;
     on_finishes[i] = new C_SaferCond;
 
     std::string snap_name;
@@ -261,16 +257,24 @@ int group_snap_remove_by_record(librados::IoCtx& group_ioctx,
     snap_t snap_id = ictx->get_snap_id(ne, "");
     r = ictx->get_snap_name(snap_id, &snap_name);
     ictx->snap_lock.put_read();
+
     if (r >= 0) {
-      ictx->operations->snap_remove(ne, snap_name.c_str(), on_finishes[i]);
+      ldout(cct, 20) << "removing individual snapshot from image " << ictx->name
+                     << dendl;
+      ictx->operations->snap_remove(ne, snap_name, on_finishes[i]);
+    } else {
+      // We are ok to ignore missing image snapshots. The snapshot could have
+      // been inconsistent in the first place.
+      on_finishes[i]->complete(0);
     }
-    // We are ok to ignore missing image snapshots. The snapshot could have been inconsistent in the first place.
   }
 
   for (int i = 0; i < snap_count; ++i) {
     r = on_finishes[i]->wait();
     delete on_finishes[i];
-    if (r < 0 && r != -ENOENT) { // if previous attempts to remove this snapshot failed then the image's snapshot may not exist
+    if (r < 0 && r != -ENOENT) {
+      // if previous attempts to remove this snapshot failed then the image's
+      // snapshot may not exist
       lderr(cct) << "Failed deleting image snapshot. Ret code: " << r << dendl;
       ret_code = r;
     }
@@ -280,8 +284,8 @@ int group_snap_remove_by_record(librados::IoCtx& group_ioctx,
     goto finish;
   }
 
-  ldout(cct, 20) << "Removed images snapshots removing snapshot record." <<
-    dendl;
+  ldout(cct, 20) << "Removed images snapshots removing snapshot record."
+                 << dendl;
 
   r = cls_client::group_snap_remove(&group_ioctx, group_header_oid,
       group_snap.id);
@@ -710,9 +714,8 @@ int Group<I>::snap_create(librados::IoCtx& group_ioctx,
   }
 
   for (auto image: images) {
-    librbd::IoCtx* image_io_ctx = new librbd::IoCtx;
-
-    r = rados.ioctx_create2(image.spec.pool_id, *image_io_ctx);
+    librbd::IoCtx image_io_ctx;
+    r = rados.ioctx_create2(image.spec.pool_id, image_io_ctx);
     if (r < 0) {
       ldout(cct, 1) << "Failed to create io context for image" << dendl;
     }
@@ -720,7 +723,7 @@ int Group<I>::snap_create(librados::IoCtx& group_ioctx,
     ldout(cct, 20) << "Openning image with id" << image.spec.image_id << dendl;
 
     librbd::ImageCtx* image_ctx = new ImageCtx("", image.spec.image_id.c_str(),
-                                              nullptr, *image_io_ctx, false);
+                                              nullptr, image_io_ctx, false);
 
     C_SaferCond* on_finish = new C_SaferCond;
 
@@ -886,9 +889,6 @@ int Group<I>::snap_remove(librados::IoCtx& group_ioctx, const char *group_name,
   CephContext *cct = (CephContext *)group_ioctx.cct();
   librados::Rados rados(group_ioctx);
 
-  std::vector<cls::rbd::GroupSnapshot> snaps;
-  std::vector<C_SaferCond*> on_finishes;
-
   string group_id;
   int r = cls_client::dir_get_id(&group_ioctx, RBD_GROUP_DIRECTORY,
                                 group_name, &group_id);
@@ -898,12 +898,12 @@ int Group<I>::snap_remove(librados::IoCtx& group_ioctx, const char *group_name,
               << dendl;
     return r;
   }
-  string group_header_oid = util::group_header_name(group_id);
-
+  std::vector<cls::rbd::GroupSnapshot> snaps;
   r = group_snap_list(group_ioctx, group_name, &snaps);
   if (r < 0) {
     return r;
   }
+
   cls::rbd::GroupSnapshot *group_snap = nullptr;
   for (auto &snap : snaps) {
     if (snap.name == string(snap_name)) {
@@ -915,9 +915,9 @@ int Group<I>::snap_remove(librados::IoCtx& group_ioctx, const char *group_name,
     return -ENOENT;
   }
 
+  string group_header_oid = util::group_header_name(group_id);
   r = group_snap_remove_by_record(group_ioctx, *group_snap, group_id,
-      group_header_oid);
-
+                                  group_header_oid);
   return r;
 }