[libcamera-devel] [PATCH v3] android: camera_device: Fix crash in calling CameraDevice::close()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 2 10:51:02 CEST 2021
Hi Jacopo,
On Thu, Sep 02, 2021 at 10:25:59AM +0200, Jacopo Mondi wrote:
> On Thu, Sep 02, 2021 at 10:40:39AM +0300, Laurent Pinchart wrote:
> > On Thu, Sep 02, 2021 at 08:59:34AM +0200, Jacopo Mondi wrote:
> > > On Thu, Sep 02, 2021 at 06:11:40AM +0300, Laurent Pinchart wrote:
> > > > On Wed, Sep 01, 2021 at 05:47:08PM +0200, Jacopo Mondi wrote:
> > > > > On Wed, Sep 01, 2021 at 03:37:39AM +0900, Hirokazu Honda wrote:
> > > > > > The problem is happening because we seem to add a CameraStream
> > > > > > associated buffer(depending on the CameraStream::Type) to the Request,
> > > > > > in CameraDevice::processCaptureRequest().
> > > > > >
> > > > > > However, when the camera stops, all the current buffers are marked with
> > > > > > FrameMetadata::FrameCancelled and proceed to completion. But the buffer
> > > > > > associated with the CameraStream (that was previously added to the
> > > > > > request) has now been cleared out with a part of streams_.clear(), even
> > > > > > before the camera stop() has been invoked. Any access to those request
> > > > > > buffers after they have been cleared, shall result in a crash.
> > > > >
> > > > > I recall it was painful at the time when we had to deal with flush()
> > > > > to get the right ordering, and I had done the same. However, since
> > > > > stop() is only called by configureStreams() I removed the
> > > > > streams_.clear() there and had issues, so I gave up.
> > > >
> > > > Do you mean you have tried removing streams_.clear() from
> > > > configureStreams() while testing this patch and ran into issues, or that
> > > > you tried that when implementing support for flush() ?
> > >
> > > I meant when developing flush().
> > > I haven't tried to do the same on top of this patch, but it might be
> > > good to do so ?
> >
> > I think so, because if it works, it's a good cleanup (possibly as part
> > of this patch actually, not on top), and if it doesn't, there's
> > something fishy :-) Would you be able to test it ?
>
> With this applied
>
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -544,6 +544,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> LOG(HAL, Error) << "No streams in configuration";
> return -EINVAL;
> }
> + streams_.reserve(stream_list->num_streams);
>
> #if defined(OS_CHROMEOS)
> if (!validateCropRotate(*stream_list))
> @@ -560,14 +561,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> return -EINVAL;
> }
>
> - /*
> - * Clear and remove any existing configuration from previous calls, and
> - * ensure the required entries are available without further
> - * reallocation.
> - */
> - streams_.clear();
> - streams_.reserve(stream_list->num_streams);
> -
> std::vector<Camera3StreamConfig> streamConfigs;
> streamConfigs.reserve(stream_list->num_streams);
>
> I can easily get a segfault running CTS.
> I analyzed the coredump and the stack trace doesn't help much
>
> (soraka-libcamera-gdb) bt
> #0 0x000000043983e000 in ?? ()
> #1 0x00007eee396ee5b5 in base::OnceCallback<void ()>::Run() && (this=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/callback.h:102
> #2 base::TaskAnnotator::RunTask (this=<optimized out>, trace_event_name=<optimized out>, pending_task=0x7eee1c009310) at ../../../libchrome-0.0.1/platform2/libchrome/base/task/common/task_annotator.cc:163
> #3 0x00007eee396ff465 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl (this=this at entry=0x7eee2c035ed0, continuation_lazy_now=continuation_lazy_now at entry=0x7eee34bd0910)
> at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:332
> #4 0x00007eee396ff27c in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork (this=0x7eee2c035ed0)
> at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:252
> #5 0x00007eee396a322d in base::MessagePumpDefault::Run (this=0x7eee1c00cc70, delegate=<optimized out>) at ../../../libchrome-0.0.1/platform2/libchrome/base/message_loop/message_pump_default.cc:39
> #6 0x00007eee396ff730 in base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run (this=<optimized out>, application_tasks_allowed=<optimized out>, timeout=...)
> at ../../../libchrome-0.0.1/platform2/libchrome/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc:446
> #7 0x00007eee396d0325 in base::RunLoop::Run (this=0x7eee34bd0aa0) at ../../../libchrome-0.0.1/platform2/libchrome/base/run_loop.cc:124
> #8 0x00007eee3971d04e in base::Thread::ThreadMain (this=0x7eee2c034090) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/thread.cc:382
> #9 0x00007eee39718f3d in base::(anonymous namespace)::ThreadFunc (params=0x7eee2c033c50) at ../../../libchrome-0.0.1/platform2/libchrome/base/threading/platform_thread_posix.cc:87
> #10 0x00007eee395adfbf in start_thread (arg=0x7eee34bd1640) at pthread_create.c:463
> #11 0x00007eee393f735f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
> (soraka-libcamera-gdb) quit
>
> The last ?? in #0 makes me think I might not have symbols for the
> camera service, which I cannot easily recompile due to the cros-debug
> USE flag issue. I need to rebuild all the packages and I've now kicked
> it.
>
> One day I'll learn to shut up and ack patches without asking for
> reasons to fall into rabbit holes like this one.
Thanks for the quick test.
Hiro, is this something you can look at ?
> > > > > But I have this patch gone through cros_camera_test and also CTS seems
> > > > > fine. Also looking at the code it seems an harmless change so I wonder
> > > > > what was wrong at the time.
> > > > >
> > > > > As far as I can tell:
> > > > >
> > > > > Tested-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > Acked-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > >
> > > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > > ---
> > > > > > src/android/camera_device.cpp | 4 ++--
> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > index 8ca76719..fda77db4 100644
> > > > > > --- a/src/android/camera_device.cpp
> > > > > > +++ b/src/android/camera_device.cpp
> > > > > > @@ -423,8 +423,6 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
> > > > > >
> > > > > > void CameraDevice::close()
> > > > > > {
> > > > > > - streams_.clear();
> > > > > > -
> > > > > > stop();
> > > > > >
> > > > > > camera_->release();
> > > > > > @@ -457,6 +455,8 @@ void CameraDevice::stop()
> > > > > > camera_->stop();
> > > > > >
> > > > > > descriptors_.clear();
> > > > > > + streams_.clear();
> > > > > > +
> > > > > > state_ = State::Stopped;
> > > > > > }
> > > > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list