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

[Umang Jain]

Hi Laurent,

On 3/11/21 5:29 PM, Laurent Pinchart wrote:
> 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 ?
OR null-check createRequest()'s returned object before wrapping it (i.e. 
ensure RequestWrap doesn't wrap a nullptr from the start), so we don't 
have to check everything for the wrap->request_ is valid or not.
>
>>>    		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;