[libcamera-devel,v4,1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers

Message ID 20200611171528.9381-2-email@uajain.com
State Superseded
Headers show
Series
  • Introduce UVC hotplugging support
Related show

Commit Message

Umang Jain June 11, 2020, 5:16 p.m. UTC
This placeholder for the pipeline-handlers created by CameraManager,
was responsible to hold a reference to pipeline-handlers which were
not getting dropped anywhere in the code-path. Instead of introducing
a fix of decrementing the reference, it is decided to eliminate this
vector entirely from  the CameraManager. This is due to the fact that
the vector does not seem to have much use in CameraManager and thus
reducing futile book-keeping.

Since the vector is eliminated, there are no unchecked reference of the
pipeline-handlers. This fixes the initial issue of dangling driver directories
(for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'
operation while the CameraManager is running. The directories were still kept
around even after 'unbind' because of the pipeline-handler's unchecked reference
holding onto them.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/libcamera/camera_manager.cpp | 3 ---
 1 file changed, 3 deletions(-)

Comments

Laurent Pinchart June 12, 2020, 10:01 a.m. UTC | #1
Hi Umang,

Thank you for the patch.

On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:
> This placeholder for the pipeline-handlers created by CameraManager,
> was responsible to hold a reference to pipeline-handlers which were
> not getting dropped anywhere in the code-path. Instead of introducing
> a fix of decrementing the reference, it is decided to eliminate this
> vector entirely from  the CameraManager. This is due to the fact that
> the vector does not seem to have much use in CameraManager and thus
> reducing futile book-keeping.

I think the change is good, but "does not seem to have much use" sounds
like you're not really sure :-)

The pipes_ vector has been there pretty much since day 1, and served the
purpose of storing pipeline handler instances when they were not yet
referenced by the Camera class. This was used to retrieve cameras, and
to delete pipeline handler instances when stopping the camera manager.
In

commit f3695e9b09ce4a88d6e0480d9e5058673af34a49
Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Date:   Thu Jan 3 13:10:37 2019 +0200

    libcamera: camera_manager: Register cameras with the camera manager

cameras were registered directly with the camera manager, and the pipes_
vector was only used to delete pipeline handler instances. In

commit 5b02e03199b79165086135236d8fb9b2c673aae1
Author: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Date:   Tue Jan 22 16:31:39 2019 +0100

    libcamera: camera: Associate cameras with their pipeline handler

the pipeline handler instances got stored in a shared_ptr, referenced
from both the pipes_ vector and the Camera class. The explicit delete
was removed, and pipes_ was simply cleared instead. We could have
removed the pipes_ vector at that point, as pipeline handlers were
referenced from the Camera class.

It would be nice to summarize this in the commit message to explain why
the pipes_ vector isn't needed anymore.

> Since the vector is eliminated, there are no unchecked reference of the
> pipeline-handlers. This fixes the initial issue of dangling driver directories
> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'
> operation while the CameraManager is running. The directories were still kept
> around even after 'unbind' because of the pipeline-handler's unchecked reference
> holding onto them.

Have you tried to run the hotplug test under valgrind ? There are lots
of use-after-free issues with this patch applied:

==18826== Thread 2:
==18826== Invalid read of size 8
==18826==    at 0x4A5A06C: std::__1::vector<std::__1::weak_ptr<libcamera::Camera>, std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::size() const (vector:656)
==18826==    by 0x4A59778: std::__1::vector<std::__1::weak_ptr<libcamera::Camera>, std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::clear() (vector:771)
==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect() (pipeline_handler.cpp:571)
==18826==    by 0x4A586C6: libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*) (pipeline_handler.cpp:545)
==18826==    by 0x4A5EC3D: libcamera::BoundMethodMember<libcamera::PipelineHandler, void, libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*) (bound_method.h:198)
==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs<void, libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*, std::__1::integer_sequence<unsigned long, 0ul>) (bound_method.h:124)
==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs<void, libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*) (bound_method.h:133)
==18826==    by 0x49ABB26: libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr<libcamera::BoundMethodPackBase>, bool) (bound_method.cpp:85)
==18826==    by 0x4A5EB85: libcamera::BoundMethodMember<libcamera::PipelineHandler, void, libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool) (bound_method.h:193)
==18826==    by 0x4A0F600: libcamera::Signal<libcamera::MediaDevice*>::emit(libcamera::MediaDevice*) (signal.h:127)
==18826==    by 0x4A0E589: libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) (device_enumerator.cpp:290)
==18826==    by 0x4B0C5C1: libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*) (device_enumerator_udev.cpp:344)
==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184 free'd
==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)
==18826==    by 0x4AFD391: libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)
==18826==    by 0x4A61E9E: std::__1::default_delete<libcamera::PipelineHandler>::operator()(libcamera::PipelineHandler*) const (memory:2363)
==18826==    by 0x4A61C3F: std::__1::__shared_ptr_pointer<libcamera::PipelineHandler*, std::__1::default_delete<libcamera::PipelineHandler>, std::__1::allocator<libcamera::PipelineHandler> >::__on_zero_shared() (memory:3536)
==18826==    by 0x49AC479: std::__1::__shared_count::__release_shared() (memory:3440)
==18826==    by 0x49AC41E: std::__1::__shared_weak_count::__release_shared() (memory:3482)
==18826==    by 0x49B35DB: std::__1::shared_ptr<libcamera::PipelineHandler>::~shared_ptr() (memory:4207)
==18826==    by 0x49B0982: libcamera::Camera::Private::~Private() (camera.cpp:300)
==18826==    by 0x49C213A: std::__1::default_delete<libcamera::Camera::Private>::operator()(libcamera::Camera::Private*) const (memory:2363)
==18826==    by 0x49C20BE: std::__1::unique_ptr<libcamera::Camera::Private, std::__1::default_delete<libcamera::Camera::Private> >::reset(libcamera::Camera::Private*) (memory:2618)
==18826==    by 0x49B3878: std::__1::unique_ptr<libcamera::Camera::Private, std::__1::default_delete<libcamera::Camera::Private> >::~unique_ptr() (memory:2572)
==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)
==18826==  Block was alloc'd at
==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)
==18826==    by 0x4D0F76B: operator new(unsigned long) (in /usr/lib64/libc++.so.1.0)
==18826==    by 0x4AFD409: libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*) (uvcvideo.cpp:562)
==18826==    by 0x4A58A4F: libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*) (pipeline_handler.cpp:640)
==18826==    by 0x49C9A83: libcamera::CameraManager::Private::createPipelineHandlers() (camera_manager.cpp:148)
==18826==    by 0x49C9952: libcamera::CameraManager::Private::init() (camera_manager.cpp:127)
==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run() (camera_manager.cpp:104)
==18826==    by 0x4A71CDC: libcamera::Thread::startThread() (thread.cpp:288)
==18826==    by 0x4A76520: _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_ (type_traits:3480)
==18826==    by 0x4A7642D: void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*, 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*>&, std::__1::__tuple_indices<2ul>) (thread:273)
==18826==    by 0x4A75D95: void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)
==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)

The change below should fix this. I'll let you add a comment at the
beginning of PipelineHandler::disconnect() to explain what is going on
:-)

diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index bad79dcb6219..8358266d83ff 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -559,7 +559,9 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
  */
 void PipelineHandler::disconnect()
 {
-	for (std::weak_ptr<Camera> ptr : cameras_) {
+	std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
+
+	for (std::weak_ptr<Camera> ptr : cameras) {
 		std::shared_ptr<Camera> camera = ptr.lock();
 		if (!camera)
 			continue;
@@ -567,8 +569,6 @@ void PipelineHandler::disconnect()
 		camera->disconnect();
 		manager_->removeCamera(camera);
 	}
-
-	cameras_.clear();
 }
 
 /**

> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/libcamera/camera_manager.cpp | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 576856a..b8128df 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -63,7 +63,6 @@ private:
>  	bool initialized_;
>  	int status_;
>  
> -	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  
>  	IPAManager ipaManager_;
> @@ -144,7 +143,6 @@ int CameraManager::Private::init()
>  			LOG(Camera, Debug)
>  				<< "Pipeline handler \"" << factory->name()
>  				<< "\" matched";
> -			pipes_.push_back(std::move(pipe));
>  		}
>  	}
>  
> @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()
>  	 * they all get destroyed before the device enumerator deletes the
>  	 * media devices.
>  	 */

The above comment needs to be updated.

-	 * Release all references to cameras and pipeline handlers to ensure
-	 * they all get destroyed before the device enumerator deletes the
-	 * media devices.
+	 * Release all references to cameras to ensure they all get destroyed
+	 * before the device enumerator deletes the media devices.

> -	pipes_.clear();
>  	cameras_.clear();
>  
>  	enumerator_.reset(nullptr);
Umang Jain June 15, 2020, 1:40 p.m. UTC | #2
Hi Laurent,

On 6/12/20 3:31 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:
>> This placeholder for the pipeline-handlers created by CameraManager,
>> was responsible to hold a reference to pipeline-handlers which were
>> not getting dropped anywhere in the code-path. Instead of introducing
>> a fix of decrementing the reference, it is decided to eliminate this
>> vector entirely from  the CameraManager. This is due to the fact that
>> the vector does not seem to have much use in CameraManager and thus
>> reducing futile book-keeping.
> I think the change is good, but "does not seem to have much use" sounds
> like you're not really sure :-)
>
> The pipes_ vector has been there pretty much since day 1, and served the
> purpose of storing pipeline handler instances when they were not yet
> referenced by the Camera class. This was used to retrieve cameras, and
> to delete pipeline handler instances when stopping the camera manager.
> In
>
> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49
> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Date:   Thu Jan 3 13:10:37 2019 +0200
>
>      libcamera: camera_manager: Register cameras with the camera manager
>
> cameras were registered directly with the camera manager, and the pipes_
> vector was only used to delete pipeline handler instances. In
>
> commit 5b02e03199b79165086135236d8fb9b2c673aae1
> Author: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Date:   Tue Jan 22 16:31:39 2019 +0100
>
>      libcamera: camera: Associate cameras with their pipeline handler
>
> the pipeline handler instances got stored in a shared_ptr, referenced
> from both the pipes_ vector and the Camera class. The explicit delete
> was removed, and pipes_ was simply cleared instead. We could have
> removed the pipes_ vector at that point, as pipeline handlers were
> referenced from the Camera class.
>
> It would be nice to summarize this in the commit message to explain why
> the pipes_ vector isn't needed anymore.
>
>> Since the vector is eliminated, there are no unchecked reference of the
>> pipeline-handlers. This fixes the initial issue of dangling driver directories
>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'
>> operation while the CameraManager is running. The directories were still kept
>> around even after 'unbind' because of the pipeline-handler's unchecked reference
>> holding onto them.
> Have you tried to run the hotplug test under valgrind ? There are lots
> of use-after-free issues with this patch applied:
>
> ==18826== Thread 2:
> ==18826== Invalid read of size 8
> ==18826==    at 0x4A5A06C: std::__1::vector<std::__1::weak_ptr<libcamera::Camera>, std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::size() const (vector:656)
> ==18826==    by 0x4A59778: std::__1::vector<std::__1::weak_ptr<libcamera::Camera>, std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::clear() (vector:771)
> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect() (pipeline_handler.cpp:571)
> ==18826==    by 0x4A586C6: libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*) (pipeline_handler.cpp:545)
> ==18826==    by 0x4A5EC3D: libcamera::BoundMethodMember<libcamera::PipelineHandler, void, libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*) (bound_method.h:198)
> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs<void, libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*, std::__1::integer_sequence<unsigned long, 0ul>) (bound_method.h:124)
> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs<void, libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*) (bound_method.h:133)
> ==18826==    by 0x49ABB26: libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr<libcamera::BoundMethodPackBase>, bool) (bound_method.cpp:85)
> ==18826==    by 0x4A5EB85: libcamera::BoundMethodMember<libcamera::PipelineHandler, void, libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool) (bound_method.h:193)
> ==18826==    by 0x4A0F600: libcamera::Signal<libcamera::MediaDevice*>::emit(libcamera::MediaDevice*) (signal.h:127)
> ==18826==    by 0x4A0E589: libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) (device_enumerator.cpp:290)
> ==18826==    by 0x4B0C5C1: libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*) (device_enumerator_udev.cpp:344)
> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184 free'd
> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)
> ==18826==    by 0x4AFD391: libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)
> ==18826==    by 0x4A61E9E: std::__1::default_delete<libcamera::PipelineHandler>::operator()(libcamera::PipelineHandler*) const (memory:2363)
> ==18826==    by 0x4A61C3F: std::__1::__shared_ptr_pointer<libcamera::PipelineHandler*, std::__1::default_delete<libcamera::PipelineHandler>, std::__1::allocator<libcamera::PipelineHandler> >::__on_zero_shared() (memory:3536)
> ==18826==    by 0x49AC479: std::__1::__shared_count::__release_shared() (memory:3440)
> ==18826==    by 0x49AC41E: std::__1::__shared_weak_count::__release_shared() (memory:3482)
> ==18826==    by 0x49B35DB: std::__1::shared_ptr<libcamera::PipelineHandler>::~shared_ptr() (memory:4207)
> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private() (camera.cpp:300)
> ==18826==    by 0x49C213A: std::__1::default_delete<libcamera::Camera::Private>::operator()(libcamera::Camera::Private*) const (memory:2363)
> ==18826==    by 0x49C20BE: std::__1::unique_ptr<libcamera::Camera::Private, std::__1::default_delete<libcamera::Camera::Private> >::reset(libcamera::Camera::Private*) (memory:2618)
> ==18826==    by 0x49B3878: std::__1::unique_ptr<libcamera::Camera::Private, std::__1::default_delete<libcamera::Camera::Private> >::~unique_ptr() (memory:2572)
> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)
> ==18826==  Block was alloc'd at
> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)
> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in /usr/lib64/libc++.so.1.0)
> ==18826==    by 0x4AFD409: libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*) (uvcvideo.cpp:562)
> ==18826==    by 0x4A58A4F: libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*) (pipeline_handler.cpp:640)
> ==18826==    by 0x49C9A83: libcamera::CameraManager::Private::createPipelineHandlers() (camera_manager.cpp:148)
> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init() (camera_manager.cpp:127)
> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run() (camera_manager.cpp:104)
> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread() (thread.cpp:288)
> ==18826==    by 0x4A76520: _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_ (type_traits:3480)
> ==18826==    by 0x4A7642D: void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*, 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*>&, std::__1::__tuple_indices<2ul>) (thread:273)
> ==18826==    by 0x4A75D95: void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)
> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)
>
> The change below should fix this. I'll let you add a comment at the
> beginning of PipelineHandler::disconnect() to explain what is going on
> :-)

I will go with something like the following:

+       /* Since cameras_ holds a weak reference of the cameras,
+        * we just need to empty the vector here instead of 
cameras_.clear() it.
+        */

> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index bad79dcb6219..8358266d83ff 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -559,7 +559,9 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
>    */
>   void PipelineHandler::disconnect()
>   {
> -	for (std::weak_ptr<Camera> ptr : cameras_) {
> +	std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
> +
> +	for (std::weak_ptr<Camera> ptr : cameras) {
>   		std::shared_ptr<Camera> camera = ptr.lock();
>   		if (!camera)
>   			continue;
> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()
>   		camera->disconnect();
>   		manager_->removeCamera(camera);
>   	}
> -
> -	cameras_.clear();
>   }
>   
>   /**

Quick question: You mentioned on IRC that this can be folded into a patch.

Did you mean squash it into existing patch or treat this as a separate 
patch?

>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> ---
>>   src/libcamera/camera_manager.cpp | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>> index 576856a..b8128df 100644
>> --- a/src/libcamera/camera_manager.cpp
>> +++ b/src/libcamera/camera_manager.cpp
>> @@ -63,7 +63,6 @@ private:
>>   	bool initialized_;
>>   	int status_;
>>   
>> -	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>>   	std::unique_ptr<DeviceEnumerator> enumerator_;
>>   
>>   	IPAManager ipaManager_;
>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()
>>   			LOG(Camera, Debug)
>>   				<< "Pipeline handler \"" << factory->name()
>>   				<< "\" matched";
>> -			pipes_.push_back(std::move(pipe));
>>   		}
>>   	}
>>   
>> @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()
>>   	 * they all get destroyed before the device enumerator deletes the
>>   	 * media devices.
>>   	 */
> The above comment needs to be updated.
>
> -	 * Release all references to cameras and pipeline handlers to ensure
> -	 * they all get destroyed before the device enumerator deletes the
> -	 * media devices.
> +	 * Release all references to cameras to ensure they all get destroyed
> +	 * before the device enumerator deletes the media devices.
>
>> -	pipes_.clear();
>>   	cameras_.clear();
>>   
>>   	enumerator_.reset(nullptr);
Kieran Bingham June 15, 2020, 2:01 p.m. UTC | #3
Hi Umang,

On 15/06/2020 14:40, Umang Jain wrote:
> Hi Laurent,
> 
> On 6/12/20 3:31 PM, Laurent Pinchart wrote:
>> Hi Umang,
>>
>> Thank you for the patch.
>>
>> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:
>>> This placeholder for the pipeline-handlers created by CameraManager,
>>> was responsible to hold a reference to pipeline-handlers which were
>>> not getting dropped anywhere in the code-path. Instead of introducing
>>> a fix of decrementing the reference, it is decided to eliminate this
>>> vector entirely from  the CameraManager. This is due to the fact that
>>> the vector does not seem to have much use in CameraManager and thus
>>> reducing futile book-keeping.
>> I think the change is good, but "does not seem to have much use" sounds
>> like you're not really sure :-)
>>
>> The pipes_ vector has been there pretty much since day 1, and served the
>> purpose of storing pipeline handler instances when they were not yet
>> referenced by the Camera class. This was used to retrieve cameras, and
>> to delete pipeline handler instances when stopping the camera manager.
>> In
>>
>> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49
>> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Date:   Thu Jan 3 13:10:37 2019 +0200
>>
>>      libcamera: camera_manager: Register cameras with the camera manager
>>
>> cameras were registered directly with the camera manager, and the pipes_
>> vector was only used to delete pipeline handler instances. In
>>
>> commit 5b02e03199b79165086135236d8fb9b2c673aae1
>> Author: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>> Date:   Tue Jan 22 16:31:39 2019 +0100
>>
>>      libcamera: camera: Associate cameras with their pipeline handler
>>
>> the pipeline handler instances got stored in a shared_ptr, referenced
>> from both the pipes_ vector and the Camera class. The explicit delete
>> was removed, and pipes_ was simply cleared instead. We could have
>> removed the pipes_ vector at that point, as pipeline handlers were
>> referenced from the Camera class.
>>
>> It would be nice to summarize this in the commit message to explain why
>> the pipes_ vector isn't needed anymore.
>>
>>> Since the vector is eliminated, there are no unchecked reference of the
>>> pipeline-handlers. This fixes the initial issue of dangling driver
>>> directories
>>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on
>>> 'unbind' → 'bind'
>>> operation while the CameraManager is running. The directories were
>>> still kept
>>> around even after 'unbind' because of the pipeline-handler's
>>> unchecked reference
>>> holding onto them.
>> Have you tried to run the hotplug test under valgrind ? There are lots
>> of use-after-free issues with this patch applied:
>>
>> ==18826== Thread 2:
>> ==18826== Invalid read of size 8
>> ==18826==    at 0x4A5A06C:
>> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
>> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::size()
>> const (vector:656)
>> ==18826==    by 0x4A59778:
>> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
>> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::clear()
>> (vector:771)
>> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect()
>> (pipeline_handler.cpp:571)
>> ==18826==    by 0x4A586C6:
>> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)
>> (pipeline_handler.cpp:545)
>> ==18826==    by 0x4A5EC3D:
>> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
>> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)
>> (bound_method.h:198)
>> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs<void,
>> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,
>> std::__1::integer_sequence<unsigned long, 0ul>) (bound_method.h:124)
>> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs<void,
>> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)
>> (bound_method.h:133)
>> ==18826==    by 0x49ABB26:
>> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr<libcamera::BoundMethodPackBase>,
>> bool) (bound_method.cpp:85)
>> ==18826==    by 0x4A5EB85:
>> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
>> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)
>> (bound_method.h:193)
>> ==18826==    by 0x4A0F600:
>> libcamera::Signal<libcamera::MediaDevice*>::emit(libcamera::MediaDevice*)
>> (signal.h:127)
>> ==18826==    by 0x4A0E589:
>> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string<char,
>> std::__1::char_traits<char>, std::__1::allocator<char> > const&)
>> (device_enumerator.cpp:290)
>> ==18826==    by 0x4B0C5C1:
>> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)
>> (device_enumerator_udev.cpp:344)
>> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184
>> free'd
>> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)
>> ==18826==    by 0x4AFD391:
>> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)
>> ==18826==    by 0x4A61E9E:
>> std::__1::default_delete<libcamera::PipelineHandler>::operator()(libcamera::PipelineHandler*)
>> const (memory:2363)
>> ==18826==    by 0x4A61C3F:
>> std::__1::__shared_ptr_pointer<libcamera::PipelineHandler*,
>> std::__1::default_delete<libcamera::PipelineHandler>,
>> std::__1::allocator<libcamera::PipelineHandler> >::__on_zero_shared()
>> (memory:3536)
>> ==18826==    by 0x49AC479:
>> std::__1::__shared_count::__release_shared() (memory:3440)
>> ==18826==    by 0x49AC41E:
>> std::__1::__shared_weak_count::__release_shared() (memory:3482)
>> ==18826==    by 0x49B35DB:
>> std::__1::shared_ptr<libcamera::PipelineHandler>::~shared_ptr()
>> (memory:4207)
>> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private()
>> (camera.cpp:300)
>> ==18826==    by 0x49C213A:
>> std::__1::default_delete<libcamera::Camera::Private>::operator()(libcamera::Camera::Private*)
>> const (memory:2363)
>> ==18826==    by 0x49C20BE:
>> std::__1::unique_ptr<libcamera::Camera::Private,
>> std::__1::default_delete<libcamera::Camera::Private>
>> >::reset(libcamera::Camera::Private*) (memory:2618)
>> ==18826==    by 0x49B3878:
>> std::__1::unique_ptr<libcamera::Camera::Private,
>> std::__1::default_delete<libcamera::Camera::Private> >::~unique_ptr()
>> (memory:2572)
>> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)
>> ==18826==  Block was alloc'd at
>> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)
>> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in
>> /usr/lib64/libc++.so.1.0)
>> ==18826==    by 0x4AFD409:
>> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)
>> (uvcvideo.cpp:562)
>> ==18826==    by 0x4A58A4F:
>> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)
>> (pipeline_handler.cpp:640)
>> ==18826==    by 0x49C9A83:
>> libcamera::CameraManager::Private::createPipelineHandlers()
>> (camera_manager.cpp:148)
>> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init()
>> (camera_manager.cpp:127)
>> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run()
>> (camera_manager.cpp:104)
>> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread()
>> (thread.cpp:288)
>> ==18826==    by 0x4A76520:
>> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_
>> (type_traits:3480)
>> ==18826==    by 0x4A7642D: void
>> std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct,
>> std::__1::default_delete<std::__1::__thread_struct> >, void
>> (libcamera::Thread::*)(), libcamera::Thread*,
>> 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
>> std::__1::default_delete<std::__1::__thread_struct> >, void
>> (libcamera::Thread::*)(), libcamera::Thread*>&,
>> std::__1::__tuple_indices<2ul>) (thread:273)
>> ==18826==    by 0x4A75D95: void*
>> std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
>> std::__1::default_delete<std::__1::__thread_struct> >, void
>> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)
>> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)
>>
>> The change below should fix this. I'll let you add a comment at the
>> beginning of PipelineHandler::disconnect() to explain what is going on
>> :-)
> 
> I will go with something like the following:
> 
> +       /* Since cameras_ holds a weak reference of the cameras,
> +        * we just need to empty the vector here instead of
> cameras_.clear() it.
> +        */

I'm not sure that's quite describing what's happening.





