[libcamera-devel] [PATCH v3] android: camera_device: Fix crash in calling CameraDevice::close()

[Jacopo Mondi]

On Thu, Sep 02, 2021 at 10:40:39AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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.


> > > > 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