[libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers
Umang Jain
email at uajain.com
Mon Jun 15 15:40:10 CEST 2020
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 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
> :-)
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 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);
More information about the libcamera-devel
mailing list