From 0620e58db11ff1e9f7206e24238abf6242c3e59a Mon Sep 17 00:00:00 2001 From: Soumya Koduri Date: Thu, 23 Jun 2022 23:03:18 +0530 Subject: [PATCH] rgw/dbstore: Fix crash in delete_stale_objs Fix a race between RemoveBucket and delete_stale_objs operations by using shared_ptr to add reference to DB Ops. Fixes:https://tracker.ceph.com/issues/55828 Signed-off-by: Soumya Koduri --- src/rgw/store/dbstore/common/dbstore.cc | 14 ++-- src/rgw/store/dbstore/common/dbstore.h | 54 +++++++-------- src/rgw/store/dbstore/sqlite/sqliteDB.cc | 86 +++++++----------------- src/rgw/store/dbstore/sqlite/sqliteDB.h | 2 - 4 files changed, 55 insertions(+), 101 deletions(-) diff --git a/src/rgw/store/dbstore/common/dbstore.cc b/src/rgw/store/dbstore/common/dbstore.cc index 692d954b7813..2cd970925084 100644 --- a/src/rgw/store/dbstore/common/dbstore.cc +++ b/src/rgw/store/dbstore/common/dbstore.cc @@ -82,8 +82,6 @@ int DB::Destroy(const DoutPrefixProvider *dpp) closeDB(dpp); - FreeDBOps(dpp); - ldpp_dout(dpp, 20)<<"DB successfully destroyed - name:" \ < DB::getDBOp(const DoutPrefixProvider *dpp, std::string_view Op, const DBOpParams *params) { if (!Op.compare("InsertUser")) @@ -138,7 +136,7 @@ DBOp *DB::getDBOp(const DoutPrefixProvider *dpp, std::string_view Op, ldpp_dout(dpp, 30)<<"No objectmap found for bucket: " \ <op.bucket.info.bucket.name << dendl; /* not found */ - return NULL; + return nullptr; } Ob = iter->second; @@ -164,7 +162,7 @@ DBOp *DB::getDBOp(const DoutPrefixProvider *dpp, std::string_view Op, if (!Op.compare("DeleteStaleObjectData")) return Ob->DeleteStaleObjectData; - return NULL; + return nullptr; } int DB::objectmapInsert(const DoutPrefixProvider *dpp, string bucket, class ObjectOp* ptr) @@ -198,7 +196,6 @@ int DB::objectmapInsert(const DoutPrefixProvider *dpp, string bucket, class Obje int DB::objectmapDelete(const DoutPrefixProvider *dpp, string bucket) { map::iterator iter; - class ObjectOp *Ob; const std::lock_guard lk(mtx); iter = DB::objectmap.find(bucket); @@ -212,9 +209,6 @@ int DB::objectmapDelete(const DoutPrefixProvider *dpp, string bucket) return 0; } - Ob = (class ObjectOp*) (iter->second); - Ob->FreeObjectOps(dpp); - DB::objectmap.erase(iter); return 0; @@ -243,7 +237,7 @@ out: int DB::ProcessOp(const DoutPrefixProvider *dpp, std::string_view Op, DBOpParams *params) { int ret = -1; - class DBOp *db_op; + shared_ptr db_op; db_op = getDBOp(dpp, Op, params); diff --git a/src/rgw/store/dbstore/common/dbstore.h b/src/rgw/store/dbstore/common/dbstore.h index 10056dfa452c..d35b1bc0f3db 100644 --- a/src/rgw/store/dbstore/common/dbstore.h +++ b/src/rgw/store/dbstore/common/dbstore.h @@ -341,21 +341,21 @@ struct DBOpPrepareParams { }; struct DBOps { - class InsertUserOp *InsertUser; - class RemoveUserOp *RemoveUser; - class GetUserOp *GetUser; - class InsertBucketOp *InsertBucket; - class UpdateBucketOp *UpdateBucket; - class RemoveBucketOp *RemoveBucket; - class GetBucketOp *GetBucket; - class ListUserBucketsOp *ListUserBuckets; - class InsertLCEntryOp *InsertLCEntry; - class RemoveLCEntryOp *RemoveLCEntry; - class GetLCEntryOp *GetLCEntry; - class ListLCEntriesOp *ListLCEntries; - class InsertLCHeadOp *InsertLCHead; - class RemoveLCHeadOp *RemoveLCHead; - class GetLCHeadOp *GetLCHead; + std::shared_ptr InsertUser; + std::shared_ptr RemoveUser; + std::shared_ptr GetUser; + std::shared_ptr InsertBucket; + std::shared_ptr UpdateBucket; + std::shared_ptr RemoveBucket; + std::shared_ptr GetBucket; + std::shared_ptr ListUserBuckets; + std::shared_ptr InsertLCEntry; + std::shared_ptr RemoveLCEntry; + std::shared_ptr GetLCEntry; + std::shared_ptr ListLCEntries; + std::shared_ptr InsertLCHead; + std::shared_ptr RemoveLCHead; + std::shared_ptr GetLCHead; }; class ObjectOp { @@ -364,19 +364,18 @@ class ObjectOp { virtual ~ObjectOp() {} - class PutObjectOp *PutObject; - class DeleteObjectOp *DeleteObject; - class GetObjectOp *GetObject; - class UpdateObjectOp *UpdateObject; - class ListBucketObjectsOp *ListBucketObjects; - class PutObjectDataOp *PutObjectData; - class UpdateObjectDataOp *UpdateObjectData; - class GetObjectDataOp *GetObjectData; - class DeleteObjectDataOp *DeleteObjectData; - class DeleteStaleObjectDataOp *DeleteStaleObjectData; + std::shared_ptr PutObject; + std::shared_ptr DeleteObject; + std::shared_ptr GetObject; + std::shared_ptr UpdateObject; + std::shared_ptr ListBucketObjects; + std::shared_ptr PutObjectData; + std::shared_ptr UpdateObjectData; + std::shared_ptr GetObjectData; + std::shared_ptr DeleteObjectData; + std::shared_ptr DeleteStaleObjectData; virtual int InitializeObjectOps(std::string db_name, const DoutPrefixProvider *dpp) { return 0; } - virtual int FreeObjectOps(const DoutPrefixProvider *dpp) { return 0; } }; class DBOp { @@ -1512,7 +1511,7 @@ class DB { int InitializeParams(const DoutPrefixProvider *dpp, DBOpParams *params); int ProcessOp(const DoutPrefixProvider *dpp, std::string_view Op, DBOpParams *params); - DBOp* getDBOp(const DoutPrefixProvider *dpp, std::string_view Op, const DBOpParams *params); + std::shared_ptr getDBOp(const DoutPrefixProvider *dpp, std::string_view Op, const DBOpParams *params); int objectmapInsert(const DoutPrefixProvider *dpp, std::string bucket, class ObjectOp* ptr); int objectmapDelete(const DoutPrefixProvider *dpp, std::string bucket); @@ -1521,7 +1520,6 @@ class DB { virtual int closeDB(const DoutPrefixProvider *dpp) { return 0; } virtual int createTables(const DoutPrefixProvider *dpp) { return 0; } virtual int InitializeDBOps(const DoutPrefixProvider *dpp) { return 0; } - virtual int FreeDBOps(const DoutPrefixProvider *dpp) { return 0; } virtual int InitPrepareParams(const DoutPrefixProvider *dpp, DBOpPrepareParams &p_params, DBOpParams* params) = 0; diff --git a/src/rgw/store/dbstore/sqlite/sqliteDB.cc b/src/rgw/store/dbstore/sqlite/sqliteDB.cc index efbad31121d8..e89ff29a8a9a 100644 --- a/src/rgw/store/dbstore/sqlite/sqliteDB.cc +++ b/src/rgw/store/dbstore/sqlite/sqliteDB.cc @@ -591,42 +591,21 @@ static int list_lc_head(const DoutPrefixProvider *dpp, DBOpInfo &op, sqlite3_stm int SQLiteDB::InitializeDBOps(const DoutPrefixProvider *dpp) { (void)createTables(dpp); - dbops.InsertUser = new SQLInsertUser(&this->db, this->getDBname(), cct); - dbops.RemoveUser = new SQLRemoveUser(&this->db, this->getDBname(), cct); - dbops.GetUser = new SQLGetUser(&this->db, this->getDBname(), cct); - dbops.InsertBucket = new SQLInsertBucket(&this->db, this->getDBname(), cct); - dbops.UpdateBucket = new SQLUpdateBucket(&this->db, this->getDBname(), cct); - dbops.RemoveBucket = new SQLRemoveBucket(&this->db, this->getDBname(), cct); - dbops.GetBucket = new SQLGetBucket(&this->db, this->getDBname(), cct); - dbops.ListUserBuckets = new SQLListUserBuckets(&this->db, this->getDBname(), cct); - dbops.InsertLCEntry = new SQLInsertLCEntry(&this->db, this->getDBname(), cct); - dbops.RemoveLCEntry = new SQLRemoveLCEntry(&this->db, this->getDBname(), cct); - dbops.GetLCEntry = new SQLGetLCEntry(&this->db, this->getDBname(), cct); - dbops.ListLCEntries = new SQLListLCEntries(&this->db, this->getDBname(), cct); - dbops.InsertLCHead = new SQLInsertLCHead(&this->db, this->getDBname(), cct); - dbops.RemoveLCHead = new SQLRemoveLCHead(&this->db, this->getDBname(), cct); - dbops.GetLCHead = new SQLGetLCHead(&this->db, this->getDBname(), cct); - - return 0; -} - -int SQLiteDB::FreeDBOps(const DoutPrefixProvider *dpp) -{ - delete dbops.InsertUser; - delete dbops.RemoveUser; - delete dbops.GetUser; - delete dbops.InsertBucket; - delete dbops.UpdateBucket; - delete dbops.RemoveBucket; - delete dbops.GetBucket; - delete dbops.ListUserBuckets; - delete dbops.InsertLCEntry; - delete dbops.RemoveLCEntry; - delete dbops.GetLCEntry; - delete dbops.ListLCEntries; - delete dbops.InsertLCHead; - delete dbops.RemoveLCHead; - delete dbops.GetLCHead; + dbops.InsertUser = make_shared(&this->db, this->getDBname(), cct); + dbops.RemoveUser = make_shared(&this->db, this->getDBname(), cct); + dbops.GetUser = make_shared(&this->db, this->getDBname(), cct); + dbops.InsertBucket = make_shared(&this->db, this->getDBname(), cct); + dbops.UpdateBucket = make_shared(&this->db, this->getDBname(), cct); + dbops.RemoveBucket = make_shared(&this->db, this->getDBname(), cct); + dbops.GetBucket = make_shared(&this->db, this->getDBname(), cct); + dbops.ListUserBuckets = make_shared(&this->db, this->getDBname(), cct); + dbops.InsertLCEntry = make_shared(&this->db, this->getDBname(), cct); + dbops.RemoveLCEntry = make_shared(&this->db, this->getDBname(), cct); + dbops.GetLCEntry = make_shared(&this->db, this->getDBname(), cct); + dbops.ListLCEntries = make_shared(&this->db, this->getDBname(), cct); + dbops.InsertLCHead = make_shared(&this->db, this->getDBname(), cct); + dbops.RemoveLCHead = make_shared(&this->db, this->getDBname(), cct); + dbops.GetLCHead = make_shared(&this->db, this->getDBname(), cct); return 0; } @@ -1062,31 +1041,16 @@ int SQLiteDB::ListAllObjects(const DoutPrefixProvider *dpp, DBOpParams *params) int SQLObjectOp::InitializeObjectOps(string db_name, const DoutPrefixProvider *dpp) { - PutObject = new SQLPutObject(sdb, db_name, cct); - DeleteObject = new SQLDeleteObject(sdb, db_name, cct); - GetObject = new SQLGetObject(sdb, db_name, cct); - UpdateObject = new SQLUpdateObject(sdb, db_name, cct); - ListBucketObjects = new SQLListBucketObjects(sdb, db_name, cct); - PutObjectData = new SQLPutObjectData(sdb, db_name, cct); - UpdateObjectData = new SQLUpdateObjectData(sdb, db_name, cct); - GetObjectData = new SQLGetObjectData(sdb, db_name, cct); - DeleteObjectData = new SQLDeleteObjectData(sdb, db_name, cct); - DeleteStaleObjectData = new SQLDeleteStaleObjectData(sdb, db_name, cct); - - return 0; -} - -int SQLObjectOp::FreeObjectOps(const DoutPrefixProvider *dpp) -{ - delete PutObject; - delete DeleteObject; - delete GetObject; - delete UpdateObject; - delete PutObjectData; - delete UpdateObjectData; - delete GetObjectData; - delete DeleteObjectData; - delete DeleteStaleObjectData; + PutObject = make_shared(sdb, db_name, cct); + DeleteObject = make_shared(sdb, db_name, cct); + GetObject = make_shared(sdb, db_name, cct); + UpdateObject = make_shared(sdb, db_name, cct); + ListBucketObjects = make_shared(sdb, db_name, cct); + PutObjectData = make_shared(sdb, db_name, cct); + UpdateObjectData = make_shared(sdb, db_name, cct); + GetObjectData = make_shared(sdb, db_name, cct); + DeleteObjectData = make_shared(sdb, db_name, cct); + DeleteStaleObjectData = make_shared(sdb, db_name, cct); return 0; } diff --git a/src/rgw/store/dbstore/sqlite/sqliteDB.h b/src/rgw/store/dbstore/sqlite/sqliteDB.h index 91575fd0a028..836a8702a03e 100644 --- a/src/rgw/store/dbstore/sqlite/sqliteDB.h +++ b/src/rgw/store/dbstore/sqlite/sqliteDB.h @@ -34,7 +34,6 @@ class SQLiteDB : public DB, virtual public DBOp { void *openDB(const DoutPrefixProvider *dpp) override; int closeDB(const DoutPrefixProvider *dpp) override; int InitializeDBOps(const DoutPrefixProvider *dpp) override; - int FreeDBOps(const DoutPrefixProvider *dpp) override; int InitPrepareParams(const DoutPrefixProvider *dpp, DBOpPrepareParams &p_params, DBOpParams* params) override; @@ -82,7 +81,6 @@ class SQLObjectOp : public ObjectOp { ~SQLObjectOp() {} int InitializeObjectOps(std::string db_name, const DoutPrefixProvider *dpp); - int FreeObjectOps(const DoutPrefixProvider *dpp); }; class SQLInsertUser : public SQLiteDB, public InsertUserOp { -- 2.47.3