>> diff --git a/src/libcamera/pipeline_handler.cpp
>> b/src/libcamera/pipeline_handler.cpp
>> index bad79dcb6219..8358266d83ff 100644
>> --- a/src/libcamera/pipeline_handler.cpp
>> +++ b/src/libcamera/pipeline_handler.cpp
>> @@ -559,7 +559,9 @@ void
>> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
>>    */
>>   void PipelineHandler::disconnect()
>>   {
>> -    for (std::weak_ptr<Camera> ptr : cameras_) {
>> +    std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };

This moves all cameras out of the cameras_ vector - into a new
'temporary' holding container.

Moving them into the new container, ensures that the references are kept
constant, but they are no longer 'held' by the pipeline.

I haven't checked, but I presume that this means below in the loop, the
code that was unable to remove something because a reference was held -
is now able to - because essentially the cameras_ object has been
cleared before entering the loop - instead of after.

>> +
>> +    for (std::weak_ptr<Camera> ptr : cameras) {
>>           std::shared_ptr<Camera> camera = ptr.lock();
>>           if (!camera)
>>               continue;
>> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()
>>           camera->disconnect();
>>           manager_->removeCamera(camera);
>>       }
>> -
>> -    cameras_.clear();

And because the cameras vector above was created with local scope - all
of it's contents get cleared at this point now automatically.



>>   }
>>     /**
> 
> Quick question: You mentioned on IRC that this can be folded into a patch.
> 
> Did you mean squash it into existing patch or treat this as a separate
> patch?

That usually means squash it into the existing relevant (this current?)
patch.


>>
>>> Signed-off-by: Umang Jain <email@uajain.com>
>>> ---
>>>   src/libcamera/camera_manager.cpp | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/src/libcamera/camera_manager.cpp
>>> b/src/libcamera/camera_manager.cpp
>>> index 576856a..b8128df 100644
>>> --- a/src/libcamera/camera_manager.cpp
>>> +++ b/src/libcamera/camera_manager.cpp
>>> @@ -63,7 +63,6 @@ private:
>>>       bool initialized_;
>>>       int status_;
>>>   -    std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>>>       std::unique_ptr<DeviceEnumerator> enumerator_;
>>>         IPAManager ipaManager_;
>>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()
>>>               LOG(Camera, Debug)
>>>                   << "Pipeline handler \"" << factory->name()
>>>                   << "\" matched";
>>> -            pipes_.push_back(std::move(pipe));
>>>           }
>>>       }
>>>   @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()
>>>        * they all get destroyed before the device enumerator deletes the
>>>        * media devices.
>>>        */
>> The above comment needs to be updated.
>>
>> -     * Release all references to cameras and pipeline handlers to ensure
>> -     * they all get destroyed before the device enumerator deletes the
>> -     * media devices.
>> +     * Release all references to cameras to ensure they all get
>> destroyed
>> +     * before the device enumerator deletes the media devices.
>>
>>> -    pipes_.clear();
>>>       cameras_.clear();
>>>         enumerator_.reset(nullptr);
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Umang Jain June 15, 2020, 3:02 p.m. UTC | #4
Hi Kieran,

