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

[Paul Elder]

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

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


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