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

[Umang Jain]

Hi Kieran,

On 11/30/22 6:53 PM, Kieran Bingham wrote:
> Quoting Umang Jain (2022-11-30 10:35:47)
>> Hi Kieran,
>>
>> 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.
>>>
>>>> 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.
>> 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.

All in all, similar to dangling pointers problem.
>
> --
> Kieran
>
>
>>> 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
>>>> -- 
>>>> 2.35.1
>>>>