libcamera: camera_manager: Construct CameraManager instances manually

The CameraManager class is not supposed to be instantiated multiple
times, which led to a singleton implementation. This requires a global
instance of the CameraManager, which is destroyed when the global
destructors are executed.

Relying on global instances causes issues with cleanup, as the order in
which the global destructors are run can't be controlled. In particular,
the Android camera HAL implementation ends up destroying the
CameraHalManager after the CameraManager, which leads to use-after-free
problems.

To solve this, remove the CameraManager::instance() method and make the
CameraManager class instantiable directly. Multiple instances are still
not allowed, and this is enforced by storing the instance pointer
internally to be checked when an instance is created.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

9 files changed, 48 insertions, 37 deletions

diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h
index ff7d4c7c..8331898c 100644
--- a/include/libcamera/camera_manager.h
+++ b/include/libcamera/camera_manager.h
@@ -23,6 +23,11 @@ class PipelineHandler;
 class CameraManager : public Object
 {
 public:
+	CameraManager();
+	CameraManager(const CameraManager &) = delete;
+	CameraManager &operator=(const CameraManager &) = delete;
+	~CameraManager();
+
 	int start();
 	void stop();
 
@@ -32,23 +37,18 @@ public:
 	void addCamera(std::shared_ptr<Camera> camera);
 	void removeCamera(Camera *camera);
 
-	static CameraManager *instance();
 	static const std::string &version() { return version_; }
 
 	void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher);
 	EventDispatcher *eventDispatcher();
 
 private:
-	CameraManager();
-	CameraManager(const CameraManager &) = delete;
-	CameraManager &operator=(const CameraManager &) = delete;
-	~CameraManager();
-
 	std::unique_ptr<DeviceEnumerator> enumerator_;
 	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
 	std::vector<std::shared_ptr<Camera>> cameras_;
 
 	static const std::string version_;
+	static CameraManager *self_;
 };
 
 } /* namespace libcamera */
diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
index cf981720..22f0323b 100644
--- a/src/android/camera_hal_manager.cpp
+++ b/src/android/camera_hal_manager.cpp
@@ -59,7 +59,7 @@ void CameraHalManager::run()
 	 * order to bind them to the camera HAL manager thread that
 	 * executes the event dispatcher.
 	 */
