[{"id":2469,"web_url":"https://patchwork.libcamera.org/comment/2469/","msgid":"<20190819084000.gpz2ga6dtwevdzww@uno.localdomain>","date":"2019-08-19T08:40:00","subject":"Re: [libcamera-devel] [PATCH 13/14] libcamera: camera_manager:\n\tConstruct CameraManager instances manually","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Sun, Aug 18, 2019 at 04:13:28AM +0300, Laurent Pinchart wrote:\n> The CameraManager class is not supposed to be instantiated multiple\n> times, which led to a singleton implementation. This requires a global\n> instance of the CameraManager, which is destroyed when the global\n> destructors are executed.\n>\n> Relying on global instances causes issues with cleanup, as the order in\n> which the global destructors are run can't be controlled. In particular,\n> the Android camera HAL implementation ends up destroying the\n> CameraHalManager after the CameraManager, which leads to use-after-free\n> problems.\n>\n> To solve this, remove the CameraManager::instance() method and make the\n> CameraManager class instantiable directly. Multiple instances are still\n> not allowed, and this is enforced by storing the instance pointer\n> internally to be checked when an instance is created.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/camera_manager.h | 12 ++++----\n> src/android/camera_hal_manager.cpp | 5 +++-\n> src/cam/main.cpp | 8 ++++-\n> src/libcamera/camera_manager.cpp | 36 ++++++++++-------------\n> src/qcam/main.cpp | 4 ++-\n> test/camera/camera_test.cpp | 3 +-\n> test/controls/control_list.cpp | 3 +-\n> test/list-cameras.cpp | 11 +++----\n> test/pipeline/ipu3/ipu3_pipeline_test.cpp | 3 +-\n> 9 files changed, 48 insertions(+), 37 deletions(-)\n>\n\n[snip]\n\n>\n> +CameraManager *CameraManager::self_ = nullptr;\n> +\n> CameraManager::CameraManager()\n> \t: enumerator_(nullptr)\n> {\n> +\tif (self_)\n> +\t\tLOG(Camera, Fatal)\n> +\t\t\t<< \"Multiple CameraManager objects are not allowed\";\n> +\n> +\tself_ = this;\n\nThis is a very weak guarantee that the CameraManager would not be\ncreated twice...\n\nI was wondering, our previous singleton implementation was based on\nlazy evaluation, by returning a statically allocated CameraManager instance:\n\nCameraManager *CameraManager::instance()\n{\n\tstatic CameraManager manager;\n\treturn &manager;\n}\n\nThis mean it gets deleted at program termination time causing the\nissue you're here trying to solve.\n\nWhat if the make the manager creation dynamic instead and provide a\nstatic destructor method, that applications could call to guarantee\nthe manager is deleted at the right time?\n\nWould something like this work in your opinion?\n\ndiff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\nindex ff7d4c7c6745..a29a24f5e37a 100644\n--- a/include/libcamera/camera_manager.h\n+++ b/include/libcamera/camera_manager.h\n@@ -33,6 +33,7 @@ public:\n void removeCamera(Camera *camera);\n\n static CameraManager *instance();\n+ static void destroy();\n static const std::string &version() { return version_; }\n\n void setEventDispatcher(std::unique_ptr dispatcher);\n@@ -49,6 +50,8 @@ private:\n std::vector> cameras_;\n\n static const std::string version_;\n+\n+ static CameraManager *manager_;\n };\n\n } /* namespace libcamera */\ndiff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\nindex 4a880684c5cb..ed175ccc5860 100644\n--- a/src/libcamera/camera_manager.cpp\n+++ b/src/libcamera/camera_manager.cpp\n@@ -26,6 +26,8 @@ namespace libcamera {\n\n LOG_DEFINE_CATEGORY(Camera)\n\n+CameraManager *CameraManager::manager_ = nullptr;\n+\n /**\n * \\class CameraManager\n * \\brief Provide access and manage all cameras in the system\n@@ -223,8 +225,16 @@ void CameraManager::removeCamera(Camera *camera)\n */\n CameraManager *CameraManager::instance()\n {\n- static CameraManager manager;\n- return &manager;\n+ if (!manager_)\n+ manager_ = new CameraManager();\n+\n+ return manager_;\n+}\n+\n+void CameraManager::destroy()\n+{\n+ delete manager_;\n+ manager_ = nullptr;\n }\n\n /**\n\n\nThanks\n j\n\n> }\n>\n> CameraManager::~CameraManager()\n> {\n> +\tself_ = nullptr;\n> }\n>\n> /**\n> @@ -212,21 +223,6 @@ void CameraManager::removeCamera(Camera *camera)\n> \t}\n> }\n>\n> -/**\n> - * \\brief Retrieve the camera manager instance\n> - *\n> - * The CameraManager is a singleton and can't be constructed manually. This\n> - * function shall instead be used to retrieve the single global instance of the\n> - * manager.\n> - *\n> - * \\return The camera manager instance\n> - */\n> -CameraManager *CameraManager::instance()\n> -{\n> -\tstatic CameraManager manager;\n> -\treturn &manager;\n> -}\n> -\n> /**\n> * \\fn const std::string &CameraManager::version()\n> * \\brief Retrieve the libcamera version string\n> diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp\n> index 05d3b77e9edb..a7ff5c52663b 100644\n> --- a/src/qcam/main.cpp\n> +++ b/src/qcam/main.cpp\n> @@ -63,7 +63,7 @@ int main(int argc, char **argv)\n> \tsigaction(SIGINT, &sa, nullptr);\n>\n> \tstd::unique_ptr dispatcher(new QtEventDispatcher());\n> -\tCameraManager *cm = CameraManager::instance();\n> +\tCameraManager *cm = new CameraManager();\n> \tcm->setEventDispatcher(std::move(dispatcher));\n>\n> \tret = cm->start();\n> @@ -79,5 +79,7 @@ int main(int argc, char **argv)\n> \tdelete mainWindow;\n>\n> \tcm->stop();\n> +\tdelete cm;\n> +\n> \treturn ret;\n> }\n> diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp\n> index 24ff5fe0c64d..0e105414bf46 100644\n> --- a/test/camera/camera_test.cpp\n> +++ b/test/camera/camera_test.cpp\n> @@ -14,7 +14,7 @@ using namespace std;\n>\n> int CameraTest::init()\n> {\n> -\tcm_ = CameraManager::instance();\n> +\tcm_ = new CameraManager();\n>\n> \tif (cm_->start()) {\n> \t\tcout << \"Failed to start camera manager\" << endl;\n> @@ -44,4 +44,5 @@ void CameraTest::cleanup()\n> \t}\n>\n> \tcm_->stop();\n> +\tdelete cm_;\n> };\n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index c834edc352f5..f1d79ff8fcfd 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -21,7 +21,7 @@ class ControlListTest : public Test\n> protected:\n> \tint init()\n> \t{\n> -\t\tcm_ = CameraManager::instance();\n> +\t\tcm_ = new CameraManager();\n>\n> \t\tif (cm_->start()) {\n> \t\t\tcout << \"Failed to start camera manager\" << endl;\n> @@ -203,6 +203,7 @@ protected:\n> \t\t}\n>\n> \t\tcm_->stop();\n> +\t\tdelete cm_;\n> \t}\n>\n> private:\n> diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp\n> index 070cbf2be977..55551d7e7e10 100644\n> --- a/test/list-cameras.cpp\n> +++ b/test/list-cameras.cpp\n> @@ -20,8 +20,8 @@ class ListTest : public Test\n> protected:\n> \tint init()\n> \t{\n> -\t\tcm = CameraManager::instance();\n> -\t\tcm->start();\n> +\t\tcm_ = new CameraManager();\n> +\t\tcm_->start();\n>\n> \t\treturn 0;\n> \t}\n> @@ -30,7 +30,7 @@ protected:\n> \t{\n> \t\tunsigned int count = 0;\n>\n> -\t\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> +\t\tfor (const std::shared_ptr &camera : cm_->cameras()) {\n> \t\t\tcout << \"- \" << camera->name() << endl;\n> \t\t\tcount++;\n> \t\t}\n> @@ -40,11 +40,12 @@ protected:\n>\n> \tvoid cleanup()\n> \t{\n> -\t\tcm->stop();\n> +\t\tcm_->stop();\n> +\t\tdelete cm_;\n> \t}\n>\n> private:\n> -\tCameraManager *cm;\n> +\tCameraManager *cm_;\n> };\n>\n> TEST_REGISTER(ListTest)\n> diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> index 1d4cc4d4950b..8bfcd609a071 100644\n> --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> @@ -92,7 +92,7 @@ int IPU3PipelineTest::init()\n>\n> \tenumerator.reset(nullptr);\n>\n> -\tcameraManager_ = CameraManager::instance();\n> +\tcameraManager_ = new CameraManager();\n> \tret = cameraManager_->start();\n> \tif (ret) {\n> \t\tcerr << \"Failed to start the CameraManager\" << endl;\n> @@ -120,6 +120,7 @@ int IPU3PipelineTest::run()\n> void IPU3PipelineTest::cleanup()\n> {\n> \tcameraManager_->stop();\n> +\tdelete cameraManager_;\n> }\n>\n> TEST_REGISTER(IPU3PipelineTest)\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FF0960E38\n\tfor ;\n\tMon, 19 Aug 2019 10:38:33 +0200 (CEST)","from uno.localdomain (unknown [87.18.63.98])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 306A2200004;\n\tMon, 19 Aug 2019 08:38:31 +0000 (UTC)"],"Date":"Mon, 19 Aug 2019 10:40:00 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190819084000.gpz2ga6dtwevdzww@uno.localdomain>","References":"<20190818011329.14499-1-laurent.pinchart@ideasonboard.com>\n\t<20190818011329.14499-14-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"up5wid6h2phnum5c\"","Content-Disposition":"inline","In-Reply-To":"<20190818011329.14499-14-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 13/14] libcamera: camera_manager:\n\tConstruct CameraManager instances manually","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 19 Aug 2019 08:38:33 -0000"}},{"id":2493,"web_url":"https://patchwork.libcamera.org/comment/2493/","msgid":"<20190819153752.GM5011@pendragon.ideasonboard.com>","date":"2019-08-19T15:37:52","subject":"Re: [libcamera-devel] [PATCH 13/14] libcamera: camera_manager:\n\tConstruct CameraManager instances manually","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Aug 19, 2019 at 10:40:00AM +0200, Jacopo Mondi wrote:\n> On Sun, Aug 18, 2019 at 04:13:28AM +0300, Laurent Pinchart wrote:\n> > The CameraManager class is not supposed to be instantiated multiple\n> > times, which led to a singleton implementation. This requires a global\n> > instance of the CameraManager, which is destroyed when the global\n> > destructors are executed.\n> >\n> > Relying on global instances causes issues with cleanup, as the order in\n> > which the global destructors are run can't be controlled. In particular,\n> > the Android camera HAL implementation ends up destroying the\n> > CameraHalManager after the CameraManager, which leads to use-after-free\n> > problems.\n> >\n> > To solve this, remove the CameraManager::instance() method and make the\n> > CameraManager class instantiable directly. Multiple instances are still\n> > not allowed, and this is enforced by storing the instance pointer\n> > internally to be checked when an instance is created.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/camera_manager.h | 12 ++++----\n> > src/android/camera_hal_manager.cpp | 5 +++-\n> > src/cam/main.cpp | 8 ++++-\n> > src/libcamera/camera_manager.cpp | 36 ++++++++++-------------\n> > src/qcam/main.cpp | 4 ++-\n> > test/camera/camera_test.cpp | 3 +-\n> > test/controls/control_list.cpp | 3 +-\n> > test/list-cameras.cpp | 11 +++----\n> > test/pipeline/ipu3/ipu3_pipeline_test.cpp | 3 +-\n> > 9 files changed, 48 insertions(+), 37 deletions(-)\n> \n> [snip]\n> \n> > +CameraManager *CameraManager::self_ = nullptr;\n> > +\n> > CameraManager::CameraManager()\n> > \t: enumerator_(nullptr)\n> > {\n> > +\tif (self_)\n> > +\t\tLOG(Camera, Fatal)\n> > +\t\t\t<< \"Multiple CameraManager objects are not allowed\";\n> > +\n> > +\tself_ = this;\n> \n> This is a very weak guarantee that the CameraManager would not be\n> created twice...\n\nNote that a fatal error message causes an std::abort() (in all builds,\nincluding release builds), so it's not *that* weak :-)\n\n> I was wondering, our previous singleton implementation was based on\n> lazy evaluation, by returning a statically allocated CameraManager instance:\n> \n> CameraManager *CameraManager::instance()\n> {\n> \tstatic CameraManager manager;\n> \treturn &manager;\n> }\n> \n> This mean it gets deleted at program termination time causing the\n> issue you're here trying to solve.\n> \n> What if the make the manager creation dynamic instead and provide a\n> static destructor method, that applications could call to guarantee\n> the manager is deleted at the right time?\n> \n> Would something like this work in your opinion?\n\nI've considered various options when working on this patch. What I don't\nlike about an explicit destroy() method like proposed below is that it\nbreaks the symmetry between construction and destruction. The instance()\nmethod dilutes the responsibility of creating the camera manager\ninstance (the first call creates it, and the callers don't know if\nthey're the first caller or not, which is the main purpose of a\nsingleton), but requires giving the responsibility for deleting the\ninstance to one particular actor in the system. I think it would be best\nto give the responsibility to create and destroy the camera manager\nexplicitly.\n\nThis being said, if that ends up being helpful, we could restore the\ninstance() method but simply make it return self_. The method would thus\nreturn nullptr before the camera manager instance is constructed, or\nafter it gets deleted. For now we don't have a need for that so I've\nsimply dropped the instance() method.\n\n> diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> index ff7d4c7c6745..a29a24f5e37a 100644\n> --- a/include/libcamera/camera_manager.h\n> +++ b/include/libcamera/camera_manager.h\n> @@ -33,6 +33,7 @@ public:\n> void removeCamera(Camera *camera);\n> \n> static CameraManager *instance();\n> + static void destroy();\n> static const std::string &version() { return version_; }\n> \n> void setEventDispatcher(std::unique_ptr dispatcher);\n> @@ -49,6 +50,8 @@ private:\n> std::vector> cameras_;\n> \n> static const std::string version_;\n> +\n> + static CameraManager *manager_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 4a880684c5cb..ed175ccc5860 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -26,6 +26,8 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Camera)\n> \n> +CameraManager *CameraManager::manager_ = nullptr;\n> +\n> /**\n> * \\class CameraManager\n> * \\brief Provide access and manage all cameras in the system\n> @@ -223,8 +225,16 @@ void CameraManager::removeCamera(Camera *camera)\n> */\n> CameraManager *CameraManager::instance()\n> {\n> - static CameraManager manager;\n> - return &manager;\n> + if (!manager_)\n> + manager_ = new CameraManager();\n> +\n> + return manager_;\n> +}\n> +\n> +void CameraManager::destroy()\n> +{\n> + delete manager_;\n> + manager_ = nullptr;\n> }\n> \n> /**\n> \n> > }\n> >\n> > CameraManager::~CameraManager()\n> > {\n> > +\tself_ = nullptr;\n> > }\n> >\n> > /**\n> > @@ -212,21 +223,6 @@ void CameraManager::removeCamera(Camera *camera)\n> > \t}\n> > }\n> >\n> > -/**\n> > - * \\brief Retrieve the camera manager instance\n> > - *\n> > - * The CameraManager is a singleton and can't be constructed manually. This\n> > - * function shall instead be used to retrieve the single global instance of the\n> > - * manager.\n> > - *\n> > - * \\return The camera manager instance\n> > - */\n> > -CameraManager *CameraManager::instance()\n> > -{\n> > -\tstatic CameraManager manager;\n> > -\treturn &manager;\n> > -}\n> > -\n> > /**\n> > * \\fn const std::string &CameraManager::version()\n> > * \\brief Retrieve the libcamera version string\n> > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp\n> > index 05d3b77e9edb..a7ff5c52663b 100644\n> > --- a/src/qcam/main.cpp\n> > +++ b/src/qcam/main.cpp\n> > @@ -63,7 +63,7 @@ int main(int argc, char **argv)\n> > \tsigaction(SIGINT, &sa, nullptr);\n> >\n> > \tstd::unique_ptr dispatcher(new QtEventDispatcher());\n> > -\tCameraManager *cm = CameraManager::instance();\n> > +\tCameraManager *cm = new CameraManager();\n> > \tcm->setEventDispatcher(std::move(dispatcher));\n> >\n> > \tret = cm->start();\n> > @@ -79,5 +79,7 @@ int main(int argc, char **argv)\n> > \tdelete mainWindow;\n> >\n> > \tcm->stop();\n> > +\tdelete cm;\n> > +\n> > \treturn ret;\n> > }\n> > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp\n> > index 24ff5fe0c64d..0e105414bf46 100644\n> > --- a/test/camera/camera_test.cpp\n> > +++ b/test/camera/camera_test.cpp\n> > @@ -14,7 +14,7 @@ using namespace std;\n> >\n> > int CameraTest::init()\n> > {\n> > -\tcm_ = CameraManager::instance();\n> > +\tcm_ = new CameraManager();\n> >\n> > \tif (cm_->start()) {\n> > \t\tcout << \"Failed to start camera manager\" << endl;\n> > @@ -44,4 +44,5 @@ void CameraTest::cleanup()\n> > \t}\n> >\n> > \tcm_->stop();\n> > +\tdelete cm_;\n> > };\n> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > index c834edc352f5..f1d79ff8fcfd 100644\n> > --- a/test/controls/control_list.cpp\n> > +++ b/test/controls/control_list.cpp\n> > @@ -21,7 +21,7 @@ class ControlListTest : public Test\n> > protected:\n> > \tint init()\n> > \t{\n> > -\t\tcm_ = CameraManager::instance();\n> > +\t\tcm_ = new CameraManager();\n> >\n> > \t\tif (cm_->start()) {\n> > \t\t\tcout << \"Failed to start camera manager\" << endl;\n> > @@ -203,6 +203,7 @@ protected:\n> > \t\t}\n> >\n> > \t\tcm_->stop();\n> > +\t\tdelete cm_;\n> > \t}\n> >\n> > private:\n> > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp\n> > index 070cbf2be977..55551d7e7e10 100644\n> > --- a/test/list-cameras.cpp\n> > +++ b/test/list-cameras.cpp\n> > @@ -20,8 +20,8 @@ class ListTest : public Test\n> > protected:\n> > \tint init()\n> > \t{\n> > -\t\tcm = CameraManager::instance();\n> > -\t\tcm->start();\n> > +\t\tcm_ = new CameraManager();\n> > +\t\tcm_->start();\n> >\n> > \t\treturn 0;\n> > \t}\n> > @@ -30,7 +30,7 @@ protected:\n> > \t{\n> > \t\tunsigned int count = 0;\n> >\n> > -\t\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> > +\t\tfor (const std::shared_ptr &camera : cm_->cameras()) {\n> > \t\t\tcout << \"- \" << camera->name() << endl;\n> > \t\t\tcount++;\n> > \t\t}\n> > @@ -40,11 +40,12 @@ protected:\n> >\n> > \tvoid cleanup()\n> > \t{\n> > -\t\tcm->stop();\n> > +\t\tcm_->stop();\n> > +\t\tdelete cm_;\n> > \t}\n> >\n> > private:\n> > -\tCameraManager *cm;\n> > +\tCameraManager *cm_;\n> > };\n> >\n> > TEST_REGISTER(ListTest)\n> > diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > index 1d4cc4d4950b..8bfcd609a071 100644\n> > --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > @@ -92,7 +92,7 @@ int IPU3PipelineTest::init()\n> >\n> > \tenumerator.reset(nullptr);\n> >\n> > -\tcameraManager_ = CameraManager::instance();\n> > +\tcameraManager_ = new CameraManager();\n> > \tret = cameraManager_->start();\n> > \tif (ret) {\n> > \t\tcerr << \"Failed to start the CameraManager\" << endl;\n> > @@ -120,6 +120,7 @@ int IPU3PipelineTest::run()\n> > void IPU3PipelineTest::cleanup()\n> > {\n> > \tcameraManager_->stop();\n> > +\tdelete cameraManager_;\n> > }\n> >\n> > TEST_REGISTER(IPU3PipelineTest)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B49C260C1E\n\tfor ;\n\tMon, 19 Aug 2019 17:37:58 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1BA2E510;\n\tMon, 19 Aug 2019 17:37:58 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1566229078;\n\tbh=eM3ma8DCBnu7euXSQOcNaWLtLEsmwgX1Lc7voyRRnq8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MZE3RI010t8MypQ4x35uk68VN47XyJ4A89XUoa65rS6t/xEj6SGMhXQ968yvf731X\n\tsmt57G3Ynu0mKpqaGzNMc8Mb5knKX7iAh+qRV9BI2CFn84ZI529RjdhtzRhcwchdqT\n\tHV+Ea2ZR4TChSixt6qI1lY2A54gCxdkUCPNjN1wU=","Date":"Mon, 19 Aug 2019 18:37:52 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190819153752.GM5011@pendragon.ideasonboard.com>","References":"<20190818011329.14499-1-laurent.pinchart@ideasonboard.com>\n\t<20190818011329.14499-14-laurent.pinchart@ideasonboard.com>\n\t<20190819084000.gpz2ga6dtwevdzww@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190819084000.gpz2ga6dtwevdzww@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 13/14] libcamera: camera_manager:\n\tConstruct CameraManager instances manually","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 19 Aug 2019 15:37:58 -0000"}},{"id":2498,"web_url":"https://patchwork.libcamera.org/comment/2498/","msgid":"<20190819160500.ffdxswtspc7e7cl5@uno.localdomain>","date":"2019-08-19T16:05:00","subject":"Re: [libcamera-devel] [PATCH 13/14] libcamera: camera_manager:\n\tConstruct CameraManager instances manually","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Aug 19, 2019 at 06:37:52PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Aug 19, 2019 at 10:40:00AM +0200, Jacopo Mondi wrote:\n> > On Sun, Aug 18, 2019 at 04:13:28AM +0300, Laurent Pinchart wrote:\n> > > The CameraManager class is not supposed to be instantiated multiple\n> > > times, which led to a singleton implementation. This requires a global\n> > > instance of the CameraManager, which is destroyed when the global\n> > > destructors are executed.\n> > >\n> > > Relying on global instances causes issues with cleanup, as the order in\n> > > which the global destructors are run can't be controlled. In particular,\n> > > the Android camera HAL implementation ends up destroying the\n> > > CameraHalManager after the CameraManager, which leads to use-after-free\n> > > problems.\n> > >\n> > > To solve this, remove the CameraManager::instance() method and make the\n> > > CameraManager class instantiable directly. Multiple instances are still\n> > > not allowed, and this is enforced by storing the instance pointer\n> > > internally to be checked when an instance is created.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > include/libcamera/camera_manager.h | 12 ++++----\n> > > src/android/camera_hal_manager.cpp | 5 +++-\n> > > src/cam/main.cpp | 8 ++++-\n> > > src/libcamera/camera_manager.cpp | 36 ++++++++++-------------\n> > > src/qcam/main.cpp | 4 ++-\n> > > test/camera/camera_test.cpp | 3 +-\n> > > test/controls/control_list.cpp | 3 +-\n> > > test/list-cameras.cpp | 11 +++----\n> > > test/pipeline/ipu3/ipu3_pipeline_test.cpp | 3 +-\n> > > 9 files changed, 48 insertions(+), 37 deletions(-)\n> >\n> > [snip]\n> >\n> > > +CameraManager *CameraManager::self_ = nullptr;\n> > > +\n> > > CameraManager::CameraManager()\n> > > \t: enumerator_(nullptr)\n> > > {\n> > > +\tif (self_)\n> > > +\t\tLOG(Camera, Fatal)\n> > > +\t\t\t<< \"Multiple CameraManager objects are not allowed\";\n> > > +\n> > > +\tself_ = this;\n> >\n> > This is a very weak guarantee that the CameraManager would not be\n> > created twice...\n>\n> Note that a fatal error message causes an std::abort() (in all builds,\n> including release builds), so it's not *that* weak :-)\n>\n\nOh I see, I thought it applied to debug builds only (and I initially\nmissed it was \"Fatal\")\n\n> > I was wondering, our previous singleton implementation was based on\n> > lazy evaluation, by returning a statically allocated CameraManager instance:\n> >\n> > CameraManager *CameraManager::instance()\n> > {\n> > \tstatic CameraManager manager;\n> > \treturn &manager;\n> > }\n> >\n> > This mean it gets deleted at program termination time causing the\n> > issue you're here trying to solve.\n> >\n> > What if the make the manager creation dynamic instead and provide a\n> > static destructor method, that applications could call to guarantee\n> > the manager is deleted at the right time?\n> >\n> > Would something like this work in your opinion?\n>\n> I've considered various options when working on this patch. What I don't\n> like about an explicit destroy() method like proposed below is that it\n> breaks the symmetry between construction and destruction. The instance()\n> method dilutes the responsibility of creating the camera manager\n> instance (the first call creates it, and the callers don't know if\n> they're the first caller or not, which is the main purpose of a\n> singleton), but requires giving the responsibility for deleting the\n> instance to one particular actor in the system. I think it would be best\n> to give the responsibility to create and destroy the camera manager\n> explicitly.\n\nI understand, and it would probably create a more confused API to deal\nwith.\n\n>\n> This being said, if that ends up being helpful, we could restore the\n> instance() method but simply make it return self_. The method would thus\n> return nullptr before the camera manager instance is constructed, or\n> after it gets deleted. For now we don't have a need for that so I've\n> simply dropped the instance() method.\n>\n\nAs long as application instantiating multiple camera manager instance\nare loudly notified (and crashing is loud enough), I'm fine with this\napproach.\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n> > diff --git a/include/libcamera/camera_manager.h b/include/libcamera/camera_manager.h\n> > index ff7d4c7c6745..a29a24f5e37a 100644\n> > --- a/include/libcamera/camera_manager.h\n> > +++ b/include/libcamera/camera_manager.h\n> > @@ -33,6 +33,7 @@ public:\n> > void removeCamera(Camera *camera);\n> >\n> > static CameraManager *instance();\n> > + static void destroy();\n> > static const std::string &version() { return version_; }\n> >\n> > void setEventDispatcher(std::unique_ptr dispatcher);\n> > @@ -49,6 +50,8 @@ private:\n> > std::vector> cameras_;\n> >\n> > static const std::string version_;\n> > +\n> > + static CameraManager *manager_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> > index 4a880684c5cb..ed175ccc5860 100644\n> > --- a/src/libcamera/camera_manager.cpp\n> > +++ b/src/libcamera/camera_manager.cpp\n> > @@ -26,6 +26,8 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(Camera)\n> >\n> > +CameraManager *CameraManager::manager_ = nullptr;\n> > +\n> > /**\n> > * \\class CameraManager\n> > * \\brief Provide access and manage all cameras in the system\n> > @@ -223,8 +225,16 @@ void CameraManager::removeCamera(Camera *camera)\n> > */\n> > CameraManager *CameraManager::instance()\n> > {\n> > - static CameraManager manager;\n> > - return &manager;\n> > + if (!manager_)\n> > + manager_ = new CameraManager();\n> > +\n> > + return manager_;\n> > +}\n> > +\n> > +void CameraManager::destroy()\n> > +{\n> > + delete manager_;\n> > + manager_ = nullptr;\n> > }\n> >\n> > /**\n> >\n> > > }\n> > >\n> > > CameraManager::~CameraManager()\n> > > {\n> > > +\tself_ = nullptr;\n> > > }\n> > >\n> > > /**\n> > > @@ -212,21 +223,6 @@ void CameraManager::removeCamera(Camera *camera)\n> > > \t}\n> > > }\n> > >\n> > > -/**\n> > > - * \\brief Retrieve the camera manager instance\n> > > - *\n> > > - * The CameraManager is a singleton and can't be constructed manually. This\n> > > - * function shall instead be used to retrieve the single global instance of the\n> > > - * manager.\n> > > - *\n> > > - * \\return The camera manager instance\n> > > - */\n> > > -CameraManager *CameraManager::instance()\n> > > -{\n> > > -\tstatic CameraManager manager;\n> > > -\treturn &manager;\n> > > -}\n> > > -\n> > > /**\n> > > * \\fn const std::string &CameraManager::version()\n> > > * \\brief Retrieve the libcamera version string\n> > > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp\n> > > index 05d3b77e9edb..a7ff5c52663b 100644\n> > > --- a/src/qcam/main.cpp\n> > > +++ b/src/qcam/main.cpp\n> > > @@ -63,7 +63,7 @@ int main(int argc, char **argv)\n> > > \tsigaction(SIGINT, &sa, nullptr);\n> > >\n> > > \tstd::unique_ptr dispatcher(new QtEventDispatcher());\n> > > -\tCameraManager *cm = CameraManager::instance();\n> > > +\tCameraManager *cm = new CameraManager();\n> > > \tcm->setEventDispatcher(std::move(dispatcher));\n> > >\n> > > \tret = cm->start();\n> > > @@ -79,5 +79,7 @@ int main(int argc, char **argv)\n> > > \tdelete mainWindow;\n> > >\n> > > \tcm->stop();\n> > > +\tdelete cm;\n> > > +\n> > > \treturn ret;\n> > > }\n> > > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp\n> > > index 24ff5fe0c64d..0e105414bf46 100644\n> > > --- a/test/camera/camera_test.cpp\n> > > +++ b/test/camera/camera_test.cpp\n> > > @@ -14,7 +14,7 @@ using namespace std;\n> > >\n> > > int CameraTest::init()\n> > > {\n> > > -\tcm_ = CameraManager::instance();\n> > > +\tcm_ = new CameraManager();\n> > >\n> > > \tif (cm_->start()) {\n> > > \t\tcout << \"Failed to start camera manager\" << endl;\n> > > @@ -44,4 +44,5 @@ void CameraTest::cleanup()\n> > > \t}\n> > >\n> > > \tcm_->stop();\n> > > +\tdelete cm_;\n> > > };\n> > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > > index c834edc352f5..f1d79ff8fcfd 100644\n> > > --- a/test/controls/control_list.cpp\n> > > +++ b/test/controls/control_list.cpp\n> > > @@ -21,7 +21,7 @@ class ControlListTest : public Test\n> > > protected:\n> > > \tint init()\n> > > \t{\n> > > -\t\tcm_ = CameraManager::instance();\n> > > +\t\tcm_ = new CameraManager();\n> > >\n> > > \t\tif (cm_->start()) {\n> > > \t\t\tcout << \"Failed to start camera manager\" << endl;\n> > > @@ -203,6 +203,7 @@ protected:\n> > > \t\t}\n> > >\n> > > \t\tcm_->stop();\n> > > +\t\tdelete cm_;\n> > > \t}\n> > >\n> > > private:\n> > > diff --git a/test/list-cameras.cpp b/test/list-cameras.cpp\n> > > index 070cbf2be977..55551d7e7e10 100644\n> > > --- a/test/list-cameras.cpp\n> > > +++ b/test/list-cameras.cpp\n> > > @@ -20,8 +20,8 @@ class ListTest : public Test\n> > > protected:\n> > > \tint init()\n> > > \t{\n> > > -\t\tcm = CameraManager::instance();\n> > > -\t\tcm->start();\n> > > +\t\tcm_ = new CameraManager();\n> > > +\t\tcm_->start();\n> > >\n> > > \t\treturn 0;\n> > > \t}\n> > > @@ -30,7 +30,7 @@ protected:\n> > > \t{\n> > > \t\tunsigned int count = 0;\n> > >\n> > > -\t\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> > > +\t\tfor (const std::shared_ptr &camera : cm_->cameras()) {\n> > > \t\t\tcout << \"- \" << camera->name() << endl;\n> > > \t\t\tcount++;\n> > > \t\t}\n> > > @@ -40,11 +40,12 @@ protected:\n> > >\n> > > \tvoid cleanup()\n> > > \t{\n> > > -\t\tcm->stop();\n> > > +\t\tcm_->stop();\n> > > +\t\tdelete cm_;\n> > > \t}\n> > >\n> > > private:\n> > > -\tCameraManager *cm;\n> > > +\tCameraManager *cm_;\n> > > };\n> > >\n> > > TEST_REGISTER(ListTest)\n> > > diff --git a/test/pipeline/ipu3/ipu3_pipeline_test.cpp b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > > index 1d4cc4d4950b..8bfcd609a071 100644\n> > > --- a/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > > +++ b/test/pipeline/ipu3/ipu3_pipeline_test.cpp\n> > > @@ -92,7 +92,7 @@ int IPU3PipelineTest::init()\n> > >\n> > > \tenumerator.reset(nullptr);\n> > >\n> > > -\tcameraManager_ = CameraManager::instance();\n> > > +\tcameraManager_ = new CameraManager();\n> > > \tret = cameraManager_->start();\n> > > \tif (ret) {\n> > > \t\tcerr << \"Failed to start the CameraManager\" << endl;\n> > > @@ -120,6 +120,7 @@ int IPU3PipelineTest::run()\n> > > void IPU3PipelineTest::cleanup()\n> > > {\n> > > \tcameraManager_->stop();\n> > > +\tdelete cameraManager_;\n> > > }\n> > >\n> > > TEST_REGISTER(IPU3PipelineTest)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4686560C1E\n\tfor ;\n\tMon, 19 Aug 2019 18:03:33 +0200 (CEST)","from uno.localdomain (unknown [87.18.63.98])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 3C51740005;\n\tMon, 19 Aug 2019 16:03:32 +0000 (UTC)"],"X-Originating-IP":"87.18.63.98","Date":"Mon, 19 Aug 2019 18:05:00 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190819160500.ffdxswtspc7e7cl5@uno.localdomain>","References":"<20190818011329.14499-1-laurent.pinchart@ideasonboard.com>\n\t<20190818011329.14499-14-laurent.pinchart@ideasonboard.com>\n\t<20190819084000.gpz2ga6dtwevdzww@uno.localdomain>\n\t<20190819153752.GM5011@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"efuf6xu25ak3fc5p\"","Content-Disposition":"inline","In-Reply-To":"<20190819153752.GM5011@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 13/14] libcamera: camera_manager:\n\tConstruct CameraManager instances manually","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 19 Aug 2019 16:03:33 -0000"}}]