[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:05:28 CEST 2020
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 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.
>
> 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 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