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

[Laurent Pinchart]

Hello,

On Wed, Nov 30, 2022 at 11:36:15AM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Paul Elder (2022-11-30 11:31:06)
> > On Wed, Nov 30, 2022 at 07:24:52PM +0800, Umang Jain wrote:
> > > On 11/30/22 6:53 PM, Kieran Bingham wrote:
> > > > Quoting Umang Jain (2022-11-30 10:35:47)
> > > > > On 11/30/22 1:39 AM, Kieran Bingham via libcamera-devel 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.

Well, if applications really mess it up, it's bound to happen. I do
however agree that in this case it seems possibly too easy for
applications to get it wrong.

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

We have an issue if the application gets it wrong. How would a unit test
help here ?

> > > > > If the buffers to be associated with the Requests are allocated and
> > > > > mis-managed by the application themselves - how would that be address by
> > > > > libcamera-core?
> > > > > 
> > > > > I am reading the Request->addBuffer() documentation and clearly mentioned:
> > > > > 
> > > > >    *
> > > > >    * A reference to the buffer is stored in the request. The caller is responsible
> > > > >    * for ensuring that the buffer will remain valid until the request complete
> > > > >    * callback is called.
> > > > >    *
> > > > > 
> > > > > In this case the buffers from manually freed before the request got
> > > > > completed (via set 'cancelled' status and completeRequest() codepath in
> > > > > PIpelineHandler::Stop()). So that's not the right thing to do / happen.
> > > >
> > > > aha, ok - if it's documented, then we're probably good with this for
> > > > now.
> > > > 
> > > > I do wonder if we should have some reverse association such that when
> > > > you add a buffer to a request, the buffer knows which request it's
> > > > contained in, and can 'remove' itself when it's freed... but ... that
> > > > makes me wonder if we could legitimately have a single buffer in
> > > > multiple requests which would break that possibility ... so maybe we're
> > > > fine with just the documentation as we have it.
> > > 
> > > We can enforce a std::shared_ptr <> model (but that's also not 100% safety)
> > > but not sure how well it'll scale with language bindings in the future.

std::shared_ptr<> are usually painful, and not just for language
bindings, they tend to creep in everywhere when you start using them. I
wouldn't go that route if possible.

> > > All in all, similar to dangling pointers problem.
> > 
> > Oops, I didn't notice this thread in the meantime.
> > 
> > So this patch is good on its own and I don't have to do anything extra
> > then? :)
> 
> That depends on how likely it is that applications could /cause/ this
> problem?

There's something I don't quite get. The requests are destroyed after
stop(). I thus expect all requests to have completed, either normally or
because they have been cancelled, before the stop() function returns.
How comes Request::Private::doCancelRequest(), called from the request
destructor, sees a non-empty pending_ list ?

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

-- 
Regards,

Laurent Pinchart