From c59e0d7c353129ca563eaba757ebe102316bc4d9 Mon Sep 17 00:00:00 2001 From: Golitsin Vyacheslav Date: Mon, 28 Sep 2026 08:43:06 +0300 Subject: [PATCH] fix(DBDriver): resolve lock-order inversion between dbSafeAccess and trashes mutexes (#1775) * fix(DBDriver): resolve lock-order inversion between dbSafeAccess and trashes mutexes emptyTrashes() acquired _dbSafeAccessMutex while holding _trashesMutex (M1->M0), whereas the load() path acquires _dbSafeAccessMutex and then, inside loadQuery()->getLastWordId(), acquires _trashesMutex (M0->M1). This opposite nesting forms a lock-order cycle that ThreadSanitizer flags as a potential deadlock. Acquire _dbSafeAccessMutex only after releasing _trashesMutex, matching the sequential 'look in trash, then database' pattern used by every other DBDriver accessor (getLastWordId, getLastMapId, getInvertedIndexNi, ...). Refs: #1765 * Fxing the actual deadlock * Added doc --------- Co-authored-by: matlabbe --- corelib/include/rtabmap/core/DBDriver.h | 9 +++++++++ corelib/src/DBDriverSqlite3.cpp | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/corelib/include/rtabmap/core/DBDriver.h b/corelib/include/rtabmap/core/DBDriver.h index 8787bb7d..bc8c2cff 100644 --- a/corelib/include/rtabmap/core/DBDriver.h +++ b/corelib/include/rtabmap/core/DBDriver.h @@ -331,6 +331,12 @@ protected: /** * @name Backend implementation (subclass responsibility) * @brief Pure virtual SQL/backend hooks invoked by public wrappers above. + * + * These are called with \c _dbSafeAccessMutex locked. Implementations must not + * call public methods that look in the trash (they lock \c _trashesMutex), as it + * would invert the lock order used by emptyTrashes() and could deadlock. Call the + * corresponding \c *Query() method directly instead (e.g., getLastIdQuery("Word", id) + * instead of getLastWordId(id)). * @{*/ virtual bool connectDatabaseQuery(const std::string & url, bool overwritten = false, bool readOnly = false) = 0; virtual void disconnectDatabaseQuery(bool save = true, const std::string & outputUrl = "") = 0; @@ -457,6 +463,9 @@ private: UMutex _transactionMutex; std::map _trashSignatures;// std::map _trashVisualWords; // + // Lock order: _trashesMutex -> _dbSafeAccessMutex -> _transactionMutex. + // emptyTrashes() locks _dbSafeAccessMutex before releasing _trashesMutex, so that + // an item not found in the trash is guaranteed to be readable from the database. UMutex _trashesMutex; UMutex _dbSafeAccessMutex; USemaphore _addSem; diff --git a/corelib/src/DBDriverSqlite3.cpp b/corelib/src/DBDriverSqlite3.cpp index f63dea09..bb476ab5 100644 --- a/corelib/src/DBDriverSqlite3.cpp +++ b/corelib/src/DBDriverSqlite3.cpp @@ -3624,8 +3624,8 @@ void DBDriverSqlite3::loadQuery(VWDictionary & dictionary, bool lastStateOnly, b rc = sqlite3_finalize(ppStmt); UASSERT_MSG(rc == SQLITE_OK, uFormat("DB error (%s): %s", _version.c_str(), sqlite3_errmsg(_ppDb)).c_str()); - // Get Last word id - getLastWordId(id); + // Get Last word id (query directly: _dbSafeAccessMutex is already locked by DBDriver::load()) + getLastIdQuery("Word", id); dictionary.setLastWordId(id); if(!idsOnly && uStrNumCmp(_version, "0.23.0") >= 0) {