[libcamera-devel] [PATCH v4 1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 15 17:21:22 CEST 2020
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 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.
> >> + */
> >
> > 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 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