[{"id":5190,"web_url":"https://patchwork.libcamera.org/comment/5190/","msgid":"<20200612100131.GG5957@pendragon.ideasonboard.com>","date":"2020-06-12T10:01:31","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:\n> This placeholder for the pipeline-handlers created by CameraManager,\n> was responsible to hold a reference to pipeline-handlers which were\n> not getting dropped anywhere in the code-path. Instead of introducing\n> a fix of decrementing the reference, it is decided to eliminate this\n> vector entirely from the CameraManager. This is due to the fact that\n> the vector does not seem to have much use in CameraManager and thus\n> reducing futile book-keeping.\n\nI think the change is good, but \"does not seem to have much use\" sounds\nlike you're not really sure :-)\n\nThe pipes_ vector has been there pretty much since day 1, and served the\npurpose of storing pipeline handler instances when they were not yet\nreferenced by the Camera class. This was used to retrieve cameras, and\nto delete pipeline handler instances when stopping the camera manager.\nIn\n\ncommit f3695e9b09ce4a88d6e0480d9e5058673af34a49\nAuthor: Laurent Pinchart \nDate: Thu Jan 3 13:10:37 2019 +0200\n\n libcamera: camera_manager: Register cameras with the camera manager\n\ncameras were registered directly with the camera manager, and the pipes_\nvector was only used to delete pipeline handler instances. In\n\ncommit 5b02e03199b79165086135236d8fb9b2c673aae1\nAuthor: Niklas Söderlund \nDate: Tue Jan 22 16:31:39 2019 +0100\n\n libcamera: camera: Associate cameras with their pipeline handler\n\nthe pipeline handler instances got stored in a shared_ptr, referenced\nfrom both the pipes_ vector and the Camera class. The explicit delete\nwas removed, and pipes_ was simply cleared instead. We could have\nremoved the pipes_ vector at that point, as pipeline handlers were\nreferenced from the Camera class.\n\nIt would be nice to summarize this in the commit message to explain why\nthe pipes_ vector isn't needed anymore.\n\n> Since the vector is eliminated, there are no unchecked reference of the\n> pipeline-handlers. This fixes the initial issue of dangling driver directories\n> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'\n> operation while the CameraManager is running. The directories were still kept\n> around even after 'unbind' because of the pipeline-handler's unchecked reference\n> holding onto them.\n\nHave you tried to run the hotplug test under valgrind ? There are lots\nof use-after-free issues with this patch applied:\n\n==18826== Thread 2:\n==18826== Invalid read of size 8\n==18826== at 0x4A5A06C: std::__1::vector, std::__1::allocator > >::size() const (vector:656)\n==18826== by 0x4A59778: std::__1::vector, std::__1::allocator > >::clear() (vector:771)\n==18826== by 0x4A5887D: libcamera::PipelineHandler::disconnect() (pipeline_handler.cpp:571)\n==18826== by 0x4A586C6: libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*) (pipeline_handler.cpp:545)\n==18826== by 0x4A5EC3D: libcamera::BoundMethodMember::invoke(libcamera::MediaDevice*) (bound_method.h:198)\n==18826== by 0x4A5ECC4: void libcamera::BoundMethodArgs::invokePack<0ul>(libcamera::BoundMethodPackBase*, std::__1::integer_sequence) (bound_method.h:124)\n==18826== by 0x4A5EA9C: libcamera::BoundMethodArgs::invokePack(libcamera::BoundMethodPackBase*) (bound_method.h:133)\n==18826== by 0x49ABB26: libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr, bool) (bound_method.cpp:85)\n==18826== by 0x4A5EB85: libcamera::BoundMethodMember::activate(libcamera::MediaDevice*, bool) (bound_method.h:193)\n==18826== by 0x4A0F600: libcamera::Signal::emit(libcamera::MediaDevice*) (signal.h:127)\n==18826== by 0x4A0E589: libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string, std::__1::allocator > const&) (device_enumerator.cpp:290)\n==18826== by 0x4B0C5C1: libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*) (device_enumerator_udev.cpp:344)\n==18826== Address 0x5bd0af8 is 136 bytes inside a block of size 184 free'd\n==18826== at 0x48379CB: free (vg_replace_malloc.c:540)\n==18826== by 0x4AFD391: libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)\n==18826== by 0x4A61E9E: std::__1::default_delete::operator()(libcamera::PipelineHandler*) const (memory:2363)\n==18826== by 0x4A61C3F: std::__1::__shared_ptr_pointer, std::__1::allocator >::__on_zero_shared() (memory:3536)\n==18826== by 0x49AC479: std::__1::__shared_count::__release_shared() (memory:3440)\n==18826== by 0x49AC41E: std::__1::__shared_weak_count::__release_shared() (memory:3482)\n==18826== by 0x49B35DB: std::__1::shared_ptr::~shared_ptr() (memory:4207)\n==18826== by 0x49B0982: libcamera::Camera::Private::~Private() (camera.cpp:300)\n==18826== by 0x49C213A: std::__1::default_delete::operator()(libcamera::Camera::Private*) const (memory:2363)\n==18826== by 0x49C20BE: std::__1::unique_ptr >::reset(libcamera::Camera::Private*) (memory:2618)\n==18826== by 0x49B3878: std::__1::unique_ptr >::~unique_ptr() (memory:2572)\n==18826== by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)\n==18826== Block was alloc'd at\n==18826== at 0x483679F: malloc (vg_replace_malloc.c:309)\n==18826== by 0x4D0F76B: operator new(unsigned long) (in /usr/lib64/libc++.so.1.0)\n==18826== by 0x4AFD409: libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*) (uvcvideo.cpp:562)\n==18826== by 0x4A58A4F: libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*) (pipeline_handler.cpp:640)\n==18826== by 0x49C9A83: libcamera::CameraManager::Private::createPipelineHandlers() (camera_manager.cpp:148)\n==18826== by 0x49C9952: libcamera::CameraManager::Private::init() (camera_manager.cpp:127)\n==18826== by 0x49C97F2: libcamera::CameraManager::Private::run() (camera_manager.cpp:104)\n==18826== by 0x4A71CDC: libcamera::Thread::startThread() (thread.cpp:288)\n==18826== by 0x4A76520: _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_ (type_traits:3480)\n==18826== by 0x4A7642D: void std::__1::__thread_execute >, void (libcamera::Thread::*)(), libcamera::Thread*, 2ul>(std::__1::tuple >, void (libcamera::Thread::*)(), libcamera::Thread*>&, std::__1::__tuple_indices<2ul>) (thread:273)\n==18826== by 0x4A75D95: void* std::__1::__thread_proxy >, void (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)\n==18826== by 0x52B8F3F: start_thread (pthread_create.c:479)\n\nThe change below should fix this. I'll let you add a comment at the\nbeginning of PipelineHandler::disconnect() to explain what is going on\n:-)\n\ndiff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\nindex bad79dcb6219..8358266d83ff 100644\n--- a/src/libcamera/pipeline_handler.cpp\n+++ b/src/libcamera/pipeline_handler.cpp\n@@ -559,7 +559,9 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)\n */\n void PipelineHandler::disconnect()\n {\n-\tfor (std::weak_ptr ptr : cameras_) {\n+\tstd::vector> cameras{ std::move(cameras_) };\n+\n+\tfor (std::weak_ptr ptr : cameras) {\n \t\tstd::shared_ptr camera = ptr.lock();\n \t\tif (!camera)\n \t\t\tcontinue;\n@@ -567,8 +569,6 @@ void PipelineHandler::disconnect()\n \t\tcamera->disconnect();\n \t\tmanager_->removeCamera(camera);\n \t}\n-\n-\tcameras_.clear();\n }\n \n /**\n\n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/camera_manager.cpp | 3 ---\n> 1 file changed, 3 deletions(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 576856a..b8128df 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -63,7 +63,6 @@ private:\n> \tbool initialized_;\n> \tint status_;\n> \n> -\tstd::vector> pipes_;\n> \tstd::unique_ptr enumerator_;\n> \n> \tIPAManager ipaManager_;\n> @@ -144,7 +143,6 @@ int CameraManager::Private::init()\n> \t\t\tLOG(Camera, Debug)\n> \t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n> \t\t\t\t<< \"\\\" matched\";\n> -\t\t\tpipes_.push_back(std::move(pipe));\n> \t\t}\n> \t}\n> \n> @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()\n> \t * they all get destroyed before the device enumerator deletes the\n> \t * media devices.\n> \t */\n\nThe above comment needs to be updated.\n\n-\t * Release all references to cameras and pipeline handlers to ensure\n-\t * they all get destroyed before the device enumerator deletes the\n-\t * media devices.\n+\t * Release all references to cameras to ensure they all get destroyed\n+\t * before the device enumerator deletes the media devices.\n\n> -\tpipes_.clear();\n> \tcameras_.clear();\n> \n> \tenumerator_.reset(nullptr);","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48A3460C4E\n\tfor ;\n\tFri, 12 Jun 2020 12:01:53 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DA16B24F;\n\tFri, 12 Jun 2020 12:01:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"j8OR5LpI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591956112;\n\tbh=XqXFzi/gNvTaAf0UQ6x54+6GxLcrGrKlmaNKI6hmOlY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j8OR5LpI81tX8ho1ZH0PFxFNe4VcTOKh5JznCE9tCS+ZzM/A+WkMUGGQUBgSkIzdf\n\ttZ/3tAxpROeoNrXyhwnhvOMFMB4d2tu2Ey5T4EkP/iz0An9ecj0we43fyaQdfieETI\n\t2FyqF2pf91zsgGcuMa7dre9Yfb0QLJezg+G9Hllw=","Date":"Fri, 12 Jun 2020 13:01:31 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200612100131.GG5957@pendragon.ideasonboard.com>","References":"<20200611171528.9381-1-email@uajain.com>\n\t<20200611171528.9381-2-email@uajain.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200611171528.9381-2-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 12 Jun 2020 10:01:53 -0000"}},{"id":5204,"web_url":"https://patchwork.libcamera.org/comment/5204/","msgid":"","date":"2020-06-15T13:40:10","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nOn 6/12/20 3:31 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:\n>> This placeholder for the pipeline-handlers created by CameraManager,\n>> was responsible to hold a reference to pipeline-handlers which were\n>> not getting dropped anywhere in the code-path. Instead of introducing\n>> a fix of decrementing the reference, it is decided to eliminate this\n>> vector entirely from the CameraManager. This is due to the fact that\n>> the vector does not seem to have much use in CameraManager and thus\n>> reducing futile book-keeping.\n> I think the change is good, but \"does not seem to have much use\" sounds\n> like you're not really sure :-)\n>\n> The pipes_ vector has been there pretty much since day 1, and served the\n> purpose of storing pipeline handler instances when they were not yet\n> referenced by the Camera class. This was used to retrieve cameras, and\n> to delete pipeline handler instances when stopping the camera manager.\n> In\n>\n> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49\n> Author: Laurent Pinchart \n> Date: Thu Jan 3 13:10:37 2019 +0200\n>\n> libcamera: camera_manager: Register cameras with the camera manager\n>\n> cameras were registered directly with the camera manager, and the pipes_\n> vector was only used to delete pipeline handler instances. In\n>\n> commit 5b02e03199b79165086135236d8fb9b2c673aae1\n> Author: Niklas Söderlund \n> Date: Tue Jan 22 16:31:39 2019 +0100\n>\n> libcamera: camera: Associate cameras with their pipeline handler\n>\n> the pipeline handler instances got stored in a shared_ptr, referenced\n> from both the pipes_ vector and the Camera class. The explicit delete\n> was removed, and pipes_ was simply cleared instead. We could have\n> removed the pipes_ vector at that point, as pipeline handlers were\n> referenced from the Camera class.\n>\n> It would be nice to summarize this in the commit message to explain why\n> the pipes_ vector isn't needed anymore.\n>\n>> Since the vector is eliminated, there are no unchecked reference of the\n>> pipeline-handlers. This fixes the initial issue of dangling driver directories\n>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'\n>> operation while the CameraManager is running. The directories were still kept\n>> around even after 'unbind' because of the pipeline-handler's unchecked reference\n>> holding onto them.\n> Have you tried to run the hotplug test under valgrind ? There are lots\n> of use-after-free issues with this patch applied:\n>\n> ==18826== Thread 2:\n> ==18826== Invalid read of size 8\n> ==18826== at 0x4A5A06C: std::__1::vector, std::__1::allocator > >::size() const (vector:656)\n> ==18826== by 0x4A59778: std::__1::vector, std::__1::allocator > >::clear() (vector:771)\n> ==18826== by 0x4A5887D: libcamera::PipelineHandler::disconnect() (pipeline_handler.cpp:571)\n> ==18826== by 0x4A586C6: libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*) (pipeline_handler.cpp:545)\n> ==18826== by 0x4A5EC3D: libcamera::BoundMethodMember::invoke(libcamera::MediaDevice*) (bound_method.h:198)\n> ==18826== by 0x4A5ECC4: void libcamera::BoundMethodArgs::invokePack<0ul>(libcamera::BoundMethodPackBase*, std::__1::integer_sequence) (bound_method.h:124)\n> ==18826== by 0x4A5EA9C: libcamera::BoundMethodArgs::invokePack(libcamera::BoundMethodPackBase*) (bound_method.h:133)\n> ==18826== by 0x49ABB26: libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr, bool) (bound_method.cpp:85)\n> ==18826== by 0x4A5EB85: libcamera::BoundMethodMember::activate(libcamera::MediaDevice*, bool) (bound_method.h:193)\n> ==18826== by 0x4A0F600: libcamera::Signal::emit(libcamera::MediaDevice*) (signal.h:127)\n> ==18826== by 0x4A0E589: libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string, std::__1::allocator > const&) (device_enumerator.cpp:290)\n> ==18826== by 0x4B0C5C1: libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*) (device_enumerator_udev.cpp:344)\n> ==18826== Address 0x5bd0af8 is 136 bytes inside a block of size 184 free'd\n> ==18826== at 0x48379CB: free (vg_replace_malloc.c:540)\n> ==18826== by 0x4AFD391: libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)\n> ==18826== by 0x4A61E9E: std::__1::default_delete::operator()(libcamera::PipelineHandler*) const (memory:2363)\n> ==18826== by 0x4A61C3F: std::__1::__shared_ptr_pointer, std::__1::allocator >::__on_zero_shared() (memory:3536)\n> ==18826== by 0x49AC479: std::__1::__shared_count::__release_shared() (memory:3440)\n> ==18826== by 0x49AC41E: std::__1::__shared_weak_count::__release_shared() (memory:3482)\n> ==18826== by 0x49B35DB: std::__1::shared_ptr::~shared_ptr() (memory:4207)\n> ==18826== by 0x49B0982: libcamera::Camera::Private::~Private() (camera.cpp:300)\n> ==18826== by 0x49C213A: std::__1::default_delete::operator()(libcamera::Camera::Private*) const (memory:2363)\n> ==18826== by 0x49C20BE: std::__1::unique_ptr >::reset(libcamera::Camera::Private*) (memory:2618)\n> ==18826== by 0x49B3878: std::__1::unique_ptr >::~unique_ptr() (memory:2572)\n> ==18826== by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)\n> ==18826== Block was alloc'd at\n> ==18826== at 0x483679F: malloc (vg_replace_malloc.c:309)\n> ==18826== by 0x4D0F76B: operator new(unsigned long) (in /usr/lib64/libc++.so.1.0)\n> ==18826== by 0x4AFD409: libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*) (uvcvideo.cpp:562)\n> ==18826== by 0x4A58A4F: libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*) (pipeline_handler.cpp:640)\n> ==18826== by 0x49C9A83: libcamera::CameraManager::Private::createPipelineHandlers() (camera_manager.cpp:148)\n> ==18826== by 0x49C9952: libcamera::CameraManager::Private::init() (camera_manager.cpp:127)\n> ==18826== by 0x49C97F2: libcamera::CameraManager::Private::run() (camera_manager.cpp:104)\n> ==18826== by 0x4A71CDC: libcamera::Thread::startThread() (thread.cpp:288)\n> ==18826== by 0x4A76520: _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_ (type_traits:3480)\n> ==18826== by 0x4A7642D: void std::__1::__thread_execute >, void (libcamera::Thread::*)(), libcamera::Thread*, 2ul>(std::__1::tuple >, void (libcamera::Thread::*)(), libcamera::Thread*>&, std::__1::__tuple_indices<2ul>) (thread:273)\n> ==18826== by 0x4A75D95: void* std::__1::__thread_proxy >, void (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)\n> ==18826== by 0x52B8F3F: start_thread (pthread_create.c:479)\n>\n> The change below should fix this. I'll let you add a comment at the\n> beginning of PipelineHandler::disconnect() to explain what is going on\n> :-)\n\nI will go with something like the following:\n\n+       /* Since cameras_ holds a weak reference of the cameras,\n+        * we just need to empty the vector here instead of \ncameras_.clear() it.\n+        */\n\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index bad79dcb6219..8358266d83ff 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -559,7 +559,9 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)\n> */\n> void PipelineHandler::disconnect()\n> {\n> -\tfor (std::weak_ptr ptr : cameras_) {\n> +\tstd::vector> cameras{ std::move(cameras_) };\n> +\n> +\tfor (std::weak_ptr ptr : cameras) {\n> \t\tstd::shared_ptr camera = ptr.lock();\n> \t\tif (!camera)\n> \t\t\tcontinue;\n> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()\n> \t\tcamera->disconnect();\n> \t\tmanager_->removeCamera(camera);\n> \t}\n> -\n> -\tcameras_.clear();\n> }\n> \n> /**\n\nQuick question: You mentioned on IRC that this can be folded into a patch.\n\nDid you mean squash it into existing patch or treat this as a separate \npatch?\n\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/libcamera/camera_manager.cpp | 3 ---\n>> 1 file changed, 3 deletions(-)\n>>\n>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n>> index 576856a..b8128df 100644\n>> --- a/src/libcamera/camera_manager.cpp\n>> +++ b/src/libcamera/camera_manager.cpp\n>> @@ -63,7 +63,6 @@ private:\n>> \tbool initialized_;\n>> \tint status_;\n>> \n>> -\tstd::vector> pipes_;\n>> \tstd::unique_ptr enumerator_;\n>> \n>> \tIPAManager ipaManager_;\n>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()\n>> \t\t\tLOG(Camera, Debug)\n>> \t\t\t\t<< \"Pipeline handler \\\"\" << factory->name()\n>> \t\t\t\t<< \"\\\" matched\";\n>> -\t\t\tpipes_.push_back(std::move(pipe));\n>> \t\t}\n>> \t}\n>> \n>> @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()\n>> \t * they all get destroyed before the device enumerator deletes the\n>> \t * media devices.\n>> \t */\n> The above comment needs to be updated.\n>\n> -\t * Release all references to cameras and pipeline handlers to ensure\n> -\t * they all get destroyed before the device enumerator deletes the\n> -\t * media devices.\n> +\t * Release all references to cameras to ensure they all get destroyed\n> +\t * before the device enumerator deletes the media devices.\n>\n>> -\tpipes_.clear();\n>> \tcameras_.clear();\n>> \n>> \tenumerator_.reset(nullptr);","headers":{"Return-Path":"","Received":["from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD7B2603D8\n\tfor ;\n\tMon, 15 Jun 2020 15:40:12 +0200 (CEST)","by filterdrecv-p3iad2-784dbb6bd8-5m689 with SMTP id\n\tfilterdrecv-p3iad2-784dbb6bd8-5m689-19-5EE77A3A-B4\n\t2020-06-15 13:40:10.913402757 +0000 UTC m=+1013594.839828343","from mail.uajain.com (unknown)\n\tby ismtpd0002p1maa1.sendgrid.net (SG) with ESMTP\n\tid ubryMC1WQPGsokUekZfRZg Mon, 15 Jun 2020 13:40:10.326 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=uajain.com\n\theader.i=@uajain.com header.b=\"sn5myXhR\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=subject:references:from:mime-version:in-reply-to:to:cc:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=I2N5I81tzkQqlpuVCJEC9c2uIp9x1B+0No62vrPHLl4=;\n\tb=sn5myXhRutILjnxdopeJqxlqAcMx90brrChBcA+oFVm5l061MiJ55iWM8qUJehsVbGMX\n\tKMMJ7LDRM8scx3otAmsZPUdIEa/19KQ4MIneokDQuSHyohYA2HBTsHav0cMW1tZ9tEMirR\n\tKlaIPZkhndilhs1E2FFuuUg03URSYKLmI=","References":"<20200611171528.9381-1-email@uajain.com>\n\t<20200611171528.9381-2-email@uajain.com>\n\t<20200612100131.GG5957@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 15 Jun 2020 13:40:10 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<20200612100131.GG5957@pendragon.ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcbjgBrPKwU6zsq5Nc1XDaVo+1reyEYkYkwPqpycl9k/7fhNiOuNs8pY7LV/OZkSUFGso2AfN4PPO/qhbgSS+cqioRZp/eh3jcsGOV1CYcBcstSdLrvGNvUcm7o+ynwns49ZaoTkugRcYIqRHUypXuTUDocQDBHDLRPtuYk1VzDX4Nl3twPbhPBWyTHROWz3AmOvWQEH1iQ6B4A9riTXqrGA==","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 15 Jun 2020 13:40:13 -0000"}},{"id":5205,"web_url":"https://patchwork.libcamera.org/comment/5205/","msgid":"<74c6771c-7d59-9f1a-ce66-06a429d022ca@ideasonboard.com>","date":"2020-06-15T14:01:51","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 15/06/2020 14:40, Umang Jain wrote:\n> Hi Laurent,\n> \n> On 6/12/20 3:31 PM, Laurent Pinchart wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:\n>>> This placeholder for the pipeline-handlers created by CameraManager,\n>>> was responsible to hold a reference to pipeline-handlers which were\n>>> not getting dropped anywhere in the code-path. Instead of introducing\n>>> a fix of decrementing the reference, it is decided to eliminate this\n>>> vector entirely from  the CameraManager. This is due to the fact that\n>>> the vector does not seem to have much use in CameraManager and thus\n>>> reducing futile book-keeping.\n>> I think the change is good, but \"does not seem to have much use\" sounds\n>> like you're not really sure :-)\n>>\n>> The pipes_ vector has been there pretty much since day 1, and served the\n>> purpose of storing pipeline handler instances when they were not yet\n>> referenced by the Camera class. This was used to retrieve cameras, and\n>> to delete pipeline handler instances when stopping the camera manager.\n>> In\n>>\n>> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49\n>> Author: Laurent Pinchart \n>> Date:   Thu Jan 3 13:10:37 2019 +0200\n>>\n>>      libcamera: camera_manager: Register cameras with the camera manager\n>>\n>> cameras were registered directly with the camera manager, and the pipes_\n>> vector was only used to delete pipeline handler instances. In\n>>\n>> commit 5b02e03199b79165086135236d8fb9b2c673aae1\n>> Author: Niklas Söderlund \n>> Date:   Tue Jan 22 16:31:39 2019 +0100\n>>\n>>      libcamera: camera: Associate cameras with their pipeline handler\n>>\n>> the pipeline handler instances got stored in a shared_ptr, referenced\n>> from both the pipes_ vector and the Camera class. The explicit delete\n>> was removed, and pipes_ was simply cleared instead. We could have\n>> removed the pipes_ vector at that point, as pipeline handlers were\n>> referenced from the Camera class.\n>>\n>> It would be nice to summarize this in the commit message to explain why\n>> the pipes_ vector isn't needed anymore.\n>>\n>>> Since the vector is eliminated, there are no unchecked reference of the\n>>> pipeline-handlers. This fixes the initial issue of dangling driver\n>>> directories\n>>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on\n>>> 'unbind' → 'bind'\n>>> operation while the CameraManager is running. The directories were\n>>> still kept\n>>> around even after 'unbind' because of the pipeline-handler's\n>>> unchecked reference\n>>> holding onto them.\n>> Have you tried to run the hotplug test under valgrind ? There are lots\n>> of use-after-free issues with this patch applied:\n>>\n>> ==18826== Thread 2:\n>> ==18826== Invalid read of size 8\n>> ==18826==    at 0x4A5A06C:\n>> std::__1::vector,\n>> std::__1::allocator > >::size()\n>> const (vector:656)\n>> ==18826==    by 0x4A59778:\n>> std::__1::vector,\n>> std::__1::allocator > >::clear()\n>> (vector:771)\n>> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect()\n>> (pipeline_handler.cpp:571)\n>> ==18826==    by 0x4A586C6:\n>> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)\n>> (pipeline_handler.cpp:545)\n>> ==18826==    by 0x4A5EC3D:\n>> libcamera::BoundMethodMember> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)\n>> (bound_method.h:198)\n>> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,\n>> std::__1::integer_sequence) (bound_method.h:124)\n>> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)\n>> (bound_method.h:133)\n>> ==18826==    by 0x49ABB26:\n>> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr,\n>> bool) (bound_method.cpp:85)\n>> ==18826==    by 0x4A5EB85:\n>> libcamera::BoundMethodMember> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)\n>> (bound_method.h:193)\n>> ==18826==    by 0x4A0F600:\n>> libcamera::Signal::emit(libcamera::MediaDevice*)\n>> (signal.h:127)\n>> ==18826==    by 0x4A0E589:\n>> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string> std::__1::char_traits, std::__1::allocator > const&)\n>> (device_enumerator.cpp:290)\n>> ==18826==    by 0x4B0C5C1:\n>> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)\n>> (device_enumerator_udev.cpp:344)\n>> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184\n>> free'd\n>> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)\n>> ==18826==    by 0x4AFD391:\n>> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)\n>> ==18826==    by 0x4A61E9E:\n>> std::__1::default_delete::operator()(libcamera::PipelineHandler*)\n>> const (memory:2363)\n>> ==18826==    by 0x4A61C3F:\n>> std::__1::__shared_ptr_pointer> std::__1::default_delete,\n>> std::__1::allocator >::__on_zero_shared()\n>> (memory:3536)\n>> ==18826==    by 0x49AC479:\n>> std::__1::__shared_count::__release_shared() (memory:3440)\n>> ==18826==    by 0x49AC41E:\n>> std::__1::__shared_weak_count::__release_shared() (memory:3482)\n>> ==18826==    by 0x49B35DB:\n>> std::__1::shared_ptr::~shared_ptr()\n>> (memory:4207)\n>> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private()\n>> (camera.cpp:300)\n>> ==18826==    by 0x49C213A:\n>> std::__1::default_delete::operator()(libcamera::Camera::Private*)\n>> const (memory:2363)\n>> ==18826==    by 0x49C20BE:\n>> std::__1::unique_ptr> std::__1::default_delete\n>> >::reset(libcamera::Camera::Private*) (memory:2618)\n>> ==18826==    by 0x49B3878:\n>> std::__1::unique_ptr> std::__1::default_delete >::~unique_ptr()\n>> (memory:2572)\n>> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)\n>> ==18826==  Block was alloc'd at\n>> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)\n>> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in\n>> /usr/lib64/libc++.so.1.0)\n>> ==18826==    by 0x4AFD409:\n>> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)\n>> (uvcvideo.cpp:562)\n>> ==18826==    by 0x4A58A4F:\n>> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)\n>> (pipeline_handler.cpp:640)\n>> ==18826==    by 0x49C9A83:\n>> libcamera::CameraManager::Private::createPipelineHandlers()\n>> (camera_manager.cpp:148)\n>> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init()\n>> (camera_manager.cpp:127)\n>> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run()\n>> (camera_manager.cpp:104)\n>> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread()\n>> (thread.cpp:288)\n>> ==18826==    by 0x4A76520:\n>> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_\n>> (type_traits:3480)\n>> ==18826==    by 0x4A7642D: void\n>> std::__1::__thread_execute> std::__1::default_delete >, void\n>> (libcamera::Thread::*)(), libcamera::Thread*,\n>> 2ul>(std::__1::tuple> std::__1::default_delete >, void\n>> (libcamera::Thread::*)(), libcamera::Thread*>&,\n>> std::__1::__tuple_indices<2ul>) (thread:273)\n>> ==18826==    by 0x4A75D95: void*\n>> std::__1::__thread_proxy> std::__1::default_delete >, void\n>> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)\n>> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)\n>>\n>> The change below should fix this. I'll let you add a comment at the\n>> beginning of PipelineHandler::disconnect() to explain what is going on\n>> :-)\n> \n> I will go with something like the following:\n> \n> +       /* Since cameras_ holds a weak reference of the cameras,\n> +        * we just need to empty the vector here instead of\n> cameras_.clear() it.\n> +        */\n\nI'm not sure that's quite describing what's happening.\n\n\n\n\n\n>> diff --git a/src/libcamera/pipeline_handler.cpp\n>> b/src/libcamera/pipeline_handler.cpp\n>> index bad79dcb6219..8358266d83ff 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -559,7 +559,9 @@ void\n>> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)\n>>    */\n>>   void PipelineHandler::disconnect()\n>>   {\n>> -    for (std::weak_ptr ptr : cameras_) {\n>> +    std::vector> cameras{ std::move(cameras_) };\n\nThis moves all cameras out of the cameras_ vector - into a new\n'temporary' holding container.\n\nMoving them into the new container, ensures that the references are kept\nconstant, but they are no longer 'held' by the pipeline.\n\nI haven't checked, but I presume that this means below in the loop, the\ncode that was unable to remove something because a reference was held -\nis now able to - because essentially the cameras_ object has been\ncleared before entering the loop - instead of after.\n\n>> +\n>> +    for (std::weak_ptr ptr : cameras) {\n>>           std::shared_ptr camera = ptr.lock();\n>>           if (!camera)\n>>               continue;\n>> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()\n>>           camera->disconnect();\n>>           manager_->removeCamera(camera);\n>>       }\n>> -\n>> -    cameras_.clear();\n\nAnd because the cameras vector above was created with local scope - all\nof it's contents get cleared at this point now automatically.\n\n\n\n>>   }\n>>     /**\n> \n> Quick question: You mentioned on IRC that this can be folded into a patch.\n> \n> Did you mean squash it into existing patch or treat this as a separate\n> patch?\n\nThat usually means squash it into the existing relevant (this current?)\npatch.\n\n\n>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>>   src/libcamera/camera_manager.cpp | 3 ---\n>>>   1 file changed, 3 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/camera_manager.cpp\n>>> b/src/libcamera/camera_manager.cpp\n>>> index 576856a..b8128df 100644\n>>> --- a/src/libcamera/camera_manager.cpp\n>>> +++ b/src/libcamera/camera_manager.cpp\n>>> @@ -63,7 +63,6 @@ private:\n>>>       bool initialized_;\n>>>       int status_;\n>>>   -    std::vector> pipes_;\n>>>       std::unique_ptr enumerator_;\n>>>         IPAManager ipaManager_;\n>>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()\n>>>               LOG(Camera, Debug)\n>>>                   << \"Pipeline handler \\\"\" << factory->name()\n>>>                   << \"\\\" matched\";\n>>> -            pipes_.push_back(std::move(pipe));\n>>>           }\n>>>       }\n>>>   @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()\n>>>        * they all get destroyed before the device enumerator deletes the\n>>>        * media devices.\n>>>        */\n>> The above comment needs to be updated.\n>>\n>> -     * Release all references to cameras and pipeline handlers to ensure\n>> -     * they all get destroyed before the device enumerator deletes the\n>> -     * media devices.\n>> +     * Release all references to cameras to ensure they all get\n>> destroyed\n>> +     * before the device enumerator deletes the media devices.\n>>\n>>> -    pipes_.clear();\n>>>       cameras_.clear();\n>>>         enumerator_.reset(nullptr);\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 perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDDA9603D8\n\tfor ;\n\tMon, 15 Jun 2020 16:01:54 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2706DF9;\n\tMon, 15 Jun 2020 16:01:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PrwjENTg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592229714;\n\tbh=bj8yPXpTxdpccbf/jsIhf3OpIaQzqUPO1TlQUiq08zI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=PrwjENTgmqd+ogS2wtOs+WogohupWehVX48ijO2tEgFYpt6gFtP26kkHxB3AsVMZi\n\t71+6YezIsjHMijzyHiS1HbV7ulQbDVkVnZtJMEi2INJHrmL83cVfqUtYntPSrIk+4w\n\tI5lsQ47OzKtldp1PVJ/XwedB+gLB6LGebc8LXZQo=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Umang Jain ,\n\tLaurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20200611171528.9381-1-email@uajain.com>\n\t<20200611171528.9381-2-email@uajain.com>\n\t<20200612100131.GG5957@pendragon.ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<74c6771c-7d59-9f1a-ce66-06a429d022ca@ideasonboard.com>","Date":"Mon, 15 Jun 2020 15:01:51 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 15 Jun 2020 14:01:55 -0000"}},{"id":5206,"web_url":"https://patchwork.libcamera.org/comment/5206/","msgid":"","date":"2020-06-15T15:02:23","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Kieran,\n\nOn 6/15/20 7:31 PM, Kieran Bingham wrote:\n> Hi Umang,\n>\n> On 15/06/2020 14:40, Umang Jain wrote:\n>> Hi Laurent,\n>>\n>> On 6/12/20 3:31 PM, Laurent Pinchart wrote:\n>>> Hi Umang,\n>>>\n>>> Thank you for the patch.\n>>>\n>>> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:\n>>>> This placeholder for the pipeline-handlers created by CameraManager,\n>>>> was responsible to hold a reference to pipeline-handlers which were\n>>>> not getting dropped anywhere in the code-path. Instead of introducing\n>>>> a fix of decrementing the reference, it is decided to eliminate this\n>>>> vector entirely from  the CameraManager. This is due to the fact that\n>>>> the vector does not seem to have much use in CameraManager and thus\n>>>> reducing futile book-keeping.\n>>> I think the change is good, but \"does not seem to have much use\" sounds\n>>> like you're not really sure :-)\n>>>\n>>> The pipes_ vector has been there pretty much since day 1, and served the\n>>> purpose of storing pipeline handler instances when they were not yet\n>>> referenced by the Camera class. This was used to retrieve cameras, and\n>>> to delete pipeline handler instances when stopping the camera manager.\n>>> In\n>>>\n>>> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49\n>>> Author: Laurent Pinchart \n>>> Date:   Thu Jan 3 13:10:37 2019 +0200\n>>>\n>>>      libcamera: camera_manager: Register cameras with the camera manager\n>>>\n>>> cameras were registered directly with the camera manager, and the pipes_\n>>> vector was only used to delete pipeline handler instances. In\n>>>\n>>> commit 5b02e03199b79165086135236d8fb9b2c673aae1\n>>> Author: Niklas Söderlund \n>>> Date:   Tue Jan 22 16:31:39 2019 +0100\n>>>\n>>>      libcamera: camera: Associate cameras with their pipeline handler\n>>>\n>>> the pipeline handler instances got stored in a shared_ptr, referenced\n>>> from both the pipes_ vector and the Camera class. The explicit delete\n>>> was removed, and pipes_ was simply cleared instead. We could have\n>>> removed the pipes_ vector at that point, as pipeline handlers were\n>>> referenced from the Camera class.\n>>>\n>>> It would be nice to summarize this in the commit message to explain why\n>>> the pipes_ vector isn't needed anymore.\n>>>\n>>>> Since the vector is eliminated, there are no unchecked reference of the\n>>>> pipeline-handlers. This fixes the initial issue of dangling driver\n>>>> directories\n>>>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on\n>>>> 'unbind' → 'bind'\n>>>> operation while the CameraManager is running. The directories were\n>>>> still kept\n>>>> around even after 'unbind' because of the pipeline-handler's\n>>>> unchecked reference\n>>>> holding onto them.\n>>> Have you tried to run the hotplug test under valgrind ? There are lots\n>>> of use-after-free issues with this patch applied:\n>>>\n>>> ==18826== Thread 2:\n>>> ==18826== Invalid read of size 8\n>>> ==18826==    at 0x4A5A06C:\n>>> std::__1::vector,\n>>> std::__1::allocator > >::size()\n>>> const (vector:656)\n>>> ==18826==    by 0x4A59778:\n>>> std::__1::vector,\n>>> std::__1::allocator > >::clear()\n>>> (vector:771)\n>>> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect()\n>>> (pipeline_handler.cpp:571)\n>>> ==18826==    by 0x4A586C6:\n>>> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)\n>>> (pipeline_handler.cpp:545)\n>>> ==18826==    by 0x4A5EC3D:\n>>> libcamera::BoundMethodMember>> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)\n>>> (bound_method.h:198)\n>>> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs>> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,\n>>> std::__1::integer_sequence) (bound_method.h:124)\n>>> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs>> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)\n>>> (bound_method.h:133)\n>>> ==18826==    by 0x49ABB26:\n>>> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr,\n>>> bool) (bound_method.cpp:85)\n>>> ==18826==    by 0x4A5EB85:\n>>> libcamera::BoundMethodMember>> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)\n>>> (bound_method.h:193)\n>>> ==18826==    by 0x4A0F600:\n>>> libcamera::Signal::emit(libcamera::MediaDevice*)\n>>> (signal.h:127)\n>>> ==18826==    by 0x4A0E589:\n>>> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string>> std::__1::char_traits, std::__1::allocator > const&)\n>>> (device_enumerator.cpp:290)\n>>> ==18826==    by 0x4B0C5C1:\n>>> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)\n>>> (device_enumerator_udev.cpp:344)\n>>> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184\n>>> free'd\n>>> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)\n>>> ==18826==    by 0x4AFD391:\n>>> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)\n>>> ==18826==    by 0x4A61E9E:\n>>> std::__1::default_delete::operator()(libcamera::PipelineHandler*)\n>>> const (memory:2363)\n>>> ==18826==    by 0x4A61C3F:\n>>> std::__1::__shared_ptr_pointer>> std::__1::default_delete,\n>>> std::__1::allocator >::__on_zero_shared()\n>>> (memory:3536)\n>>> ==18826==    by 0x49AC479:\n>>> std::__1::__shared_count::__release_shared() (memory:3440)\n>>> ==18826==    by 0x49AC41E:\n>>> std::__1::__shared_weak_count::__release_shared() (memory:3482)\n>>> ==18826==    by 0x49B35DB:\n>>> std::__1::shared_ptr::~shared_ptr()\n>>> (memory:4207)\n>>> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private()\n>>> (camera.cpp:300)\n>>> ==18826==    by 0x49C213A:\n>>> std::__1::default_delete::operator()(libcamera::Camera::Private*)\n>>> const (memory:2363)\n>>> ==18826==    by 0x49C20BE:\n>>> std::__1::unique_ptr>> std::__1::default_delete\n>>>> ::reset(libcamera::Camera::Private*) (memory:2618)\n>>> ==18826==    by 0x49B3878:\n>>> std::__1::unique_ptr>> std::__1::default_delete >::~unique_ptr()\n>>> (memory:2572)\n>>> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)\n>>> ==18826==  Block was alloc'd at\n>>> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)\n>>> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in\n>>> /usr/lib64/libc++.so.1.0)\n>>> ==18826==    by 0x4AFD409:\n>>> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)\n>>> (uvcvideo.cpp:562)\n>>> ==18826==    by 0x4A58A4F:\n>>> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)\n>>> (pipeline_handler.cpp:640)\n>>> ==18826==    by 0x49C9A83:\n>>> libcamera::CameraManager::Private::createPipelineHandlers()\n>>> (camera_manager.cpp:148)\n>>> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init()\n>>> (camera_manager.cpp:127)\n>>> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run()\n>>> (camera_manager.cpp:104)\n>>> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread()\n>>> (thread.cpp:288)\n>>> ==18826==    by 0x4A76520:\n>>> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_\n>>> (type_traits:3480)\n>>> ==18826==    by 0x4A7642D: void\n>>> std::__1::__thread_execute>> std::__1::default_delete >, void\n>>> (libcamera::Thread::*)(), libcamera::Thread*,\n>>> 2ul>(std::__1::tuple>> std::__1::default_delete >, void\n>>> (libcamera::Thread::*)(), libcamera::Thread*>&,\n>>> std::__1::__tuple_indices<2ul>) (thread:273)\n>>> ==18826==    by 0x4A75D95: void*\n>>> std::__1::__thread_proxy>> std::__1::default_delete >, void\n>>> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)\n>>> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)\n>>>\n>>> The change below should fix this. I'll let you add a comment at the\n>>> beginning of PipelineHandler::disconnect() to explain what is going on\n>>> :-)\n>> I will go with something like the following:\n>>\n>> +       /* Since cameras_ holds a weak reference of the cameras,\n>> +        * we just need to empty the vector here instead of\n>> cameras_.clear() it.\n>> +        */\n> I'm not sure that's quite describing what's happening.\n>\n>\n>\n>\n>\n>>> diff --git a/src/libcamera/pipeline_handler.cpp\n>>> b/src/libcamera/pipeline_handler.cpp\n>>> index bad79dcb6219..8358266d83ff 100644\n>>> --- a/src/libcamera/pipeline_handler.cpp\n>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>> @@ -559,7 +559,9 @@ void\n>>> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)\n>>>    */\n>>>   void PipelineHandler::disconnect()\n>>>   {\n>>> -    for (std::weak_ptr ptr : cameras_) {\n>>> +    std::vector> cameras{ std::move(cameras_) };\n> This moves all cameras out of the cameras_ vector - into a new\n> 'temporary' holding container.\n>\n> Moving them into the new container, ensures that the references are kept\n> constant, but they are no longer 'held' by the pipeline.\n\nhmm, cameras_ is a vector of weak_ptr references, right? According to \nthe concepts I have\n\nin my head (:D), pipeline doesn't _really_ hold a (strong)reference \nthrough this vector.\n\nHence, isn't moving from cameras_ to temporary container merely to avoid \n.clear()  call below?\n\n>\n> I haven't checked, but I presume that this means below in the loop, the\n> code that was unable to remove something because a reference was held -\n> is now able to - because essentially the cameras_ object has been\n> cleared before entering the loop - instead of after.\n>\n>>> +\n>>> +    for (std::weak_ptr ptr : cameras) {\n>>>           std::shared_ptr camera = ptr.lock();\n>>>           if (!camera)\n>>>               continue;\n>>> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()\n>>>           camera->disconnect();\n>>>           manager_->removeCamera(camera);\n>>>       }\n>>> -\n>>> -    cameras_.clear();\n> And because the cameras vector above was created with local scope - all\n> of it's contents get cleared at this point now automatically.\n\nYes, but according to docs online, .clear() also tries to destroy the \nobjects in the vector\n\nand I am under the impression that, since they are weak-references here, \nthey are\n\n_not_ supposed to be destroyed(or decrement the refcount) the objects. \n(that should happen\n\nautomatically when refcount drops to 0).\n\n>\n>\n>\n>>>   }\n>>>     /**\n>> Quick question: You mentioned on IRC that this can be folded into a patch.\n>>\n>> Did you mean squash it into existing patch or treat this as a separate\n>> patch?\n> That usually means squash it into the existing relevant (this current?)\n> patch.\n>\n>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>>   src/libcamera/camera_manager.cpp | 3 ---\n>>>>   1 file changed, 3 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/camera_manager.cpp\n>>>> b/src/libcamera/camera_manager.cpp\n>>>> index 576856a..b8128df 100644\n>>>> --- a/src/libcamera/camera_manager.cpp\n>>>> +++ b/src/libcamera/camera_manager.cpp\n>>>> @@ -63,7 +63,6 @@ private:\n>>>>       bool initialized_;\n>>>>       int status_;\n>>>>   -    std::vector> pipes_;\n>>>>       std::unique_ptr enumerator_;\n>>>>         IPAManager ipaManager_;\n>>>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()\n>>>>               LOG(Camera, Debug)\n>>>>                   << \"Pipeline handler \\\"\" << factory->name()\n>>>>                   << \"\\\" matched\";\n>>>> -            pipes_.push_back(std::move(pipe));\n>>>>           }\n>>>>       }\n>>>>   @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()\n>>>>        * they all get destroyed before the device enumerator deletes the\n>>>>        * media devices.\n>>>>        */\n>>> The above comment needs to be updated.\n>>>\n>>> -     * Release all references to cameras and pipeline handlers to ensure\n>>> -     * they all get destroyed before the device enumerator deletes the\n>>> -     * media devices.\n>>> +     * Release all references to cameras to ensure they all get\n>>> destroyed\n>>> +     * before the device enumerator deletes the media devices.\n>>>\n>>>> -    pipes_.clear();\n>>>>       cameras_.clear();\n>>>>         enumerator_.reset(nullptr);\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://u15657259.ct.sendgrid.net/ls/click?upn=8H1KCc2bev8KdIveckpOEBeWjI3THEr-2F8W-2FrEpvXj1fUaD8nZSgfyCwFn-2BKX4QPmXiU9H2IGCLkMp0Cw1NTkpQ-3D-3DDJ-N_C3wFy2Q4UgRsRLDAYieRZ5Z3EhAWyy0-2FkOzyYc6FPc1dn6ROcAJqKXb9hjP566uPJMdELFe3GohTLWwC0myBlrYgTXKbgNWKhCFl6ePGYmSF4nYd6A9-2BWivkIa1mO-2BS8MCKqKHfu-2BFzZmuiY-2FqbM7xe-2FL9w6rjernZd59Uz-2Bc8ddTJ6ISK6R0qrMzttzQX9ED9I-2F9ipg1ivJPnxtOFZW07b2-2Ft3Yxf6CJbdHCKcJ2u3NFuupig2e61AhqFLdvMTA","headers":{"Return-Path":"","Received":["from o1.f.az.sendgrid.net (o1.f.az.sendgrid.net [208.117.55.132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 718EB603D8\n\tfor ;\n\tMon, 15 Jun 2020 17:02:25 +0200 (CEST)","by filterdrecv-p3iad2-784dbb6bd8-vb6nr with SMTP id\n\tfilterdrecv-p3iad2-784dbb6bd8-vb6nr-18-5EE78D7F-EE\n\t2020-06-15 15:02:23.811619665 +0000 UTC m=+1018526.147742256","from mail.uajain.com (unknown)\n\tby ismtpd0007p1hnd1.sendgrid.net (SG) with ESMTP\n\tid ulCMNkW6RsOx4501rShYrQ Mon, 15 Jun 2020 15:02:23.283 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=uajain.com\n\theader.i=@uajain.com header.b=\"aIi5bvwt\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=uajain.com;\n\th=subject:references:from:mime-version:in-reply-to:to:cc:content-type:\n\tcontent-transfer-encoding;\n\ts=s1; bh=BAyOnnUxKdon51eI1HF9325KnP/rnReS1/5rXz82k04=;\n\tb=aIi5bvwt19HGTCYRvlyRoC1iqWpCjy6C5SxVPsI0g79MQWVxFG/vbkz0ySN5+uINz0nY\n\tzNY0Gs/Z8r3AT72dzUjL9LaO+AAB41TimE7QJvUOVKFxThbpiCPKHDnq/8Plrp8Ok7WIwD\n\tzmgyiuw+hGZ297/ps3x8iG8T9o6rwJEIw=","References":"<20200611171528.9381-1-email@uajain.com>\n\t<20200611171528.9381-2-email@uajain.com>\n\t<20200612100131.GG5957@pendragon.ideasonboard.com>\n\t\n\t<74c6771c-7d59-9f1a-ce66-06a429d022ca@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Mon, 15 Jun 2020 15:02:23 +0000 (UTC)","Mime-Version":"1.0","In-Reply-To":"<74c6771c-7d59-9f1a-ce66-06a429d022ca@ideasonboard.com>","X-SG-EID":"1Q40EQ7YGir8a9gjSIAdTjhngY657NMk9ckeo4dbHZDiOpywc/L3L9rFqlwE4KPcbjgBrPKwU6zsq5Nc1XDaVq6tbjcRjNXBOrA5GlvWwDu1dfQO6RM4w4PyUQPPCr/v0jCu6h35AI/uHI65XTQitbWpNYdUAjABO9a1pkheejjz6nwY6kTjJpIxrdpuZiEc7wNaq7jvsB3zvgQ3uAi4QTvz0NuJmCkK3fKYLul4ieUkrMM/w4L1BRDEsANBoWynAOa7UBu7zxyH/pIoWtdz/A==","To":"kieran.bingham@ideasonboard.com, Laurent Pinchart\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 15 Jun 2020 15:02:26 -0000"}},{"id":5207,"web_url":"https://patchwork.libcamera.org/comment/5207/","msgid":"<20200615150528.GA1629@pendragon.ideasonboard.com>","date":"2020-06-15T15:05:28","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Jun 15, 2020 at 03:01:51PM +0100, Kieran Bingham wrote:\n> On 15/06/2020 14:40, Umang Jain wrote:\n> > On 6/12/20 3:31 PM, Laurent Pinchart wrote:\n> >> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:\n> >>> This placeholder for the pipeline-handlers created by CameraManager,\n> >>> was responsible to hold a reference to pipeline-handlers which were\n> >>> not getting dropped anywhere in the code-path. Instead of introducing\n> >>> a fix of decrementing the reference, it is decided to eliminate this\n> >>> vector entirely from  the CameraManager. This is due to the fact that\n> >>> the vector does not seem to have much use in CameraManager and thus\n> >>> reducing futile book-keeping.\n> >> \n> >> I think the change is good, but \"does not seem to have much use\" sounds\n> >> like you're not really sure :-)\n> >>\n> >> The pipes_ vector has been there pretty much since day 1, and served the\n> >> purpose of storing pipeline handler instances when they were not yet\n> >> referenced by the Camera class. This was used to retrieve cameras, and\n> >> to delete pipeline handler instances when stopping the camera manager.\n> >> In\n> >>\n> >> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49\n> >> Author: Laurent Pinchart \n> >> Date:   Thu Jan 3 13:10:37 2019 +0200\n> >>\n> >>      libcamera: camera_manager: Register cameras with the camera manager\n> >>\n> >> cameras were registered directly with the camera manager, and the pipes_\n> >> vector was only used to delete pipeline handler instances. In\n> >>\n> >> commit 5b02e03199b79165086135236d8fb9b2c673aae1\n> >> Author: Niklas Söderlund \n> >> Date:   Tue Jan 22 16:31:39 2019 +0100\n> >>\n> >>      libcamera: camera: Associate cameras with their pipeline handler\n> >>\n> >> the pipeline handler instances got stored in a shared_ptr, referenced\n> >> from both the pipes_ vector and the Camera class. The explicit delete\n> >> was removed, and pipes_ was simply cleared instead. We could have\n> >> removed the pipes_ vector at that point, as pipeline handlers were\n> >> referenced from the Camera class.\n> >>\n> >> It would be nice to summarize this in the commit message to explain why\n> >> the pipes_ vector isn't needed anymore.\n> >>\n> >>> Since the vector is eliminated, there are no unchecked reference of the\n> >>> pipeline-handlers. This fixes the initial issue of dangling driver\n> >>> directories\n> >>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on\n> >>> 'unbind' → 'bind'\n> >>> operation while the CameraManager is running. The directories were\n> >>> still kept\n> >>> around even after 'unbind' because of the pipeline-handler's\n> >>> unchecked reference\n> >>> holding onto them.\n> >> \n> >> Have you tried to run the hotplug test under valgrind ? There are lots\n> >> of use-after-free issues with this patch applied:\n> >>\n> >> ==18826== Thread 2:\n> >> ==18826== Invalid read of size 8\n> >> ==18826==    at 0x4A5A06C:\n> >> std::__1::vector,\n> >> std::__1::allocator > >::size()\n> >> const (vector:656)\n> >> ==18826==    by 0x4A59778:\n> >> std::__1::vector,\n> >> std::__1::allocator > >::clear()\n> >> (vector:771)\n> >> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect()\n> >> (pipeline_handler.cpp:571)\n> >> ==18826==    by 0x4A586C6:\n> >> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)\n> >> (pipeline_handler.cpp:545)\n> >> ==18826==    by 0x4A5EC3D:\n> >> libcamera::BoundMethodMember >> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)\n> >> (bound_method.h:198)\n> >> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs >> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,\n> >> std::__1::integer_sequence) (bound_method.h:124)\n> >> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs >> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)\n> >> (bound_method.h:133)\n> >> ==18826==    by 0x49ABB26:\n> >> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr,\n> >> bool) (bound_method.cpp:85)\n> >> ==18826==    by 0x4A5EB85:\n> >> libcamera::BoundMethodMember >> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)\n> >> (bound_method.h:193)\n> >> ==18826==    by 0x4A0F600:\n> >> libcamera::Signal::emit(libcamera::MediaDevice*)\n> >> (signal.h:127)\n> >> ==18826==    by 0x4A0E589:\n> >> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string >> std::__1::char_traits, std::__1::allocator > const&)\n> >> (device_enumerator.cpp:290)\n> >> ==18826==    by 0x4B0C5C1:\n> >> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)\n> >> (device_enumerator_udev.cpp:344)\n> >> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184\n> >> free'd\n> >> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)\n> >> ==18826==    by 0x4AFD391:\n> >> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)\n> >> ==18826==    by 0x4A61E9E:\n> >> std::__1::default_delete::operator()(libcamera::PipelineHandler*)\n> >> const (memory:2363)\n> >> ==18826==    by 0x4A61C3F:\n> >> std::__1::__shared_ptr_pointer >> std::__1::default_delete,\n> >> std::__1::allocator >::__on_zero_shared()\n> >> (memory:3536)\n> >> ==18826==    by 0x49AC479:\n> >> std::__1::__shared_count::__release_shared() (memory:3440)\n> >> ==18826==    by 0x49AC41E:\n> >> std::__1::__shared_weak_count::__release_shared() (memory:3482)\n> >> ==18826==    by 0x49B35DB:\n> >> std::__1::shared_ptr::~shared_ptr()\n> >> (memory:4207)\n> >> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private()\n> >> (camera.cpp:300)\n> >> ==18826==    by 0x49C213A:\n> >> std::__1::default_delete::operator()(libcamera::Camera::Private*)\n> >> const (memory:2363)\n> >> ==18826==    by 0x49C20BE:\n> >> std::__1::unique_ptr >> std::__1::default_delete\n> >> >::reset(libcamera::Camera::Private*) (memory:2618)\n> >> ==18826==    by 0x49B3878:\n> >> std::__1::unique_ptr >> std::__1::default_delete >::~unique_ptr()\n> >> (memory:2572)\n> >> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)\n> >> ==18826==  Block was alloc'd at\n> >> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)\n> >> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in\n> >> /usr/lib64/libc++.so.1.0)\n> >> ==18826==    by 0x4AFD409:\n> >> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)\n> >> (uvcvideo.cpp:562)\n> >> ==18826==    by 0x4A58A4F:\n> >> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)\n> >> (pipeline_handler.cpp:640)\n> >> ==18826==    by 0x49C9A83:\n> >> libcamera::CameraManager::Private::createPipelineHandlers()\n> >> (camera_manager.cpp:148)\n> >> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init()\n> >> (camera_manager.cpp:127)\n> >> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run()\n> >> (camera_manager.cpp:104)\n> >> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread()\n> >> (thread.cpp:288)\n> >> ==18826==    by 0x4A76520:\n> >> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_\n> >> (type_traits:3480)\n> >> ==18826==    by 0x4A7642D: void\n> >> std::__1::__thread_execute >> std::__1::default_delete >, void\n> >> (libcamera::Thread::*)(), libcamera::Thread*,\n> >> 2ul>(std::__1::tuple >> std::__1::default_delete >, void\n> >> (libcamera::Thread::*)(), libcamera::Thread*>&,\n> >> std::__1::__tuple_indices<2ul>) (thread:273)\n> >> ==18826==    by 0x4A75D95: void*\n> >> std::__1::__thread_proxy >> std::__1::default_delete >, void\n> >> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)\n> >> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)\n> >>\n> >> The change below should fix this. I'll let you add a comment at the\n> >> beginning of PipelineHandler::disconnect() to explain what is going on\n> >> :-)\n> > \n> > I will go with something like the following:\n> > \n> > +       /* Since cameras_ holds a weak reference of the cameras,\n> > +        * we just need to empty the vector here instead of cameras_.clear() it.\n> > +        */\n> \n> I'm not sure that's quite describing what's happening.\n> \n> >> diff --git a/src/libcamera/pipeline_handler.cpp\n> >> b/src/libcamera/pipeline_handler.cpp\n> >> index bad79dcb6219..8358266d83ff 100644\n> >> --- a/src/libcamera/pipeline_handler.cpp\n> >> +++ b/src/libcamera/pipeline_handler.cpp\n> >> @@ -559,7 +559,9 @@ void\n> >> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)\n> >>    */\n> >>   void PipelineHandler::disconnect()\n> >>   {\n> >> -    for (std::weak_ptr ptr : cameras_) {\n> >> +    std::vector> cameras{ std::move(cameras_) };\n> \n> This moves all cameras out of the cameras_ vector - into a new\n> 'temporary' holding container.\n> \n> Moving them into the new container, ensures that the references are kept\n> constant, but they are no longer 'held' by the pipeline.\n> \n> I haven't checked, but I presume that this means below in the loop, the\n> code that was unable to remove something because a reference was held -\n> is now able to - because essentially the cameras_ object has been\n> cleared before entering the loop - instead of after.\n> \n> >> +\n> >> +    for (std::weak_ptr ptr : cameras) {\n> >>           std::shared_ptr camera = ptr.lock();\n> >>           if (!camera)\n> >>               continue;\n> >> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()\n> >>           camera->disconnect();\n> >>           manager_->removeCamera(camera);\n> >>       }\n> >> -\n> >> -    cameras_.clear();\n> \n> And because the cameras vector above was created with local scope - all\n> of it's contents get cleared at this point now automatically.\n\nThis change is needed because when the last reference to the last camera\nis dropped, the corresponding pipeline handler gets deleted. This can\nhappen in the manager_->removeCamera(camera) call. If we iterate over\ncameras_, and access cameras_ after the loop, we will access freed\nmemory. This function thus needs to make sure it won't access any member\nof the PipelineHandler class at a point where the pipeline handler may\nhave been deleted.\n\n> >>   }\n> >>     /**\n> > \n> > Quick question: You mentioned on IRC that this can be folded into a patch.\n> > \n> > Did you mean squash it into existing patch or treat this as a separate\n> > patch?\n> \n> That usually means squash it into the existing relevant (this current?)\n> patch.\n> \n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>>   src/libcamera/camera_manager.cpp | 3 ---\n> >>>   1 file changed, 3 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/camera_manager.cpp\n> >>> b/src/libcamera/camera_manager.cpp\n> >>> index 576856a..b8128df 100644\n> >>> --- a/src/libcamera/camera_manager.cpp\n> >>> +++ b/src/libcamera/camera_manager.cpp\n> >>> @@ -63,7 +63,6 @@ private:\n> >>>       bool initialized_;\n> >>>       int status_;\n> >>>   -    std::vector> pipes_;\n> >>>       std::unique_ptr enumerator_;\n> >>>         IPAManager ipaManager_;\n> >>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()\n> >>>               LOG(Camera, Debug)\n> >>>                   << \"Pipeline handler \\\"\" << factory->name()\n> >>>                   << \"\\\" matched\";\n> >>> -            pipes_.push_back(std::move(pipe));\n> >>>           }\n> >>>       }\n> >>>   @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()\n> >>>        * they all get destroyed before the device enumerator deletes the\n> >>>        * media devices.\n> >>>        */\n> >> \n> >> The above comment needs to be updated.\n> >>\n> >> -     * Release all references to cameras and pipeline handlers to ensure\n> >> -     * they all get destroyed before the device enumerator deletes the\n> >> -     * media devices.\n> >> +     * Release all references to cameras to ensure they all get destroyed\n> >> +     * before the device enumerator deletes the media devices.\n> >>\n> >>> -    pipes_.clear();\n> >>>       cameras_.clear();\n> >>>         enumerator_.reset(nullptr);","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 004F3603D8\n\tfor ;\n\tMon, 15 Jun 2020 17:05:50 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5F765F9;\n\tMon, 15 Jun 2020 17:05:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vIkxh1cb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592233550;\n\tbh=5KMn1Gkd1yZ09O900MIReLVnkaX7AG5O+ZINDVu+Wkw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vIkxh1cb4V8Rlm0ESa6myw4iumyUBAVCS36mo9Os9Psu2V/b0vEHql92vzb0T/Syg\n\t8+vRUbA56dL+xHHReNYm0t3qByWfRtpMl+rtXoO8d10YDvFTvpKI0wPuGsQoS7hUnm\n\tPQ3IRFimq16TTxSScU2pdyn/FHcZ8Nr0Te9I+AWU=","Date":"Mon, 15 Jun 2020 18:05:28 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Umang Jain , libcamera-devel@lists.libcamera.org","Message-ID":"<20200615150528.GA1629@pendragon.ideasonboard.com>","References":"<20200611171528.9381-1-email@uajain.com>\n\t<20200611171528.9381-2-email@uajain.com>\n\t<20200612100131.GG5957@pendragon.ideasonboard.com>\n\t\n\t<74c6771c-7d59-9f1a-ce66-06a429d022ca@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<74c6771c-7d59-9f1a-ce66-06a429d022ca@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 15 Jun 2020 15:05:51 -0000"}},{"id":5210,"web_url":"https://patchwork.libcamera.org/comment/5210/","msgid":"<20200615152122.GC1629@pendragon.ideasonboard.com>","date":"2020-06-15T15:21:22","subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Mon, Jun 15, 2020 at 03:02:23PM +0000, Umang Jain wrote:\n> On 6/15/20 7:31 PM, Kieran Bingham wrote:\n> > On 15/06/2020 14:40, Umang Jain wrote:\n> >> On 6/12/20 3:31 PM, Laurent Pinchart wrote:\n> >>> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:\n> >>>> This placeholder for the pipeline-handlers created by CameraManager,\n> >>>> was responsible to hold a reference to pipeline-handlers which were\n> >>>> not getting dropped anywhere in the code-path. Instead of introducing\n> >>>> a fix of decrementing the reference, it is decided to eliminate this\n> >>>> vector entirely from  the CameraManager. This is due to the fact that\n> >>>> the vector does not seem to have much use in CameraManager and thus\n> >>>> reducing futile book-keeping.\n> >>> \n> >>> I think the change is good, but \"does not seem to have much use\" sounds\n> >>> like you're not really sure :-)\n> >>>\n> >>> The pipes_ vector has been there pretty much since day 1, and served the\n> >>> purpose of storing pipeline handler instances when they were not yet\n> >>> referenced by the Camera class. This was used to retrieve cameras, and\n> >>> to delete pipeline handler instances when stopping the camera manager.\n> >>> In\n> >>>\n> >>> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49\n> >>> Author: Laurent Pinchart \n> >>> Date:   Thu Jan 3 13:10:37 2019 +0200\n> >>>\n> >>>      libcamera: camera_manager: Register cameras with the camera manager\n> >>>\n> >>> cameras were registered directly with the camera manager, and the pipes_\n> >>> vector was only used to delete pipeline handler instances. In\n> >>>\n> >>> commit 5b02e03199b79165086135236d8fb9b2c673aae1\n> >>> Author: Niklas Söderlund \n> >>> Date:   Tue Jan 22 16:31:39 2019 +0100\n> >>>\n> >>>      libcamera: camera: Associate cameras with their pipeline handler\n> >>>\n> >>> the pipeline handler instances got stored in a shared_ptr, referenced\n> >>> from both the pipes_ vector and the Camera class. The explicit delete\n> >>> was removed, and pipes_ was simply cleared instead. We could have\n> >>> removed the pipes_ vector at that point, as pipeline handlers were\n> >>> referenced from the Camera class.\n> >>>\n> >>> It would be nice to summarize this in the commit message to explain why\n> >>> the pipes_ vector isn't needed anymore.\n> >>>\n> >>>> Since the vector is eliminated, there are no unchecked reference of the\n> >>>> pipeline-handlers. This fixes the initial issue of dangling driver\n> >>>> directories\n> >>>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on\n> >>>> 'unbind' → 'bind'\n> >>>> operation while the CameraManager is running. The directories were\n> >>>> still kept\n> >>>> around even after 'unbind' because of the pipeline-handler's\n> >>>> unchecked reference\n> >>>> holding onto them.\n> >>> \n> >>> Have you tried to run the hotplug test under valgrind ? There are lots\n> >>> of use-after-free issues with this patch applied:\n> >>>\n> >>> ==18826== Thread 2:\n> >>> ==18826== Invalid read of size 8\n> >>> ==18826==    at 0x4A5A06C:\n> >>> std::__1::vector,\n> >>> std::__1::allocator > >::size()\n> >>> const (vector:656)\n> >>> ==18826==    by 0x4A59778:\n> >>> std::__1::vector,\n> >>> std::__1::allocator > >::clear()\n> >>> (vector:771)\n> >>> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect()\n> >>> (pipeline_handler.cpp:571)\n> >>> ==18826==    by 0x4A586C6:\n> >>> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)\n> >>> (pipeline_handler.cpp:545)\n> >>> ==18826==    by 0x4A5EC3D:\n> >>> libcamera::BoundMethodMember >>> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)\n> >>> (bound_method.h:198)\n> >>> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs >>> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,\n> >>> std::__1::integer_sequence) (bound_method.h:124)\n> >>> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs >>> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)\n> >>> (bound_method.h:133)\n> >>> ==18826==    by 0x49ABB26:\n> >>> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr,\n> >>> bool) (bound_method.cpp:85)\n> >>> ==18826==    by 0x4A5EB85:\n> >>> libcamera::BoundMethodMember >>> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)\n> >>> (bound_method.h:193)\n> >>> ==18826==    by 0x4A0F600:\n> >>> libcamera::Signal::emit(libcamera::MediaDevice*)\n> >>> (signal.h:127)\n> >>> ==18826==    by 0x4A0E589:\n> >>> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string >>> std::__1::char_traits, std::__1::allocator > const&)\n> >>> (device_enumerator.cpp:290)\n> >>> ==18826==    by 0x4B0C5C1:\n> >>> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)\n> >>> (device_enumerator_udev.cpp:344)\n> >>> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184\n> >>> free'd\n> >>> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)\n> >>> ==18826==    by 0x4AFD391:\n> >>> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)\n> >>> ==18826==    by 0x4A61E9E:\n> >>> std::__1::default_delete::operator()(libcamera::PipelineHandler*)\n> >>> const (memory:2363)\n> >>> ==18826==    by 0x4A61C3F:\n> >>> std::__1::__shared_ptr_pointer >>> std::__1::default_delete,\n> >>> std::__1::allocator >::__on_zero_shared()\n> >>> (memory:3536)\n> >>> ==18826==    by 0x49AC479:\n> >>> std::__1::__shared_count::__release_shared() (memory:3440)\n> >>> ==18826==    by 0x49AC41E:\n> >>> std::__1::__shared_weak_count::__release_shared() (memory:3482)\n> >>> ==18826==    by 0x49B35DB:\n> >>> std::__1::shared_ptr::~shared_ptr()\n> >>> (memory:4207)\n> >>> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private()\n> >>> (camera.cpp:300)\n> >>> ==18826==    by 0x49C213A:\n> >>> std::__1::default_delete::operator()(libcamera::Camera::Private*)\n> >>> const (memory:2363)\n> >>> ==18826==    by 0x49C20BE:\n> >>> std::__1::unique_ptr >>> std::__1::default_delete\n> >>>> ::reset(libcamera::Camera::Private*) (memory:2618)\n> >>> \n> >>> ==18826==    by 0x49B3878:\n> >>> std::__1::unique_ptr >>> std::__1::default_delete >::~unique_ptr()\n> >>> (memory:2572)\n> >>> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)\n> >>> ==18826==  Block was alloc'd at\n> >>> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)\n> >>> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in\n> >>> /usr/lib64/libc++.so.1.0)\n> >>> ==18826==    by 0x4AFD409:\n> >>> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)\n> >>> (uvcvideo.cpp:562)\n> >>> ==18826==    by 0x4A58A4F:\n> >>> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)\n> >>> (pipeline_handler.cpp:640)\n> >>> ==18826==    by 0x49C9A83:\n> >>> libcamera::CameraManager::Private::createPipelineHandlers()\n> >>> (camera_manager.cpp:148)\n> >>> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init()\n> >>> (camera_manager.cpp:127)\n> >>> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run()\n> >>> (camera_manager.cpp:104)\n> >>> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread()\n> >>> (thread.cpp:288)\n> >>> ==18826==    by 0x4A76520:\n> >>> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_\n> >>> (type_traits:3480)\n> >>> ==18826==    by 0x4A7642D: void\n> >>> std::__1::__thread_execute >>> std::__1::default_delete >, void\n> >>> (libcamera::Thread::*)(), libcamera::Thread*,\n> >>> 2ul>(std::__1::tuple >>> std::__1::default_delete >, void\n> >>> (libcamera::Thread::*)(), libcamera::Thread*>&,\n> >>> std::__1::__tuple_indices<2ul>) (thread:273)\n> >>> ==18826==    by 0x4A75D95: void*\n> >>> std::__1::__thread_proxy >>> std::__1::default_delete >, void\n> >>> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)\n> >>> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)\n> >>>\n> >>> The change below should fix this. I'll let you add a comment at the\n> >>> beginning of PipelineHandler::disconnect() to explain what is going on\n> >>> :-)\n> >> \n> >> I will go with something like the following:\n> >>\n> >> +       /* Since cameras_ holds a weak reference of the cameras,\n> >> +        * we just need to empty the vector here instead of\n> >> cameras_.clear() it.\n> >> +        */\n> > \n> > I'm not sure that's quite describing what's happening.\n> >\n> >>> diff --git a/src/libcamera/pipeline_handler.cpp\n> >>> b/src/libcamera/pipeline_handler.cpp\n> >>> index bad79dcb6219..8358266d83ff 100644\n> >>> --- a/src/libcamera/pipeline_handler.cpp\n> >>> +++ b/src/libcamera/pipeline_handler.cpp\n> >>> @@ -559,7 +559,9 @@ void\n> >>> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)\n> >>>    */\n> >>>   void PipelineHandler::disconnect()\n> >>>   {\n> >>> -    for (std::weak_ptr ptr : cameras_) {\n> >>> +    std::vector> cameras{ std::move(cameras_) };\n> >\n> > This moves all cameras out of the cameras_ vector - into a new\n> > 'temporary' holding container.\n> >\n> > Moving them into the new container, ensures that the references are kept\n> > constant, but they are no longer 'held' by the pipeline.\n> \n> hmm, cameras_ is a vector of weak_ptr references, right? According to \n> the concepts I have in my head (:D), pipeline doesn't _really_ hold a\n> (strong)reference through this vector. Hence, isn't moving from\n> cameras_ to temporary container merely to avoid .clear()  call below?\n\nAs explained in a parallel reply, the reason why we need a local copy of\nthe vector here is that the call to manager_->removeCamera(camera) will\ndestroy the camera if nothing else holds a (strong) reference to it. If\nthe application doesn't hold a reference to any of the cameras for the\npipeline handler, all cameras created by the pipeline handler will be\ndestroyed. As cameras hold a reference to the pipeline handler, the\npipeline handler will thus get destroyed by the last\nmanager_->removeCamera(camera) call in the loop. The loop itself, and\nthe cameras_.clear() line, will then access freed memory. Moving\ncameras_ to a local cameras vector avoids this problem by ensuring that\ncode paths that can run after the pipeline handler gets deleted don't\naccess member variables. And yes, it's valid to delete the 'this' object\nfrom within a member function of the class (in this case indirectly,\nthrough manager_->removeCamera(camera)), provided we take care not to\naccess freed memory.\n\n> > I haven't checked, but I presume that this means below in the loop, the\n> > code that was unable to remove something because a reference was held -\n> > is now able to - because essentially the cameras_ object has been\n> > cleared before entering the loop - instead of after.\n> >\n> >>> +\n> >>> +    for (std::weak_ptr ptr : cameras) {\n> >>>           std::shared_ptr camera = ptr.lock();\n> >>>           if (!camera)\n> >>>               continue;\n> >>> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()\n> >>>           camera->disconnect();\n> >>>           manager_->removeCamera(camera);\n> >>>       }\n> >>> -\n> >>> -    cameras_.clear();\n> >\n> > And because the cameras vector above was created with local scope - all\n> > of it's contents get cleared at this point now automatically.\n> \n> Yes, but according to docs online, .clear() also tries to destroy the \n> objects in the vector and I am under the impression that, since they\n> are weak-references here, they are _not_ supposed to be destroyed(or\n> decrement the refcount) the objects. (that should happen automatically\n> when refcount drops to 0).\n\ncameras_.clear() destroys the objects contained in the vector. The\nobjects are of type std::weak_ptr. The weak pointers thus get\ndestroyed, and as they are weak pointers, they don't decrease the\nreference count on the object they point to, as they don't hold a\nreference in the first place.\n\n> >>>   }\n> >>>     /**\n> >> \n> >> Quick question: You mentioned on IRC that this can be folded into a patch.\n> >>\n> >> Did you mean squash it into existing patch or treat this as a separate\n> >> patch?\n> > \n> > That usually means squash it into the existing relevant (this current?)\n> > patch.\n> >\n> >>>> Signed-off-by: Umang Jain \n> >>>> ---\n> >>>>   src/libcamera/camera_manager.cpp | 3 ---\n> >>>>   1 file changed, 3 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/camera_manager.cpp\n> >>>> b/src/libcamera/camera_manager.cpp\n> >>>> index 576856a..b8128df 100644\n> >>>> --- a/src/libcamera/camera_manager.cpp\n> >>>> +++ b/src/libcamera/camera_manager.cpp\n> >>>> @@ -63,7 +63,6 @@ private:\n> >>>>       bool initialized_;\n> >>>>       int status_;\n> >>>>   -    std::vector> pipes_;\n> >>>>       std::unique_ptr enumerator_;\n> >>>>         IPAManager ipaManager_;\n> >>>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()\n> >>>>               LOG(Camera, Debug)\n> >>>>                   << \"Pipeline handler \\\"\" << factory->name()\n> >>>>                   << \"\\\" matched\";\n> >>>> -            pipes_.push_back(std::move(pipe));\n> >>>>           }\n> >>>>       }\n> >>>>   @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()\n> >>>>        * they all get destroyed before the device enumerator deletes the\n> >>>>        * media devices.\n> >>>>        */\n> >>> \n> >>> The above comment needs to be updated.\n> >>>\n> >>> -     * Release all references to cameras and pipeline handlers to ensure\n> >>> -     * they all get destroyed before the device enumerator deletes the\n> >>> -     * media devices.\n> >>> +     * Release all references to cameras to ensure they all get destroyed\n> >>> +     * before the device enumerator deletes the media devices.\n> >>>\n> >>>> -    pipes_.clear();\n> >>>>       cameras_.clear();\n> >>>>         enumerator_.reset(nullptr);","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 0B5E0603D8\n\tfor ;\n\tMon, 15 Jun 2020 17:21:45 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 92F5EF9;\n\tMon, 15 Jun 2020 17:21:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jKPWq38T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592234504;\n\tbh=psnehsijwqaV4GnduC5m8v5wdcN2wo7GEyX7+Aflv+Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jKPWq38TPunceeUXwQY0BxovF+B+JtfLAxCAzxvWYG/mZAtlBR7USIImvQxawbB17\n\tocHagWFXqdfcmTzWg6ttOEFNFrGtQfh1QkJo0Y1mXAqIfPNYeUXyKg6dUP0Td2uyvx\n\tfFsnTmdMPL+seQB9/xyZvrXsS6qS60fpnodw1Jdg=","Date":"Mon, 15 Jun 2020 18:21:22 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Cc":"kieran.bingham@ideasonboard.com, libcamera-devel@lists.libcamera.org","Message-ID":"<20200615152122.GC1629@pendragon.ideasonboard.com>","References":"<20200611171528.9381-1-email@uajain.com>\n\t<20200611171528.9381-2-email@uajain.com>\n\t<20200612100131.GG5957@pendragon.ideasonboard.com>\n\t\n\t<74c6771c-7d59-9f1a-ce66-06a429d022ca@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop\n\tthe vector of created PipelineHandlers","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 15 Jun 2020 15:21:45 -0000"}}]