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

[Laurent Pinchart]

Hello,

On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:
> 
> 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

Indeed :-)

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

This is what is happening already:
https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool

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

That's indeed a problem. Maybe

	if (wrap && wrap->request_)

above ? Nicolas, any opinion ?

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