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

[Laurent Pinchart]

Hi Marian

Thank you for the patch.

On Wed, Mar 10, 2021 at 08:35:31PM +0530, Umang Jain wrote:
> On 3/9/21 8:05 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.
> >
> > Also note the wrong comment, which claims that WrapRequest does not
> > take ownership of the request, however, actually it already has
> > ownership.
> >
> > Replacing request.reset() with request.release() doesn't call the
> > destructor on the request-object and only one free happens at the end.
> >
> > Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
> > ---
> >   src/gstreamer/gstlibcamerasrc.cpp | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index a8ed7652..b0194c2f 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -279,10 +279,12 @@ 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
> > +			 * RequestWrap has ownership, and we won't be
> >   			 * queueing this one due to lack of buffers.
> > +			 * So the request will be freed when RequestWrap
> > +			 * goes out of scope.
> >   			 */
> > -			request.reset();
> > +			request.release();
>
> This is a bit problematic as `request` is still in-used beyond this 
> line(there is `if (request)` block below) and here it will set it as 
> NULL. You will need to take care of this situation using the RequestWrap 
> probably. However, one thing I still am not sure of ...
>
> >   			break;
> >   		}
> >   
> 
> I am not sure if why `RequestWrap` is written intentionally in a way, to 
> transfer the ownership of the `Request`. This fact is quite evident 
> looking at the ~RequestWrap().  The larger picture here is (which is why 
> you hit this double-free issue) that, whenever RequestWrap is used - 
> always transfer ownership of Request. So, that's a 2-step manual in 
> order to use the wrap; if the latter is forgotten - we will always hit 
> double free issues.
> 
> Can you please look how the RequestWrap is used elsewhere in the 
> gstreamer plugin and try to see why transfer of ownership is intented in 
> RequestWrap? If it's not essential, we should probably re-fine 
> RequestWrap to really be a "wrapper" and not try to steal the reference 
> - and take the additional responsibility of free-ing it.

The request belongs to the gstreamer element, so we need to keep track
of it and free it. Request instances are managed with std::unique_ptr<>,
so the best option I think would be to simple move the request to the
wrap, using a unique pointer there.

How about this untested change ?

diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index 636c14dff64e..e86c3d7f74c7 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. */
-	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());
 	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
+			 * won't be queueing this one due to lack of buffers.
 			 */
-			request.reset();
+			wrap.release();
 			break;
 		}
 
 		wrap->attachBuffer(buffer);
 	}
 
-	if (request) {
+	if (wrap) {
 		GLibLocker lock(GST_OBJECT(self));
 		GST_TRACE_OBJECT(self, "Requesting buffers");
-		state->cam_->queueRequest(request.get());
+		state->cam_->queueRequest(wrap->request_.get());
 		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;


-- 
Regards,

Laurent Pinchart