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

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

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