[libcamera-devel] [PATCH] lc-compliance: simple_capture: Free Requests properly

[Kieran Bingham]

Quoting Paul Elder (2022-11-30 11:27:40)
> On Tue, Nov 29, 2022 at 05:39:16PM +0000, Kieran Bingham wrote:
> > Quoting Paul Elder via libcamera-devel (2022-11-29 11:00:05)
> > > In the simple capture tests, in the capture functions, the Requests were
> > > auto-deallocated by the function going out of scope after the test
> > > completed. However, before the end of the scope, the Framebuffers that
> > > the Requests referred to were manually freed. Thus when the Requests
> > > were deallocated, they tried to cancel their Framebuffers, which involve
> > > setting the status to cancelled, which obviously causes a segfault
> > > because the Framebuffers had already been freed.
> > 
> > This worries me that we have a path that could let application cause a
> > segfault in libcamera. That's not good.
> 
> That's true.
> 
> Should we fix this instead in Request then? By checking that
> FramBuffer::_d() == nullptr in Request::Private::doCancelRequest() like
> rsglobal does in the github issue [1]?

As this is clearly a situation that applciations could get themselves into -
perhaps we need to have 'something' but with a clear message to tellng
them they've messed up ?


 
> [1] https://github.com/kbingham/libcamera/issues/60#issue-1450103563
> 
> > 
> > > Fix this by moving the list of Requests to a member variable and
> > > deallocating them before deallocating the Framebuffers at stop() time.
> > 
> > Fixing this here seems reasonable, but I'm concerned that we now need a
> > unit test in libcamera unit tests which ensure or validate the lifetimes
> > of freeing buffers/requests.
> 
> Yeah that's true, this should probably be split into another compliance
> test in itself.
> 
> > 
> > If I understand it, that means that we have a lifetime management issue
> > still between the Request and the Buffer object that may be associated
> > with it.
> > 
> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=171
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > But I am not sure we should consider the underlying issue resolved yet.
> > I feel like the crash in lc-compliance is simply telling us we have a
> > fault with our API.
> > 
> > The question is if we should still fix lc-compliance here or not - or if
> > we should keep it 'broken' so that we can validate whatever fix we do
> > internally in libcamera?
> 
> I think split this specific issue into a separate smaller test. I
> don't think there's a need to test this specific issue in all
> combinations that we test currently in the simple capture test.

Absolutely. It's just about figuring out what the applications could be
likely to get wrong, and if /we've/ got it wrong ... then I guess
applications could.

--
Kieran


> 
> 
> Thanks,
> 
> Paul
> 
> > 
> > 
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > >  src/apps/lc-compliance/simple_capture.cpp | 7 +++----
> > >  src/apps/lc-compliance/simple_capture.h   | 1 +
> > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp
> > > index ab5cb35c..cf4d7cf3 100644
> > > --- a/src/apps/lc-compliance/simple_capture.cpp
> > > +++ b/src/apps/lc-compliance/simple_capture.cpp
> > > @@ -65,6 +65,7 @@ void SimpleCapture::stop()
> > >         camera_->requestCompleted.disconnect(this);
> > >  
> > >         Stream *stream = config_->at(0).stream();
> > > +       requests_.clear();
> > >         allocator_->free(stream);
> > >  }
> > >  
> > > @@ -95,7 +96,6 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)
> > >         captureLimit_ = numRequests;
> > >  
> > >         /* Queue the recommended number of reqeuests. */
> > > -       std::vector<std::unique_ptr<libcamera::Request>> requests;
> > >         for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > >                 std::unique_ptr<Request> request = camera_->createRequest();
> > >                 ASSERT_TRUE(request) << "Can't create request";
> > > @@ -104,7 +104,7 @@ void SimpleCaptureBalanced::capture(unsigned int numRequests)
> > >  
> > >                 ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request";
> > >  
> > > -               requests.push_back(std::move(request));
> > > +               requests_.push_back(std::move(request));
> > >         }
> > >  
> > >         /* Run capture session. */
> > > @@ -156,7 +156,6 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> > >         captureLimit_ = numRequests;
> > >  
> > >         /* Queue the recommended number of reqeuests. */
> > > -       std::vector<std::unique_ptr<libcamera::Request>> requests;
> > >         for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > >                 std::unique_ptr<Request> request = camera_->createRequest();
> > >                 ASSERT_TRUE(request) << "Can't create request";
> > > @@ -165,7 +164,7 @@ void SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> > >  
> > >                 ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
> > >  
> > > -               requests.push_back(std::move(request));
> > > +               requests_.push_back(std::move(request));
> > >         }
> > >  
> > >         /* Run capture session. */
> > > diff --git a/src/apps/lc-compliance/simple_capture.h b/src/apps/lc-compliance/simple_capture.h
> > > index fd9d2a97..2911d601 100644
> > > --- a/src/apps/lc-compliance/simple_capture.h
> > > +++ b/src/apps/lc-compliance/simple_capture.h
> > > @@ -32,6 +32,7 @@ protected:
> > >         std::shared_ptr<libcamera::Camera> camera_;
> > >         std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> > >         std::unique_ptr<libcamera::CameraConfiguration> config_;
> > > +       std::vector<std::unique_ptr<libcamera::Request>> requests_;
> > >  };
> > >  
> > >  class SimpleCaptureBalanced : public SimpleCapture
> > > -- 
> > > 2.35.1
> > >