[libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free when acquire_buffer fails

[Umang Jain]

Hi Marian,

I noticed a few potential null/segfault issues with this approach

On 3/11/21 3:03 PM, Marian Cichy wrote:
> If gst_buffer_pool_acquire_buffer in gst_libcamera_task_run fails, the
> unique_ptr to the request-object gets reset and hence, its destructor
> is called. However, the wrap-object points to the same object and is
> still alive at this moment. When the task_run-function is finished, the
> destructor of the wrap-object is called, which in return calls the
> destructor of the request-object again.
>
> Instead of taking care of both, the request and the wrap-object, we can
> move the request to the wrap which will then effectively take care of
> the request object automatically.
...take care of the request s/object/object-deletion/ maybe?

>
> Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
> Suggested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>   src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++++++++----------------
>   1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 636c14df..e86c3d7f 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -52,19 +52,19 @@ GST_DEBUG_CATEGORY_STATIC(source_debug);
>   #define GST_CAT_DEFAULT source_debug
>   
>   struct RequestWrap {
> -	RequestWrap(Request *request);
> +	RequestWrap(std::unique_ptr<Request> request);
>   	~RequestWrap();
>   
>   	void attachBuffer(GstBuffer *buffer);
>   	GstBuffer *detachBuffer(Stream *stream);
>   
>   	/* For ptr comparison only. */
I think you need to remove this comment too
> -	Request *request_;
> +	std::unique_ptr<Request> request_;
>   	std::map<Stream *, GstBuffer *> buffers_;
>   };
>   
> -RequestWrap::RequestWrap(Request *request)
> -	: request_(request)
> +RequestWrap::RequestWrap(std::unique_ptr<Request> request)
> +	: request_(std::move(request))
>   {
>   }
>   
> @@ -74,8 +74,6 @@ RequestWrap::~RequestWrap()
>   		if (item.second)
>   			gst_buffer_unref(item.second);
>   	}
> -
> -	delete request_;
>   }
>   
>   void RequestWrap::attachBuffer(GstBuffer *buffer)
> @@ -164,7 +162,7 @@ GstLibcameraSrcState::requestCompleted(Request *request)
>   	std::unique_ptr<RequestWrap> wrap = std::move(requests_.front());
>   	requests_.pop();
>   
> -	g_return_if_fail(wrap->request_ == request);
> +	g_return_if_fail(wrap->request_.get() == request);
>   
>   	if ((request->status() == Request::RequestCancelled)) {
>   		GST_DEBUG_OBJECT(src_, "Request was cancelled");
> @@ -268,8 +266,7 @@ gst_libcamera_src_task_run(gpointer user_data)
>   	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
>   	GstLibcameraSrcState *state = self->state;
>   
> -	std::unique_ptr<Request> request = state->cam_->createRequest();
> -	auto wrap = std::make_unique<RequestWrap>(request.get());
> +	auto wrap = std::make_unique<RequestWrap>(state->cam_->createRequest());
*1 for marker below
>   	for (GstPad *srcpad : state->srcpads_) {
>   		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
>   		GstBuffer *buffer;
> @@ -279,24 +276,23 @@ gst_libcamera_src_task_run(gpointer user_data)
>   						     &buffer, nullptr);
>   		if (ret != GST_FLOW_OK) {
>   			/*
> -			 * RequestWrap does not take ownership, and we won't be
> -			 * queueing this one due to lack of buffers.
> +			 * RequestWrap has ownership of the rquest, and we
s/rquest/request/
> +			 * won't be queueing this one due to lack of buffers.
>   			 */
> -			request.reset();
> +			wrap.release();
>   			break;
>   		}
>   
>   		wrap->attachBuffer(buffer);
>   	}
>   
> -	if (request) {
> +	if (wrap) {
I think this should be `if (wrap.get())` no? `wrap.release()` if hit, 
will set nullptr to the _managed_ object, so we need to check the 
managed object for null
>   		GLibLocker lock(GST_OBJECT(self));
>   		GST_TRACE_OBJECT(self, "Requesting buffers");
> -		state->cam_->queueRequest(request.get());
> +		state->cam_->queueRequest(wrap->request_.get());
from *1, createRequest() call can return nullptr, hence you have a

std::make_unique<RequestWrap>(nullptr);

which will set request_ as nullptr on the wrap object. It  is fine as 
long as wrap->request_ is not used unlike here, in this block.


>   		state->requests_.push(std::move(wrap));
>   
> -		/* The request will be deleted in the completion handler. */
> -		request.release();
> +		/* The RequestWrap will be deleted in the completion handler. */
>   	}
>   
>   	GstFlowReturn ret = GST_FLOW_OK;