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

[Marian Cichy]

On 3/10/21 7:44 PM, Laurent Pinchart wrote:
> Hi Marian
>
> On Wed, Mar 10, 2021 at 06:53:30PM +0100, Marian Cichy wrote:
>> On 3/10/21 6:30 PM, Laurent Pinchart wrote:
>>> 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_;
> *1 (marker for the discussion below)
>
>>>    	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());
>>
>> Did you mean
>>
>> state->cam_->queueRequest(wrap->request_)
>>
>> the wrapped request is not a unique_ptr here.
> Isn't it ? *1 changes request_ from a Request * to a
> std::unique_ptr<Request>.
>
>> With this change, I tested both the success and the error path and it
>> looks fine. However, I think there should be a printed message (maybe
>> GST_WARNING) when we get in the error path, as there is no indicator now
>> that buffer acquistion failed.
> Good idea.
>
> Stupid question, how do you trigger that error ?


I had a situation with my special camera sensor and driver which 
triggered it at first, some time ago, but I forgot what it was. Now I 
just triggered it by hard-coding ret = GST_FLOW_ERROR. :-)


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