[libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jun 12 12:01:31 CEST 2020
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 at 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 at 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 at 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);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list