[libcamera-devel] [PATCH 2/3] cam: kms_sink: Make lifetime management of DRM request safer
Kieran Bingham
kieran.bingham at ideasonboard.com
Sun Aug 7 21:42:42 CEST 2022
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
>
More information about the libcamera-devel
mailing list