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

[Marian Cichy]

On 3/10/21 6:53 PM, Marian Cichy wrote:
>
> On 3/10/21 6:30 PM, Laurent Pinchart wrote:
>> 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());
>
>
> Did you mean
>
> state->cam_->queueRequest(wrap->request_)
>
> the wrapped request is not a unique_ptr here.
>
>
> 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.
>
>
> Regards,
>
> Marian
>
>

Forget about this. My Mail reader decided to clip half of the message.


Works fine, but there should still be an Error message if buffer 
acquisition failed.


Regards,

Marian


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