On 6/15/20 7:31 PM, Kieran Bingham wrote:
> Hi Umang,
>
> On 15/06/2020 14:40, Umang Jain wrote:
>> Hi Laurent,
>>
>> On 6/12/20 3:31 PM, Laurent Pinchart wrote:
>>> Hi Umang,
>>>
>>> Thank you for the patch.
>>>
>>> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:
>>>> This placeholder for the pipeline-handlers created by CameraManager,
>>>> was responsible to hold a reference to pipeline-handlers which were
>>>> not getting dropped anywhere in the code-path. Instead of introducing
>>>> a fix of decrementing the reference, it is decided to eliminate this
>>>> vector entirely from  the CameraManager. This is due to the fact that
>>>> the vector does not seem to have much use in CameraManager and thus
>>>> reducing futile book-keeping.
>>> I think the change is good, but "does not seem to have much use" sounds
>>> like you're not really sure :-)
>>>
>>> The pipes_ vector has been there pretty much since day 1, and served the
>>> purpose of storing pipeline handler instances when they were not yet
>>> referenced by the Camera class. This was used to retrieve cameras, and
>>> to delete pipeline handler instances when stopping the camera manager.
>>> In
>>>
>>> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49
>>> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Date:   Thu Jan 3 13:10:37 2019 +0200
>>>
>>>       libcamera: camera_manager: Register cameras with the camera manager
>>>
>>> cameras were registered directly with the camera manager, and the pipes_
>>> vector was only used to delete pipeline handler instances. In
>>>
>>> commit 5b02e03199b79165086135236d8fb9b2c673aae1
>>> Author: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>>> Date:   Tue Jan 22 16:31:39 2019 +0100
>>>
>>>       libcamera: camera: Associate cameras with their pipeline handler
>>>
>>> the pipeline handler instances got stored in a shared_ptr, referenced
>>> from both the pipes_ vector and the Camera class. The explicit delete
>>> was removed, and pipes_ was simply cleared instead. We could have
>>> removed the pipes_ vector at that point, as pipeline handlers were
>>> referenced from the Camera class.
>>>
>>> It would be nice to summarize this in the commit message to explain why
>>> the pipes_ vector isn't needed anymore.
>>>
>>>> Since the vector is eliminated, there are no unchecked reference of the
>>>> pipeline-handlers. This fixes the initial issue of dangling driver
>>>> directories
>>>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on
>>>> 'unbind' → 'bind'
>>>> operation while the CameraManager is running. The directories were
>>>> still kept
>>>> around even after 'unbind' because of the pipeline-handler's
>>>> unchecked reference
>>>> holding onto them.
>>> Have you tried to run the hotplug test under valgrind ? There are lots
>>> of use-after-free issues with this patch applied:
>>>
>>> ==18826== Thread 2:
>>> ==18826== Invalid read of size 8
>>> ==18826==    at 0x4A5A06C:
>>> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
>>> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::size()
>>> const (vector:656)
>>> ==18826==    by 0x4A59778:
>>> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
>>> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::clear()
>>> (vector:771)
>>> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect()
>>> (pipeline_handler.cpp:571)
>>> ==18826==    by 0x4A586C6:
>>> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)
>>> (pipeline_handler.cpp:545)
>>> ==18826==    by 0x4A5EC3D:
>>> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
>>> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)
>>> (bound_method.h:198)
>>> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs<void,
>>> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,
>>> std::__1::integer_sequence<unsigned long, 0ul>) (bound_method.h:124)
>>> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs<void,
>>> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)
>>> (bound_method.h:133)
>>> ==18826==    by 0x49ABB26:
>>> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr<libcamera::BoundMethodPackBase>,
>>> bool) (bound_method.cpp:85)
>>> ==18826==    by 0x4A5EB85:
>>> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
>>> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)
>>> (bound_method.h:193)
>>> ==18826==    by 0x4A0F600:
>>> libcamera::Signal<libcamera::MediaDevice*>::emit(libcamera::MediaDevice*)
>>> (signal.h:127)
>>> ==18826==    by 0x4A0E589:
>>> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string<char,
>>> std::__1::char_traits<char>, std::__1::allocator<char> > const&)
>>> (device_enumerator.cpp:290)
>>> ==18826==    by 0x4B0C5C1:
>>> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)
>>> (device_enumerator_udev.cpp:344)
>>> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184
>>> free'd
>>> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)
>>> ==18826==    by 0x4AFD391:
>>> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)
>>> ==18826==    by 0x4A61E9E:
>>> std::__1::default_delete<libcamera::PipelineHandler>::operator()(libcamera::PipelineHandler*)
>>> const (memory:2363)
>>> ==18826==    by 0x4A61C3F:
>>> std::__1::__shared_ptr_pointer<libcamera::PipelineHandler*,
>>> std::__1::default_delete<libcamera::PipelineHandler>,
>>> std::__1::allocator<libcamera::PipelineHandler> >::__on_zero_shared()
>>> (memory:3536)
>>> ==18826==    by 0x49AC479:
>>> std::__1::__shared_count::__release_shared() (memory:3440)
>>> ==18826==    by 0x49AC41E:
>>> std::__1::__shared_weak_count::__release_shared() (memory:3482)
>>> ==18826==    by 0x49B35DB:
>>> std::__1::shared_ptr<libcamera::PipelineHandler>::~shared_ptr()
>>> (memory:4207)
>>> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private()
>>> (camera.cpp:300)
>>> ==18826==    by 0x49C213A:
>>> std::__1::default_delete<libcamera::Camera::Private>::operator()(libcamera::Camera::Private*)
>>> const (memory:2363)
>>> ==18826==    by 0x49C20BE:
>>> std::__1::unique_ptr<libcamera::Camera::Private,
>>> std::__1::default_delete<libcamera::Camera::Private>
>>>> ::reset(libcamera::Camera::Private*) (memory:2618)
>>> ==18826==    by 0x49B3878:
>>> std::__1::unique_ptr<libcamera::Camera::Private,
>>> std::__1::default_delete<libcamera::Camera::Private> >::~unique_ptr()
>>> (memory:2572)
>>> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)
>>> ==18826==  Block was alloc'd at
>>> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)
>>> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in
>>> /usr/lib64/libc++.so.1.0)
>>> ==18826==    by 0x4AFD409:
>>> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)
>>> (uvcvideo.cpp:562)
>>> ==18826==    by 0x4A58A4F:
>>> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)
>>> (pipeline_handler.cpp:640)
>>> ==18826==    by 0x49C9A83:
>>> libcamera::CameraManager::Private::createPipelineHandlers()
>>> (camera_manager.cpp:148)
>>> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init()
>>> (camera_manager.cpp:127)
>>> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run()
>>> (camera_manager.cpp:104)
>>> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread()
>>> (thread.cpp:288)
>>> ==18826==    by 0x4A76520:
>>> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_
>>> (type_traits:3480)
>>> ==18826==    by 0x4A7642D: void
>>> std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct,
>>> std::__1::default_delete<std::__1::__thread_struct> >, void
>>> (libcamera::Thread::*)(), libcamera::Thread*,
>>> 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
>>> std::__1::default_delete<std::__1::__thread_struct> >, void
>>> (libcamera::Thread::*)(), libcamera::Thread*>&,
>>> std::__1::__tuple_indices<2ul>) (thread:273)
>>> ==18826==    by 0x4A75D95: void*
>>> std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
>>> std::__1::default_delete<std::__1::__thread_struct> >, void
>>> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)
>>> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)
>>>
>>> The change below should fix this. I'll let you add a comment at the
>>> beginning of PipelineHandler::disconnect() to explain what is going on
>>> :-)
>> I will go with something like the following:
>>
>> +       /* Since cameras_ holds a weak reference of the cameras,
>> +        * we just need to empty the vector here instead of
>> cameras_.clear() it.
>> +        */
> I'm not sure that's quite describing what's happening.
>
>
>
>
>
>>> diff --git a/src/libcamera/pipeline_handler.cpp
>>> b/src/libcamera/pipeline_handler.cpp
>>> index bad79dcb6219..8358266d83ff 100644
>>> --- a/src/libcamera/pipeline_handler.cpp
>>> +++ b/src/libcamera/pipeline_handler.cpp
>>> @@ -559,7 +559,9 @@ void
>>> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
>>>     */
>>>    void PipelineHandler::disconnect()
>>>    {
>>> -    for (std::weak_ptr<Camera> ptr : cameras_) {
>>> +    std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
> This moves all cameras out of the cameras_ vector - into a new
> 'temporary' holding container.
>
> Moving them into the new container, ensures that the references are kept
> constant, but they are no longer 'held' by the pipeline.

hmm, cameras_ is a vector of weak_ptr references, right? According to 
the concepts I have

in my head (:D), pipeline doesn't _really_ hold a (strong)reference 
through this vector.

Hence, isn't moving from cameras_ to temporary container merely to avoid 
.clear()  call below?

>
> I haven't checked, but I presume that this means below in the loop, the
> code that was unable to remove something because a reference was held -
> is now able to - because essentially the cameras_ object has been
> cleared before entering the loop - instead of after.
>
>>> +
>>> +    for (std::weak_ptr<Camera> ptr : cameras) {
>>>            std::shared_ptr<Camera> camera = ptr.lock();
>>>            if (!camera)
>>>                continue;
>>> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()
>>>            camera->disconnect();
>>>            manager_->removeCamera(camera);
>>>        }
>>> -
>>> -    cameras_.clear();
> And because the cameras vector above was created with local scope - all
> of it's contents get cleared at this point now automatically.

Yes, but according to docs online, .clear() also tries to destroy the 
objects in the vector

and I am under the impression that, since they are weak-references here, 
they are

_not_ supposed to be destroyed(or decrement the refcount) the objects. 
(that should happen

automatically when refcount drops to 0).

>
>
>
>>>    }
>>>      /**
>> Quick question: You mentioned on IRC that this can be folded into a patch.
>>
>> Did you mean squash it into existing patch or treat this as a separate
>> patch?
> That usually means squash it into the existing relevant (this current?)
> patch.
>
>
>>>> Signed-off-by: Umang Jain <email@uajain.com>
>>>> ---
>>>>    src/libcamera/camera_manager.cpp | 3 ---
>>>>    1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/camera_manager.cpp
>>>> b/src/libcamera/camera_manager.cpp
>>>> index 576856a..b8128df 100644
>>>> --- a/src/libcamera/camera_manager.cpp
>>>> +++ b/src/libcamera/camera_manager.cpp
>>>> @@ -63,7 +63,6 @@ private:
>>>>        bool initialized_;
>>>>        int status_;
>>>>    -    std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>>>>        std::unique_ptr<DeviceEnumerator> enumerator_;
>>>>          IPAManager ipaManager_;
>>>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()
>>>>                LOG(Camera, Debug)
>>>>                    << "Pipeline handler \"" << factory->name()
>>>>                    << "\" matched";
>>>> -            pipes_.push_back(std::move(pipe));
>>>>            }
>>>>        }
>>>>    @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()
>>>>         * they all get destroyed before the device enumerator deletes the
>>>>         * media devices.
>>>>         */
>>> The above comment needs to be updated.
>>>
>>> -     * Release all references to cameras and pipeline handlers to ensure
>>> -     * they all get destroyed before the device enumerator deletes the
>>> -     * media devices.
>>> +     * Release all references to cameras to ensure they all get
>>> destroyed
>>> +     * before the device enumerator deletes the media devices.
>>>
>>>> -    pipes_.clear();
>>>>        cameras_.clear();
>>>>          enumerator_.reset(nullptr);
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> 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
Laurent Pinchart June 15, 2020, 3:05 p.m. UTC | #5
Hello,

On Mon, Jun 15, 2020 at 03:01:51PM +0100, Kieran Bingham wrote:
> On 15/06/2020 14:40, Umang Jain wrote:
> > On 6/12/20 3:31 PM, Laurent Pinchart wrote:
> >> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:
> >>> This placeholder for the pipeline-handlers created by CameraManager,
> >>> was responsible to hold a reference to pipeline-handlers which were
> >>> not getting dropped anywhere in the code-path. Instead of introducing
> >>> a fix of decrementing the reference, it is decided to eliminate this
> >>> vector entirely from  the CameraManager. This is due to the fact that
> >>> the vector does not seem to have much use in CameraManager and thus
> >>> reducing futile book-keeping.
> >> 
> >> I think the change is good, but "does not seem to have much use" sounds
> >> like you're not really sure :-)
> >>
> >> The pipes_ vector has been there pretty much since day 1, and served the
> >> purpose of storing pipeline handler instances when they were not yet
> >> referenced by the Camera class. This was used to retrieve cameras, and
> >> to delete pipeline handler instances when stopping the camera manager.
> >> In
> >>
> >> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49
> >> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Date:   Thu Jan 3 13:10:37 2019 +0200
> >>
> >>      libcamera: camera_manager: Register cameras with the camera manager
> >>
> >> cameras were registered directly with the camera manager, and the pipes_
> >> vector was only used to delete pipeline handler instances. In
> >>
> >> commit 5b02e03199b79165086135236d8fb9b2c673aae1
> >> Author: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >> Date:   Tue Jan 22 16:31:39 2019 +0100
> >>
> >>      libcamera: camera: Associate cameras with their pipeline handler
> >>
> >> the pipeline handler instances got stored in a shared_ptr, referenced
> >> from both the pipes_ vector and the Camera class. The explicit delete
> >> was removed, and pipes_ was simply cleared instead. We could have
> >> removed the pipes_ vector at that point, as pipeline handlers were
> >> referenced from the Camera class.
> >>
> >> It would be nice to summarize this in the commit message to explain why
> >> the pipes_ vector isn't needed anymore.
> >>
> >>> Since the vector is eliminated, there are no unchecked reference of the
> >>> pipeline-handlers. This fixes the initial issue of dangling driver
> >>> directories
> >>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on
> >>> 'unbind' → 'bind'
> >>> operation while the CameraManager is running. The directories were
> >>> still kept
> >>> around even after 'unbind' because of the pipeline-handler's
> >>> unchecked reference
> >>> holding onto them.
> >> 
> >> Have you tried to run the hotplug test under valgrind ? There are lots
> >> of use-after-free issues with this patch applied:
> >>
> >> ==18826== Thread 2:
> >> ==18826== Invalid read of size 8
> >> ==18826==    at 0x4A5A06C:
> >> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
> >> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::size()
> >> const (vector:656)
> >> ==18826==    by 0x4A59778:
> >> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
> >> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::clear()
> >> (vector:771)
> >> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect()
> >> (pipeline_handler.cpp:571)
> >> ==18826==    by 0x4A586C6:
> >> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)
> >> (pipeline_handler.cpp:545)
> >> ==18826==    by 0x4A5EC3D:
> >> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
> >> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)
> >> (bound_method.h:198)
> >> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs<void,
> >> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,
> >> std::__1::integer_sequence<unsigned long, 0ul>) (bound_method.h:124)
> >> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs<void,
> >> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)
> >> (bound_method.h:133)
> >> ==18826==    by 0x49ABB26:
> >> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr<libcamera::BoundMethodPackBase>,
> >> bool) (bound_method.cpp:85)
> >> ==18826==    by 0x4A5EB85:
> >> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
> >> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)
> >> (bound_method.h:193)
> >> ==18826==    by 0x4A0F600:
> >> libcamera::Signal<libcamera::MediaDevice*>::emit(libcamera::MediaDevice*)
> >> (signal.h:127)
> >> ==18826==    by 0x4A0E589:
> >> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string<char,
> >> std::__1::char_traits<char>, std::__1::allocator<char> > const&)
> >> (device_enumerator.cpp:290)
> >> ==18826==    by 0x4B0C5C1:
> >> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)
> >> (device_enumerator_udev.cpp:344)
> >> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184
> >> free'd
> >> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)
> >> ==18826==    by 0x4AFD391:
> >> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)
> >> ==18826==    by 0x4A61E9E:
> >> std::__1::default_delete<libcamera::PipelineHandler>::operator()(libcamera::PipelineHandler*)
> >> const (memory:2363)
> >> ==18826==    by 0x4A61C3F:
> >> std::__1::__shared_ptr_pointer<libcamera::PipelineHandler*,
> >> std::__1::default_delete<libcamera::PipelineHandler>,
> >> std::__1::allocator<libcamera::PipelineHandler> >::__on_zero_shared()
> >> (memory:3536)
> >> ==18826==    by 0x49AC479:
> >> std::__1::__shared_count::__release_shared() (memory:3440)
> >> ==18826==    by 0x49AC41E:
> >> std::__1::__shared_weak_count::__release_shared() (memory:3482)
> >> ==18826==    by 0x49B35DB:
> >> std::__1::shared_ptr<libcamera::PipelineHandler>::~shared_ptr()
> >> (memory:4207)
> >> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private()
> >> (camera.cpp:300)
> >> ==18826==    by 0x49C213A:
> >> std::__1::default_delete<libcamera::Camera::Private>::operator()(libcamera::Camera::Private*)
> >> const (memory:2363)
> >> ==18826==    by 0x49C20BE:
> >> std::__1::unique_ptr<libcamera::Camera::Private,
> >> std::__1::default_delete<libcamera::Camera::Private>
> >> >::reset(libcamera::Camera::Private*) (memory:2618)
> >> ==18826==    by 0x49B3878:
> >> std::__1::unique_ptr<libcamera::Camera::Private,
> >> std::__1::default_delete<libcamera::Camera::Private> >::~unique_ptr()
> >> (memory:2572)
> >> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)
> >> ==18826==  Block was alloc'd at
> >> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)
> >> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in
> >> /usr/lib64/libc++.so.1.0)
> >> ==18826==    by 0x4AFD409:
> >> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)
> >> (uvcvideo.cpp:562)
> >> ==18826==    by 0x4A58A4F:
> >> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)
> >> (pipeline_handler.cpp:640)
> >> ==18826==    by 0x49C9A83:
> >> libcamera::CameraManager::Private::createPipelineHandlers()
> >> (camera_manager.cpp:148)
> >> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init()
> >> (camera_manager.cpp:127)
> >> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run()
> >> (camera_manager.cpp:104)
> >> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread()
> >> (thread.cpp:288)
> >> ==18826==    by 0x4A76520:
> >> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_
> >> (type_traits:3480)
> >> ==18826==    by 0x4A7642D: void
> >> std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct,
> >> std::__1::default_delete<std::__1::__thread_struct> >, void
> >> (libcamera::Thread::*)(), libcamera::Thread*,
> >> 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
> >> std::__1::default_delete<std::__1::__thread_struct> >, void
> >> (libcamera::Thread::*)(), libcamera::Thread*>&,
> >> std::__1::__tuple_indices<2ul>) (thread:273)
> >> ==18826==    by 0x4A75D95: void*
> >> std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
> >> std::__1::default_delete<std::__1::__thread_struct> >, void
> >> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)
> >> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)
> >>
> >> The change below should fix this. I'll let you add a comment at the
> >> beginning of PipelineHandler::disconnect() to explain what is going on
> >> :-)
> > 
> > I will go with something like the following:
> > 
> > +       /* Since cameras_ holds a weak reference of the cameras,
> > +        * we just need to empty the vector here instead of cameras_.clear() it.
> > +        */
> 
> I'm not sure that's quite describing what's happening.
> 
> >> diff --git a/src/libcamera/pipeline_handler.cpp
> >> b/src/libcamera/pipeline_handler.cpp
> >> index bad79dcb6219..8358266d83ff 100644
> >> --- a/src/libcamera/pipeline_handler.cpp
> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> @@ -559,7 +559,9 @@ void
> >> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
> >>    */
> >>   void PipelineHandler::disconnect()
> >>   {
> >> -    for (std::weak_ptr<Camera> ptr : cameras_) {
> >> +    std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
> 
> This moves all cameras out of the cameras_ vector - into a new
> 'temporary' holding container.
> 
> Moving them into the new container, ensures that the references are kept
> constant, but they are no longer 'held' by the pipeline.
> 
> I haven't checked, but I presume that this means below in the loop, the
> code that was unable to remove something because a reference was held -
> is now able to - because essentially the cameras_ object has been
> cleared before entering the loop - instead of after.
> 
> >> +
> >> +    for (std::weak_ptr<Camera> ptr : cameras) {
> >>           std::shared_ptr<Camera> camera = ptr.lock();
> >>           if (!camera)
> >>               continue;
> >> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()
> >>           camera->disconnect();
> >>           manager_->removeCamera(camera);
> >>       }
> >> -
> >> -    cameras_.clear();
> 
> And because the cameras vector above was created with local scope - all
> of it's contents get cleared at this point now automatically.

This change is needed because when the last reference to the last camera
is dropped, the corresponding pipeline handler gets deleted. This can
happen in the manager_->removeCamera(camera) call. If we iterate over
cameras_, and access cameras_ after the loop, we will access freed
memory. This function thus needs to make sure it won't access any member
of the PipelineHandler class at a point where the pipeline handler may
have been deleted.

> >>   }
> >>     /**
> > 
> > Quick question: You mentioned on IRC that this can be folded into a patch.
> > 
> > Did you mean squash it into existing patch or treat this as a separate
> > patch?
> 
> That usually means squash it into the existing relevant (this current?)
> patch.
> 
> >>> Signed-off-by: Umang Jain <email@uajain.com>
> >>> ---
> >>>   src/libcamera/camera_manager.cpp | 3 ---
> >>>   1 file changed, 3 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/camera_manager.cpp
> >>> b/src/libcamera/camera_manager.cpp
> >>> index 576856a..b8128df 100644
> >>> --- a/src/libcamera/camera_manager.cpp
> >>> +++ b/src/libcamera/camera_manager.cpp
> >>> @@ -63,7 +63,6 @@ private:
> >>>       bool initialized_;
> >>>       int status_;
> >>>   -    std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> >>>       std::unique_ptr<DeviceEnumerator> enumerator_;
> >>>         IPAManager ipaManager_;
> >>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()
> >>>               LOG(Camera, Debug)
> >>>                   << "Pipeline handler \"" << factory->name()
> >>>                   << "\" matched";
> >>> -            pipes_.push_back(std::move(pipe));
> >>>           }
> >>>       }
> >>>   @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()
> >>>        * they all get destroyed before the device enumerator deletes the
> >>>        * media devices.
> >>>        */
> >> 
> >> The above comment needs to be updated.
> >>
> >> -     * Release all references to cameras and pipeline handlers to ensure
> >> -     * they all get destroyed before the device enumerator deletes the
> >> -     * media devices.
> >> +     * Release all references to cameras to ensure they all get destroyed
> >> +     * before the device enumerator deletes the media devices.
> >>
> >>> -    pipes_.clear();
> >>>       cameras_.clear();
> >>>         enumerator_.reset(nullptr);
Laurent Pinchart June 15, 2020, 3:21 p.m. UTC | #6
Hi Umang,

On Mon, Jun 15, 2020 at 03:02:23PM +0000, Umang Jain wrote:
> On 6/15/20 7:31 PM, Kieran Bingham wrote:
> > On 15/06/2020 14:40, Umang Jain wrote:
> >> On 6/12/20 3:31 PM, Laurent Pinchart wrote:
> >>> On Thu, Jun 11, 2020 at 05:16:02PM +0000, Umang Jain wrote:
> >>>> This placeholder for the pipeline-handlers created by CameraManager,
> >>>> was responsible to hold a reference to pipeline-handlers which were
> >>>> not getting dropped anywhere in the code-path. Instead of introducing
> >>>> a fix of decrementing the reference, it is decided to eliminate this
> >>>> vector entirely from  the CameraManager. This is due to the fact that
> >>>> the vector does not seem to have much use in CameraManager and thus
> >>>> reducing futile book-keeping.
> >>> 
> >>> I think the change is good, but "does not seem to have much use" sounds
> >>> like you're not really sure :-)
> >>>
> >>> The pipes_ vector has been there pretty much since day 1, and served the
> >>> purpose of storing pipeline handler instances when they were not yet
> >>> referenced by the Camera class. This was used to retrieve cameras, and
> >>> to delete pipeline handler instances when stopping the camera manager.
> >>> In
> >>>
> >>> commit f3695e9b09ce4a88d6e0480d9e5058673af34a49
> >>> Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Date:   Thu Jan 3 13:10:37 2019 +0200
> >>>
> >>>       libcamera: camera_manager: Register cameras with the camera manager
> >>>
> >>> cameras were registered directly with the camera manager, and the pipes_
> >>> vector was only used to delete pipeline handler instances. In
> >>>
> >>> commit 5b02e03199b79165086135236d8fb9b2c673aae1
> >>> Author: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>> Date:   Tue Jan 22 16:31:39 2019 +0100
> >>>
> >>>       libcamera: camera: Associate cameras with their pipeline handler
> >>>
> >>> the pipeline handler instances got stored in a shared_ptr, referenced
> >>> from both the pipes_ vector and the Camera class. The explicit delete
> >>> was removed, and pipes_ was simply cleared instead. We could have
> >>> removed the pipes_ vector at that point, as pipeline handlers were
> >>> referenced from the Camera class.
> >>>
> >>> It would be nice to summarize this in the commit message to explain why
> >>> the pipes_ vector isn't needed anymore.
> >>>
> >>>> Since the vector is eliminated, there are no unchecked reference of the
> >>>> pipeline-handlers. This fixes the initial issue of dangling driver
> >>>> directories
> >>>> (for e.g. for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on
> >>>> 'unbind' → 'bind'
> >>>> operation while the CameraManager is running. The directories were
> >>>> still kept
> >>>> around even after 'unbind' because of the pipeline-handler's
> >>>> unchecked reference
> >>>> holding onto them.
> >>> 
> >>> Have you tried to run the hotplug test under valgrind ? There are lots
> >>> of use-after-free issues with this patch applied:
> >>>
> >>> ==18826== Thread 2:
> >>> ==18826== Invalid read of size 8
> >>> ==18826==    at 0x4A5A06C:
> >>> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
> >>> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::size()
> >>> const (vector:656)
> >>> ==18826==    by 0x4A59778:
> >>> std::__1::vector<std::__1::weak_ptr<libcamera::Camera>,
> >>> std::__1::allocator<std::__1::weak_ptr<libcamera::Camera> > >::clear()
> >>> (vector:771)
> >>> ==18826==    by 0x4A5887D: libcamera::PipelineHandler::disconnect()
> >>> (pipeline_handler.cpp:571)
> >>> ==18826==    by 0x4A586C6:
> >>> libcamera::PipelineHandler::mediaDeviceDisconnected(libcamera::MediaDevice*)
> >>> (pipeline_handler.cpp:545)
> >>> ==18826==    by 0x4A5EC3D:
> >>> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
> >>> libcamera::MediaDevice*>::invoke(libcamera::MediaDevice*)
> >>> (bound_method.h:198)
> >>> ==18826==    by 0x4A5ECC4: void libcamera::BoundMethodArgs<void,
> >>> libcamera::MediaDevice*>::invokePack<0ul>(libcamera::BoundMethodPackBase*,
> >>> std::__1::integer_sequence<unsigned long, 0ul>) (bound_method.h:124)
> >>> ==18826==    by 0x4A5EA9C: libcamera::BoundMethodArgs<void,
> >>> libcamera::MediaDevice*>::invokePack(libcamera::BoundMethodPackBase*)
> >>> (bound_method.h:133)
> >>> ==18826==    by 0x49ABB26:
> >>> libcamera::BoundMethodBase::activatePack(std::__1::shared_ptr<libcamera::BoundMethodPackBase>,
> >>> bool) (bound_method.cpp:85)
> >>> ==18826==    by 0x4A5EB85:
> >>> libcamera::BoundMethodMember<libcamera::PipelineHandler, void,
> >>> libcamera::MediaDevice*>::activate(libcamera::MediaDevice*, bool)
> >>> (bound_method.h:193)
> >>> ==18826==    by 0x4A0F600:
> >>> libcamera::Signal<libcamera::MediaDevice*>::emit(libcamera::MediaDevice*)
> >>> (signal.h:127)
> >>> ==18826==    by 0x4A0E589:
> >>> libcamera::DeviceEnumerator::removeDevice(std::__1::basic_string<char,
> >>> std::__1::char_traits<char>, std::__1::allocator<char> > const&)
> >>> (device_enumerator.cpp:290)
> >>> ==18826==    by 0x4B0C5C1:
> >>> libcamera::DeviceEnumeratorUdev::udevNotify(libcamera::EventNotifier*)
> >>> (device_enumerator_udev.cpp:344)
> >>> ==18826==  Address 0x5bd0af8 is 136 bytes inside a block of size 184
> >>> free'd
> >>> ==18826==    at 0x48379CB: free (vg_replace_malloc.c:540)
> >>> ==18826==    by 0x4AFD391:
> >>> libcamera::PipelineHandlerUVC::~PipelineHandlerUVC() (uvcvideo.cpp:61)
> >>> ==18826==    by 0x4A61E9E:
> >>> std::__1::default_delete<libcamera::PipelineHandler>::operator()(libcamera::PipelineHandler*)
> >>> const (memory:2363)
> >>> ==18826==    by 0x4A61C3F:
> >>> std::__1::__shared_ptr_pointer<libcamera::PipelineHandler*,
> >>> std::__1::default_delete<libcamera::PipelineHandler>,
> >>> std::__1::allocator<libcamera::PipelineHandler> >::__on_zero_shared()
> >>> (memory:3536)
> >>> ==18826==    by 0x49AC479:
> >>> std::__1::__shared_count::__release_shared() (memory:3440)
> >>> ==18826==    by 0x49AC41E:
> >>> std::__1::__shared_weak_count::__release_shared() (memory:3482)
> >>> ==18826==    by 0x49B35DB:
> >>> std::__1::shared_ptr<libcamera::PipelineHandler>::~shared_ptr()
> >>> (memory:4207)
> >>> ==18826==    by 0x49B0982: libcamera::Camera::Private::~Private()
> >>> (camera.cpp:300)
> >>> ==18826==    by 0x49C213A:
> >>> std::__1::default_delete<libcamera::Camera::Private>::operator()(libcamera::Camera::Private*)
> >>> const (memory:2363)
> >>> ==18826==    by 0x49C20BE:
> >>> std::__1::unique_ptr<libcamera::Camera::Private,
> >>> std::__1::default_delete<libcamera::Camera::Private>
> >>>> ::reset(libcamera::Camera::Private*) (memory:2618)
> >>> 
> >>> ==18826==    by 0x49B3878:
> >>> std::__1::unique_ptr<libcamera::Camera::Private,
> >>> std::__1::default_delete<libcamera::Camera::Private> >::~unique_ptr()
> >>> (memory:2572)
> >>> ==18826==    by 0x49B12D2: libcamera::Camera::~Camera() (camera.cpp:517)
> >>> ==18826==  Block was alloc'd at
> >>> ==18826==    at 0x483679F: malloc (vg_replace_malloc.c:309)
> >>> ==18826==    by 0x4D0F76B: operator new(unsigned long) (in
> >>> /usr/lib64/libc++.so.1.0)
> >>> ==18826==    by 0x4AFD409:
> >>> libcamera::PipelineHandlerUVCFactory::createInstance(libcamera::CameraManager*)
> >>> (uvcvideo.cpp:562)
> >>> ==18826==    by 0x4A58A4F:
> >>> libcamera::PipelineHandlerFactory::create(libcamera::CameraManager*)
> >>> (pipeline_handler.cpp:640)
> >>> ==18826==    by 0x49C9A83:
> >>> libcamera::CameraManager::Private::createPipelineHandlers()
> >>> (camera_manager.cpp:148)
> >>> ==18826==    by 0x49C9952: libcamera::CameraManager::Private::init()
> >>> (camera_manager.cpp:127)
> >>> ==18826==    by 0x49C97F2: libcamera::CameraManager::Private::run()
> >>> (camera_manager.cpp:104)
> >>> ==18826==    by 0x4A71CDC: libcamera::Thread::startThread()
> >>> (thread.cpp:288)
> >>> ==18826==    by 0x4A76520:
> >>> _ZNSt3__18__invokeIMN9libcamera6ThreadEFvvEPS2_JEvEEDTcldsdeclsr3std3__1E7forwardIT0_Efp0_Efp_spclsr3std3__1E7forwardIT1_Efp1_EEEOT_OS6_DpOS7_
> >>> (type_traits:3480)
> >>> ==18826==    by 0x4A7642D: void
> >>> std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct,
> >>> std::__1::default_delete<std::__1::__thread_struct> >, void
> >>> (libcamera::Thread::*)(), libcamera::Thread*,
> >>> 2ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
> >>> std::__1::default_delete<std::__1::__thread_struct> >, void
> >>> (libcamera::Thread::*)(), libcamera::Thread*>&,
> >>> std::__1::__tuple_indices<2ul>) (thread:273)
> >>> ==18826==    by 0x4A75D95: void*
> >>> std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct,
> >>> std::__1::default_delete<std::__1::__thread_struct> >, void
> >>> (libcamera::Thread::*)(), libcamera::Thread*> >(void*) (thread:284)
> >>> ==18826==    by 0x52B8F3F: start_thread (pthread_create.c:479)
> >>>
> >>> The change below should fix this. I'll let you add a comment at the
> >>> beginning of PipelineHandler::disconnect() to explain what is going on
> >>> :-)
> >> 
> >> I will go with something like the following:
> >>
> >> +       /* Since cameras_ holds a weak reference of the cameras,
> >> +        * we just need to empty the vector here instead of
> >> cameras_.clear() it.
> >> +        */
> > 
> > I'm not sure that's quite describing what's happening.
> >
> >>> diff --git a/src/libcamera/pipeline_handler.cpp
> >>> b/src/libcamera/pipeline_handler.cpp
> >>> index bad79dcb6219..8358266d83ff 100644
> >>> --- a/src/libcamera/pipeline_handler.cpp
> >>> +++ b/src/libcamera/pipeline_handler.cpp
> >>> @@ -559,7 +559,9 @@ void
> >>> PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
> >>>     */
> >>>    void PipelineHandler::disconnect()
> >>>    {
> >>> -    for (std::weak_ptr<Camera> ptr : cameras_) {
> >>> +    std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
> >
> > This moves all cameras out of the cameras_ vector - into a new
> > 'temporary' holding container.
> >
> > Moving them into the new container, ensures that the references are kept
> > constant, but they are no longer 'held' by the pipeline.
> 
> hmm, cameras_ is a vector of weak_ptr references, right? According to 
> the concepts I have in my head (:D), pipeline doesn't _really_ hold a
> (strong)reference through this vector. Hence, isn't moving from
> cameras_ to temporary container merely to avoid .clear()  call below?

As explained in a parallel reply, the reason why we need a local copy of
the vector here is that the call to manager_->removeCamera(camera) will
destroy the camera if nothing else holds a (strong) reference to it. If
the application doesn't hold a reference to any of the cameras for the
pipeline handler, all cameras created by the pipeline handler will be
destroyed. As cameras hold a reference to the pipeline handler, the
pipeline handler will thus get destroyed by the last
manager_->removeCamera(camera) call in the loop. The loop itself, and
the cameras_.clear() line, will then access freed memory. Moving
cameras_ to a local cameras vector avoids this problem by ensuring that
code paths that can run after the pipeline handler gets deleted don't
access member variables. And yes, it's valid to delete the 'this' object
from within a member function of the class (in this case indirectly,
through manager_->removeCamera(camera)), provided we take care not to
access freed memory.

> > I haven't checked, but I presume that this means below in the loop, the
> > code that was unable to remove something because a reference was held -
> > is now able to - because essentially the cameras_ object has been
> > cleared before entering the loop - instead of after.
> >
> >>> +
> >>> +    for (std::weak_ptr<Camera> ptr : cameras) {
> >>>            std::shared_ptr<Camera> camera = ptr.lock();
> >>>            if (!camera)
> >>>                continue;
> >>> @@ -567,8 +569,6 @@ void PipelineHandler::disconnect()
> >>>            camera->disconnect();
> >>>            manager_->removeCamera(camera);
> >>>        }
> >>> -
> >>> -    cameras_.clear();
> >
> > And because the cameras vector above was created with local scope - all
> > of it's contents get cleared at this point now automatically.
> 
> Yes, but according to docs online, .clear() also tries to destroy the 
> objects in the vector and I am under the impression that, since they
> are weak-references here, they are _not_ supposed to be destroyed(or
> decrement the refcount) the objects. (that should happen automatically
> when refcount drops to 0).

cameras_.clear() destroys the objects contained in the vector. The
objects are of type std::weak_ptr<Camera>. The weak pointers thus get
destroyed, and as they are weak pointers, they don't decrease the
reference count on the object they point to, as they don't hold a
reference in the first place.

> >>>    }
> >>>      /**
> >> 
> >> Quick question: You mentioned on IRC that this can be folded into a patch.
> >>
> >> Did you mean squash it into existing patch or treat this as a separate
> >> patch?
> > 
> > That usually means squash it into the existing relevant (this current?)
> > patch.
> >
> >>>> Signed-off-by: Umang Jain <email@uajain.com>
> >>>> ---
> >>>>    src/libcamera/camera_manager.cpp | 3 ---
> >>>>    1 file changed, 3 deletions(-)
> >>>>
> >>>> diff --git a/src/libcamera/camera_manager.cpp
> >>>> b/src/libcamera/camera_manager.cpp
> >>>> index 576856a..b8128df 100644
> >>>> --- a/src/libcamera/camera_manager.cpp
> >>>> +++ b/src/libcamera/camera_manager.cpp
> >>>> @@ -63,7 +63,6 @@ private:
> >>>>        bool initialized_;
> >>>>        int status_;
> >>>>    -    std::vector<std::shared_ptr<PipelineHandler>> pipes_;
> >>>>        std::unique_ptr<DeviceEnumerator> enumerator_;
> >>>>          IPAManager ipaManager_;
> >>>> @@ -144,7 +143,6 @@ int CameraManager::Private::init()
> >>>>                LOG(Camera, Debug)
> >>>>                    << "Pipeline handler \"" << factory->name()
> >>>>                    << "\" matched";
> >>>> -            pipes_.push_back(std::move(pipe));
> >>>>            }
> >>>>        }
> >>>>    @@ -162,7 +160,6 @@ void CameraManager::Private::cleanup()
> >>>>         * they all get destroyed before the device enumerator deletes the
> >>>>         * media devices.
> >>>>         */
> >>> 
> >>> The above comment needs to be updated.
> >>>
> >>> -     * Release all references to cameras and pipeline handlers to ensure
> >>> -     * they all get destroyed before the device enumerator deletes the
> >>> -     * media devices.
> >>> +     * Release all references to cameras to ensure they all get destroyed
> >>> +     * before the device enumerator deletes the media devices.
> >>>
> >>>> -    pipes_.clear();
> >>>>        cameras_.clear();
> >>>>          enumerator_.reset(nullptr);

Patch

diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 576856a..b8128df 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -63,7 +63,6 @@  private:
 	bool initialized_;
 	int status_;
 
-	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
 	std::unique_ptr<DeviceEnumerator> enumerator_;
 
 	IPAManager ipaManager_;
@@ -144,7 +143,6 @@  int CameraManager::Private::init()
 			LOG(Camera, Debug)
 				<< "Pipeline handler \"" << factory->name()
 				<< "\" matched";
-			pipes_.push_back(std::move(pipe));
 		}
 	}
 
@@ -162,7 +160,6 @@  void CameraManager::Private::cleanup()
 	 * they all get destroyed before the device enumerator deletes the
 	 * media devices.
 	 */
-	pipes_.clear();
 	cameras_.clear();
 
 	enumerator_.reset(nullptr);