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

[Laurent Pinchart]

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 ?

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