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

[Hirokazu Honda]

Hi Umang, thank you for reviewing.

On Tue, Aug 31, 2021 at 9:02 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> The commit message can be somewhat more contextual of the problem.
>
> On 8/31/21 1:35 PM, Hirokazu Honda wrote:
> > This fixes the crash when post processing is needed and some capture
> > request is being processed by PipelineHandler upon calling
> > CameraDevice::close().
> > The crash happens in Request::completeBuffer(), which is invoked by
> > Camera::stop(). It uses FrameBuffer after free. The FrameBuffer is
> > allocated and owned by FrameBufferAllocator, and FrameBufferAllocator
> > is owned by CamerStream. So Camera::stop() should be executed before
> > destroying CameraStream.
>
>
> One question: I can see the problem but what's exactly is crashing in
> Request::completeBuffer() is something we should know? Is it the ASSERT?
> That might help us to define the problem statement better.
>

It is buffer->_d()->setRequest(nullptr) in
https://git.linuxtv.org/libcamera.git/tree/src/libcamera/request.cpp#n339
The buffer has been deallocated.

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

Thanks. I replaced my commit message with this.

-Hiro
>
> >
> > 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..74a95a2a 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -423,10 +423,10 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
> >
> >   void CameraDevice::close()
> >   {
> > -     streams_.clear();
> > -
> >       stop();
> >
> > +     streams_.clear();
>
>
> How about this being a part of a CameraDevice::stop() instead ?
>
> > +
> >       camera_->release();
> >   }
> >