Dbdriver trash mutex build time protection (#1778)

* 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

* DBDriverSqlite3: deny usage of public trash mutex protected functions from internal query implementations

---------

Co-authored-by: webzuweb <[email protected]>

corelib/include/rtabmap/core/DBDriverSqlite3.h

@@ -156,6 +156,40 @@ public:
 	void setTempStore(int tempStore);
 
 protected:
+	/**
+	 * @name Trash-checking DBDriver methods, hidden on purpose
+	 * @brief These public DBDriver methods lock the trash mutex. They are hidden here so that
+	 * *Query() implementations, which are called with the database mutex already locked,
+	 * cannot call them by mistake (it would invert the lock order with DBDriver::emptyTrashes()
+	 * and could deadlock). Call the corresponding *Query() method instead.
+	 *
+	 * To call them from outside, use a DBDriver pointer or reference (e.g., DBDriver::create()).
+	 * @{*/
+	void asyncSave(Signature * s) = delete;
+	void asyncSave(VisualWord * vw) = delete;
+	void loadSignatures(const std::list<int> & ids, std::list<Signature *> & signatures, std::set<int> * loadedFromTrash = 0, bool loadWordIdsOnly = false) = delete;
+	void loadWords(const std::set<int> & wordIds, std::list<VisualWord *> & vws) = delete;
+	void loadNodeData(Signature & signature, bool images = true, bool scan = true, bool userData = true, bool occupancyGrid = true) const = delete;
+	void loadNodeData(std::list<Signature *> & signatures, bool images = true, bool scan = true, bool userData = true, bool occupancyGrid = true) const = delete;
+	void getNodeData(int signatureId, SensorData & data, bool images = true, bool scan = true, bool userData = true, bool occupancyGrid = true) const = delete;
+	bool getCalibration(int signatureId, std::vector<CameraModel> & models, std::vector<StereoCameraModel> & stereoModels) const = delete;
+	bool getLaserScanInfo(int signatureId, LaserScan & info) const = delete;
+	bool getNodeInfo(int signatureId, Transform & pose, int & mapId, int & weight, std::string & label, double & stamp, Transform & groundTruthPose, std::vector<float> & velocity, GPS & gps, EnvSensors & sensors) const = delete;
+	void getLocalFeatures(int signatureId, std::multimap<int, int> & words, std::vector<cv::KeyPoint> & keypoints, std::vector<cv::Point3f> & points, cv::Mat & descriptors) const = delete;
+	void loadLinks(int signatureId, std::multimap<int, Link> & links, Link::Type type = Link::kUndef) const = delete;
+	void getWeight(int signatureId, int & weight) const = delete;
+	void getAllNodeIds(std::set<int> & ids, bool ignoreChildren = false, bool ignoreBadSignatures = false, bool ignoreIntermediateNodes = false) const = delete;
+	void getAllOdomPoses(std::map<int, Transform> & poses, bool ignoreChildren = false, bool ignoreIntermediateNodes = false) const = delete;
+	void getAllLinks(std::multimap<int, Link> & links, bool ignoreNullLinks = true, bool withLandmarks = false) const = delete;
+	void getLastNodeId(int & id) const = delete;
+	void getLastMapId(int & mapId) const = delete;
+	void getLastWordId(int & id) const = delete;
+	void getInvertedIndexNi(int signatureId, int & ni) const = delete;
+	void getNodesObservingLandmark(int landmarkId, std::map<int, Link> & nodes) const = delete;
+	void getNodeIdByLabel(const std::string & label, int & id) const = delete;
+	void getAllLabels(std::map<int, std::string> & labels) const = delete;
+	/** @} */
+
 	virtual bool connectDatabaseQuery(const std::string & url, bool overwritten = false, bool readOnly = false);
 	virtual void disconnectDatabaseQuery(bool save = true, const std::string & outputUrl = "");
 	virtual bool isConnectedQuery() const;

corelib/test/test_dbdriversqlite3.cpp

@@ -50,9 +50,12 @@ protected:
 		}
 	}
 
+	// Trash-checking methods are hidden in DBDriverSqlite3, call them through the base class
+	DBDriver * db() const { return driver_; }
+
 	void saveSignature(Signature * s)
 	{
-		driver_->asyncSave(s);
+		db()->asyncSave(s);
 		driver_->emptyTrashes(false);
 	}
 
@@ -103,7 +106,7 @@ TEST(DBDriverSqlite3Test, ParseParametersEnablesInMemory)
 	EXPECT_TRUE(driver.isInMemory());
 	EXPECT_TRUE(driver.isConnected());
 
