| Message ID | 20200611171528.9381-2-email@uajain.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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);
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);
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
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
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);
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);
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);
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(-)