-	cameraManager_ = libcamera::CameraManager::instance();
+	cameraManager_ = new CameraManager();
 
 	int ret = cameraManager_->start();
 	if (ret) {
@@ -93,7 +93,10 @@ void CameraHalManager::run()
 
 	/* Clean up the resources we have allocated. */
 	proxies_.clear();
+
 	cameraManager_->stop();
+	delete cameraManager_;
+	cameraManager_ = nullptr;
 }
 
 CameraProxy *CameraHalManager::open(unsigned int id,
diff --git a/src/cam/main.cpp b/src/cam/main.cpp
index 77bb20e9..9d99f558 100644
--- a/src/cam/main.cpp
+++ b/src/cam/main.cpp
@@ -23,6 +23,7 @@ class CamApp
 {
 public:
 	CamApp();
+	~CamApp();
 
 	static CamApp *instance();
 
@@ -54,6 +55,11 @@ CamApp::CamApp()
 	CamApp::app_ = this;
 }
 
+CamApp::~CamApp()
+{
+	delete cm_;
+}
+
 CamApp *CamApp::instance()
 {
 	return CamApp::app_;
@@ -67,7 +73,7 @@ int CamApp::init(int argc, char **argv)
 	if (ret < 0)
 		return ret;
 
-	cm_ = CameraManager::instance();
+	cm_ = new CameraManager();
 
 	ret = cm_->start();
 	if (ret) {
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 4a880684..12cb5a0b 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -35,11 +35,14 @@ LOG_DEFINE_CATEGORY(Camera)
  * in the system to applications. The manager owns all Camera objects and
  * handles hot-plugging and hot-unplugging to manage the lifetime of cameras.
  *
- * To interact with libcamera, an application retrieves the camera manager
- * instance with CameraManager::instance(). The manager is initially stopped,
- * and shall be configured before being started. In particular a custom event
- * dispatcher shall be installed if needed with
- * CameraManager::setEventDispatcher().
+ * To interact with libcamera, an application starts by creating a camera
+ * manager instance. Only a single instance of the camera manager may exist at
+ * a time. Attempting to create a second instance without first deleting the
+ * existing instance results in undefined behaviour.
+ *
+ * The manager is initially stopped, and shall be configured before being
+ * started. In particular a custom event dispatcher shall be installed if
+ * needed with CameraManager::setEventDispatcher().
  *
  * Once the camera manager is configured, it shall be started with start().
  * This will enumerate all the cameras present in the system, which can then be
@@ -56,13 +59,21 @@ LOG_DEFINE_CATEGORY(Camera)
  * removed due to hot-unplug.
  */
 
+CameraManager *CameraManager::self_ = nullptr;
+
 CameraManager::CameraManager()
 	: enumerator_(nullptr)
 {
+	if (self_)
+		LOG(Camera, Fatal)
+			<< "Multiple CameraManager objects are not allowed";
+
+	self_ = this;
 }
 
 CameraManager::~CameraManager()
 {
+	self_ = nullptr;
 }
 
 /**
@@ -213,21 +224,6 @@ void CameraManager::removeCamera(Camera *camera)
 }
 
 /**
- * \brief Retrieve the camera manager instance
- *
- * The CameraManager is a singleton and can't be constructed manually. This
- * function shall instead be used to retrieve the single global instance of the
- * manager.
- *
- * \return The camera manager instance
- */
-CameraManager *CameraManager::instance()
-{
-	static CameraManager manager;
-	return &manager;
-}
-
-/**
  * \fn const std::string &CameraManager::version()
  * \brief Retrieve the libcamera version string
  * \return The libcamera version string
diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
index 05d3b77e..a7ff5c52 100644
--- a/src/qcam/main.cpp
+++ b/src/qcam/main.cpp
@@ -63,7 +63,7 @@ int main(int argc, char **argv)
 	sigaction(SIGINT, &sa, nullptr);
 
 	std::unique_ptr<EventDispatcher> dispatcher(new QtEventDispatcher());
-	CameraManager *cm = CameraManager::instance();
+	CameraManager *cm = new CameraManager();
 	cm->setEventDispatcher(std::move(dispatcher));
 
 	ret = cm->start();
@@ -79,5 +79,7 @@ int main(int argc, char **argv)
 	delete mainWindow;
 
 	cm->stop();
+	delete cm;
+
 	return ret;
 }
diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp
index 24ff5fe0..0e105414 100644
--- a/test/camera/camera_test.cpp
+++ b/test/camera/camera_test.cpp
@@ -14,7 +14,7 @@ using namespace std;
 
 int CameraTest::init()
 {
-	cm_ = CameraManager::instance();
+	cm_ = new CameraManager();
 
 	if (cm_->start()) {
 		cout << "Failed to start camera manager" << endl;
@@ -44,4 +44,5 @@ void CameraTest::cleanup()
 	}
 
 	cm_->stop();
+	delete cm_;
 };
diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp
index c834edc3..f1d79ff8 100644
--- a/test/controls/control_list.cpp
+++ b/test/controls/control_list.cpp
@@ -21,7 +21,7 @@ class ControlListTest : public Test
 protected:
 	int init()
 	{
-		cm_ = CameraManager::instance();
+		cm_ = new CameraManager();
 
 		if (cm_->start()) {
 			cout << "Failed to start camera manager" << endl;
@@ -203,6 +203,7 @@ protected:
 		}
 
 		cm_->stop();
+		delete cm_;
 	}
 
 private:
diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp
index 070cbf2b..55551d7e 100644
--- a/test/list-cameras.cpp
+++ b/test/list-cameras.cpp
@@ -20,8 +20,8 @@ class ListTest : public Test
 protected:
 	int init()
 	{
-		cm = CameraManager::instance();
-		cm->start();
+		cm_ = new CameraManager();
+		cm_->start();
 
 		return 0;
 	}
@@ -30,7 +30,7 @@ protected:
 	{
 		unsigned int count = 0;
 
-		for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
+		for (const std::shared_ptr<Camera> &camera : cm_->cameras()) {
 			cout << "- " << camera->name() << endl;
 			count++;
 		}
@@ -40,11 +40,12 @@ protected:
 
 	void cleanup()
 	{
-		cm->stop();
+		cm_->stop();
+		delete cm_;
 	}
 
 private:
-	CameraManager *cm;
+	CameraManager *cm_;
 };
 
 TEST_REGISTER(ListTest)
diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
index 1d4cc4d4..8bfcd609 100644
--- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp
+++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp
@@ -92,7 +92,7 @@ int IPU3PipelineTest::init()
 
 	enumerator.reset(nullptr);
 
-	cameraManager_ = CameraManager::instance();
+	cameraManager_ = new CameraManager();
 	ret = cameraManager_->start();
 	if (ret) {
 		cerr << "Failed to start the CameraManager" << endl;
@@ -120,6 +120,7 @@ int IPU3PipelineTest::run()
 void IPU3PipelineTest::cleanup()
 {
 	cameraManager_->stop();
+	delete cameraManager_;
 }
 
 TEST_REGISTER(IPU3PipelineTest)