-	driver.asyncSave(new Signature(1));
+	static_cast<DBDriver &>(driver).asyncSave(new Signature(1));
 	driver.emptyTrashes(false);
 	EXPECT_EQ(driver.getTotalNodesSize(), 1);
 
@@ -121,7 +124,7 @@ TEST(DBDriverSqlite3Test, InMemorySaveToFileOnClose)
 	ASSERT_TRUE(driver.openConnection(path, true));
 	EXPECT_TRUE(driver.isInMemory());
 
-	driver.asyncSave(new Signature(1, 5, 1, 50.0, "sqlite_mem", Transform(1.f, 0.f, 0.f, 0.f, 0.f, 0.f)));
+	static_cast<DBDriver &>(driver).asyncSave(new Signature(1, 5, 1, 50.0, "sqlite_mem", Transform(1.f, 0.f, 0.f, 0.f, 0.f, 0.f)));
 	driver.emptyTrashes(false);
 	driver.closeConnection(true, path);
 
@@ -233,7 +236,7 @@ TEST_F(DBDriverSqlite3Fixture, SavesAndLoadsRichSensorData)
 	saveSignature(s);
 
 	std::list<Signature *> loaded;
-	driver_->loadSignatures(std::list<int>(1, 10), loaded);
+	db()->loadSignatures(std::list<int>(1, 10), loaded);
 	ASSERT_EQ(1u, loaded.size());
 	Signature * back = loaded.front();
 	EXPECT_EQ(10, back->id());
@@ -244,7 +247,7 @@ TEST_F(DBDriverSqlite3Fixture, SavesAndLoadsRichSensorData)
 
 	// Payloads come back compressed; ask the driver to fill them in.
 	std::list<Signature *> toFill(1, back);
-	driver_->loadNodeData(toFill);
+	db()->loadNodeData(toFill);
 	back->sensorData().uncompressData();
 	EXPECT_FALSE(back->sensorData().imageRaw().empty()) << "image blob did not round-trip";
 	EXPECT_FALSE(back->sensorData().depthRaw().empty()) << "depth blob did not round-trip";
@@ -320,9 +323,9 @@ TEST_F(DBDriverSqlite3Fixture, RawOnlySensorDataIsNotPersisted)
 	saveSignature(new Signature(42, 0, 1, 1.0, "", Transform::getIdentity(), Transform(), raw));
 
 	std::list<Signature *> loaded;
-	driver_->loadSignatures(std::list<int>(1, 42), loaded);
+	db()->loadSignatures(std::list<int>(1, 42), loaded);
 	ASSERT_EQ(1u, loaded.size());
-	driver_->loadNodeData(loaded);
+	db()->loadNodeData(loaded);
 	loaded.front()->sensorData().uncompressData();
 	EXPECT_TRUE(loaded.front()->sensorData().imageRaw().empty())
 			<< "raw-only image unexpectedly survived a save/load round trip";
@@ -363,9 +366,12 @@ protected:
 		UFile::erase(dbPath_.c_str());
 	}
 
+	// Trash-checking methods are hidden in DBDriverSqlite3, call them through the base class
+	DBDriver * db() const { return driver_; }
+
 	void saveSignature(Signature * s)
 	{
-		driver_->asyncSave(s);
+		db()->asyncSave(s);
 		driver_->emptyTrashes(false);
 	}
 
@@ -400,11 +406,11 @@ TEST_P(DBSchemaVersionTest, NodesAndLinksSurviveARoundTrip)
 	EXPECT_FALSE(driver_->getDatabaseVersion().empty());
 
 	std::list<Signature *> loaded;
-	driver_->loadSignatures(std::list<int>{1, 2}, loaded);
+	db()->loadSignatures(std::list<int>{1, 2}, loaded);
 	ASSERT_EQ(2u, loaded.size()) << "nodes did not survive the round trip";
 
 	// Payloads
-	driver_->loadNodeData(loaded);
+	db()->loadNodeData(loaded);
 	for(Signature * s : loaded)
 	{
 		s->sensorData().uncompressData();
@@ -415,7 +421,7 @@ TEST_P(DBSchemaVersionTest, NodesAndLinksSurviveARoundTrip)
 	// Links: the second node must still point back at the first, with the
 	// variances recovered from whatever columns this schema uses.
 	std::multimap<int, Link> links;
-	driver_->loadLinks(2, links);
+	db()->loadLinks(2, links);
 	ASSERT_FALSE(links.empty()) << "link did not survive the round trip";
 	const Link & link = links.begin()->second;
 	EXPECT_EQ(1, link.to());