[libcamera-devel] [PATCH 2/3] cam: kms_sink: Make lifetime management of DRM request safer

[Kieran Bingham]

Quoting Laurent Pinchart via libcamera-devel (2022-08-07 19:00:59)
> The drmRequest is KMSSink::processRequest() is created as a naked
> pointer, passed to the constructor of the KMSSink::Request object that
> stores it in a std::unique_ptr<>, and used later in the function. The
> current implementation is safe, but could be prone to both memory leaks
> and use-after-free bugs if modified. Improve it by replacing the naked
> pointer with a std::unique_ptr<>.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/cam/kms_sink.cpp | 7 ++++---
>  src/cam/kms_sink.h   | 5 +++--
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index 37a3bd50a2bf..16435ede6b6a 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -301,7 +301,8 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)
>         DRM::FrameBuffer *drmBuffer = iter->second.get();
>  
>         unsigned int flags = DRM::AtomicRequest::FlagAsync;
> -       DRM::AtomicRequest *drmRequest = new DRM::AtomicRequest(&dev_);

I'm a little bemused that this 'new' is removed, and now created with a
unique_ptr - but there's no corresponding removal of a delete?

Was it already leaking? or was it already cleared up by the Request in a
way that is still compatible with the unique_ptr?

Assuming this is fine,

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +       std::unique_ptr<DRM::AtomicRequest> drmRequest =
> +               std::make_unique<DRM::AtomicRequest>(&dev_);
>         drmRequest->addProperty(plane_, "FB_ID", drmBuffer->id());
>  
>         if (!active_ && !queued_) {
> @@ -324,12 +325,12 @@ bool KMSSink::processRequest(libcamera::Request *camRequest)
>                 flags |= DRM::AtomicRequest::FlagAllowModeset;
>         }
>  
> -       pending_ = std::make_unique<Request>(drmRequest, camRequest);
> +       pending_ = std::make_unique<Request>(std::move(drmRequest), camRequest);
>  
>         std::lock_guard<std::mutex> lock(lock_);
>  
>         if (!queued_) {
> -               int ret = drmRequest->commit(flags);
> +               int ret = pending_->drmRequest_->commit(flags);
>                 if (ret < 0) {
>                         std::cerr
>                                 << "Failed to commit atomic request: "
> diff --git a/src/cam/kms_sink.h b/src/cam/kms_sink.h
> index 4a0a872cb653..8f5f08667cea 100644
> --- a/src/cam/kms_sink.h
> +++ b/src/cam/kms_sink.h
> @@ -38,8 +38,9 @@ private:
>         class Request
>         {
>         public:
> -               Request(DRM::AtomicRequest *drmRequest, libcamera::Request *camRequest)
> -                       : drmRequest_(drmRequest), camRequest_(camRequest)
> +               Request(std::unique_ptr<DRM::AtomicRequest> drmRequest,
> +                       libcamera::Request *camRequest)
> +                       : drmRequest_(std::move(drmRequest)), camRequest_(camRequest)
>                 {
>                 }
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>