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

17016 diff mbox series

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@ideasonboard.com>
---
 src/cam/kms_sink.cpp | 7 ++++---
 src/cam/kms_sink.h   | 5 +++--
 2 files changed, 7 insertions(+), 5 deletions(-)

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

Hi Kieran,

On Sun, Aug 07, 2022 at 08:42:42PM +0100, Kieran Bingham wrote:
> 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@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?

The drmRequest was passed to the KMSSink::Request constructor:

	pending_ = std::make_unique<Request>(drmRequest, camRequest);

which took ownership of the pointer, storing it in a
std::unique_ptr<DRM::AtomicRequest>. From that point onwards, there's no
risk of leaks. As the processRequest() function doesn't return between
the creating of drmRequest and the construction of the Request, there's
thus no leak. The next patch in the series adds a return in the middle,
which prompted me to write this patch to avoid a leak (in a neater way
that adding a manual delete in the return path introduced by 3/3).

> Assuming this is fine,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@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)
> >                 {
> >                 }
> >

On Sun, 7 Aug 2022 at 19:01, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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<>.
>

LGTM, and also tested successfully.

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@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_);
> +       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
>