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 <skoduri@redhat.com>

diff --git a/src/rgw/store/dbstore/common/dbstore.cc b/src/rgw/store/dbstore/common/dbstore.cc
index 692d954b7813902b7c7c460e5895fa01474d1260..2cd9709250841c42407d3f2e18eae907485df4aa 100644 (file)
--- 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_name << dendl;
 
@@ -91,7 +89,7 @@ int DB::Destroy(const DoutPrefixProvider *dpp)
 }
 
 
-DBOp *DB::getDBOp(const DoutPrefixProvider *dpp, std::string_view Op,
+std::shared_ptr<class DBOp> 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: " \
       <<params->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<string, class ObjectOp*>::iterator iter;
-  class ObjectOp *Ob;
 
   const std::lock_guard<std::mutex> 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<class DBOp> 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 10056dfa452c5235464de5ee6f81ed65859937bc..d35b1bc0f3dbd26288eb17bf1f8fbebdf28b8471 100644 (file)
--- 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<class InsertUserOp> InsertUser;
+  std::shared_ptr<class RemoveUserOp> RemoveUser;
+  std::shared_ptr<class GetUserOp> GetUser;
+  std::shared_ptr<class InsertBucketOp> InsertBucket;
+  std::shared_ptr<class UpdateBucketOp> UpdateBucket;
+  std::shared_ptr<class RemoveBucketOp> RemoveBucket;
+  std::shared_ptr<class GetBucketOp> GetBucket;
+  std::shared_ptr<class ListUserBucketsOp> ListUserBuckets;
+  std::shared_ptr<class InsertLCEntryOp> InsertLCEntry;
+  std::shared_ptr<class RemoveLCEntryOp> RemoveLCEntry;
+  std::shared_ptr<class GetLCEntryOp> GetLCEntry;
+  std::shared_ptr<class ListLCEntriesOp> ListLCEntries;
+  std::shared_ptr<class  InsertLCHeadOp> InsertLCHead;
+  std::shared_ptr<class RemoveLCHeadOp> RemoveLCHead;
+  std::shared_ptr<class GetLCHeadOp> 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<class PutObjectOp> PutObject;
+    std::shared_ptr<class DeleteObjectOp> DeleteObject;
+    std::shared_ptr<class GetObjectOp> GetObject;
+    std::shared_ptr<class UpdateObjectOp> UpdateObject;
+    std::shared_ptr<class ListBucketObjectsOp> ListBucketObjects;
+    std::shared_ptr<class PutObjectDataOp> PutObjectData;
+    std::shared_ptr<class UpdateObjectDataOp> UpdateObjectData;
+    std::shared_ptr<class GetObjectDataOp> GetObjectData;
+    std::shared_ptr<class DeleteObjectDataOp> DeleteObjectData;
+    std::shared_ptr<class DeleteStaleObjectDataOp> 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<class DBOp> 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 efbad31121d8dedc06ab882f46f741be31c2eded..e89ff29a8a9a0688cd4d0ea8b29474e50540d784 100644 (file)
--- 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<SQLInsertUser>(&this->db, this->getDBname(), cct);
+  dbops.RemoveUser = make_shared<SQLRemoveUser>(&this->db, this->getDBname(), cct);
+  dbops.GetUser = make_shared<SQLGetUser>(&this->db, this->getDBname(), cct);
+  dbops.InsertBucket = make_shared<SQLInsertBucket>(&this->db, this->getDBname(), cct);
+  dbops.UpdateBucket = make_shared<SQLUpdateBucket>(&this->db, this->getDBname(), cct);
+  dbops.RemoveBucket = make_shared<SQLRemoveBucket>(&this->db, this->getDBname(), cct);
+  dbops.GetBucket = make_shared<SQLGetBucket>(&this->db, this->getDBname(), cct);
+  dbops.ListUserBuckets = make_shared<SQLListUserBuckets>(&this->db, this->getDBname(), cct);
+  dbops.InsertLCEntry = make_shared<SQLInsertLCEntry>(&this->db, this->getDBname(), cct);
+  dbops.RemoveLCEntry = make_shared<SQLRemoveLCEntry>(&this->db, this->getDBname(), cct);
+  dbops.GetLCEntry = make_shared<SQLGetLCEntry>(&this->db, this->getDBname(), cct);
+  dbops.ListLCEntries = make_shared<SQLListLCEntries>(&this->db, this->getDBname(), cct);
+  dbops.InsertLCHead = make_shared<SQLInsertLCHead>(&this->db, this->getDBname(), cct);
+  dbops.RemoveLCHead = make_shared<SQLRemoveLCHead>(&this->db, this->getDBname(), cct);
+  dbops.GetLCHead = make_shared<SQLGetLCHead>(&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<SQLPutObject>(sdb, db_name, cct);
+  DeleteObject = make_shared<SQLDeleteObject>(sdb, db_name, cct);
+  GetObject = make_shared<SQLGetObject>(sdb, db_name, cct);
+  UpdateObject = make_shared<SQLUpdateObject>(sdb, db_name, cct);
+  ListBucketObjects = make_shared<SQLListBucketObjects>(sdb, db_name, cct);
+  PutObjectData = make_shared<SQLPutObjectData>(sdb, db_name, cct);
+  UpdateObjectData = make_shared<SQLUpdateObjectData>(sdb, db_name, cct);
+  GetObjectData = make_shared<SQLGetObjectData>(sdb, db_name, cct);
+  DeleteObjectData = make_shared<SQLDeleteObjectData>(sdb, db_name, cct);
+  DeleteStaleObjectData = make_shared<SQLDeleteStaleObjectData>(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 91575fd0a028e1e83d7ed302ec566aa9f7ecc3c4..836a8702a03e203bc2f2fc1e47aa67456b0b7c7d 100644 (file)
--- 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 {