[libcamera-devel] lc-compliance: Fix segfault on request destruction

17918 diff mbox series

The Request destructor cancels all associated framebuffers, and
therefore assumes they are all still valid. The SimpleCaptureBalanced
and SimpleCaptureUnbalanced classes that run the capture sessions on
lc-compliance however were freeing the framebuffers before the requests,
stored in an automatic variable, were destroyed. This resulted in a
segfault when running lc-compliance.

Solve the issue by moving the requests vector to the SimpleCapture class
and making sure we clear it on stop() before freeing the framebuffers.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

 src/apps/lc-compliance/simple_capture.cpp | 8 ++++----
 src/apps/lc-compliance/simple_capture.h   | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

Hi Nícolas,

Thanks for looking at this issue.

Quoting Nícolas F. R. A. Prado via libcamera-devel (2022-11-30 00:10:44)
> The Request destructor cancels all associated framebuffers, and
> therefore assumes they are all still valid. The SimpleCaptureBalanced
> and SimpleCaptureUnbalanced classes that run the capture sessions on
> lc-compliance however were freeing the framebuffers before the requests,
> stored in an automatic variable, were destroyed. This resulted in a
> segfault when running lc-compliance.
> 
> Solve the issue by moving the requests vector to the SimpleCapture class
> and making sure we clear it on stop() before freeing the framebuffers.

I'm afraid Paul has 'just' beaten you to it with a patch posted
yesterday:

 - https://patchwork.libcamera.org/patch/17906/


Could you review and test his patch please?

--
Kieran


> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> ---
> 
>  src/apps/lc-compliance/simple_capture.cpp | 8 ++++----
>  src/apps/lc-compliance/simple_capture.h   | 1 +
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/apps/lc-compliance/simple_capture.cpp b/src/apps/lc-compliance/simple_capture.cpp
> index ab5cb35c11f2..d02c7b45f180 100644
> --- a/src/apps/lc-compliance/simple_capture.cpp
> +++ b/src/apps/lc-compliance/simple_capture.cpp
> @@ -64,6 +64,8 @@ void SimpleCapture::stop()
>  
>         camera_->requestCompleted.disconnect(this);
>  
> +       requests_.clear();
> +
>         Stream *stream = config_->at(0).stream();
>         allocator_->free(stream);
>  }
> @@ -95,7 +97,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 +105,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 +157,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 +165,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 fd9d2a97fd8d..2911d6019923 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.38.1
>

On Wed, Nov 30, 2022 at 10:55:50AM +0000, Kieran Bingham wrote:
> Hi Nícolas,
> 
> Thanks for looking at this issue.
> 
> Quoting Nícolas F. R. A. Prado via libcamera-devel (2022-11-30 00:10:44)
> > The Request destructor cancels all associated framebuffers, and
> > therefore assumes they are all still valid. The SimpleCaptureBalanced
> > and SimpleCaptureUnbalanced classes that run the capture sessions on
> > lc-compliance however were freeing the framebuffers before the requests,
> > stored in an automatic variable, were destroyed. This resulted in a
> > segfault when running lc-compliance.
> > 
> > Solve the issue by moving the requests vector to the SimpleCapture class
> > and making sure we clear it on stop() before freeing the framebuffers.
> 
> I'm afraid Paul has 'just' beaten you to it with a patch posted
> yesterday:
> 
>  - https://patchwork.libcamera.org/patch/17906/
> 
> 
> Could you review and test his patch please?

Ah indeed, I missed that. Well, it's almost exactly the same patch, so that's
reassuring :). I'll reply there, thanks.

Nícolas