From 8d68fa2a8e0c292d74d5f4d0080e4db370f4685e Mon Sep 17 00:00:00 2001 From: Jason Dillaman Date: Wed, 10 Jan 2018 22:42:05 -0500 Subject: [PATCH] librbd: fixed memory leak and use-after-free in group snap API Signed-off-by: Jason Dillaman --- src/librbd/Operations.cc | 14 +++++++------- src/librbd/Operations.h | 15 +++++++-------- src/librbd/api/Group.cc | 40 ++++++++++++++++++++-------------------- 3 files changed, 34 insertions(+), 35 deletions(-) diff --git a/src/librbd/Operations.cc b/src/librbd/Operations.cc index 4d71e3d319b5a..1c052eb129a7e 100644 --- a/src/librbd/Operations.cc +++ b/src/librbd/Operations.cc @@ -667,7 +667,7 @@ void Operations::execute_resize(uint64_t size, bool allow_shrink, ProgressCon template int Operations::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::snap_create(const cls::rbd::SnapshotNamespace &snap_namespace template void Operations::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::execute_snap_create(const cls::rbd::SnapshotNamespace &snap_ template int Operations::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::execute_snap_rollback(const cls::rbd::SnapshotNamespace& sna template int Operations::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::snap_remove(const cls::rbd::SnapshotNamespace& snap_namespace template void Operations::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::execute_snap_rename(const uint64_t src_snap_id, template int Operations::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::execute_snap_protect(const cls::rbd::SnapshotNamespace& snap template int Operations::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 2b0c725b981b0..ff1238ff50e54 100644 --- 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 48c31dcb71d34..2e622a6910546 100644 --- 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 io_ctxs; std::vector 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::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::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::snap_remove(librados::IoCtx& group_ioctx, const char *group_name, CephContext *cct = (CephContext *)group_ioctx.cct(); librados::Rados rados(group_ioctx); - std::vector snaps; - std::vector 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::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 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::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; } -- 2.39.5