[libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free when acquire_buffer fails
Marian Cichy
mci at pengutronix.de
Fri Mar 12 18:59:11 CET 2021
On 3/12/21 12:14 AM, Laurent Pinchart wrote:
> Hi Nicolas,
>
> On Thu, Mar 11, 2021 at 04:02:20PM -0500, Nicolas Dufresne wrote:
>> Le jeudi 11 mars 2021 à 14:22 +0200, Laurent Pinchart a écrit :
>>> On Thu, Mar 11, 2021 at 05:47:59PM +0530, Umang Jain wrote:
>>>> On 3/11/21 5:29 PM, Laurent Pinchart wrote:
>>>>> On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:
>>>>>> 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.
>>> But, as far as I understand, code further down this function still have
>>> to run in that case, we can't just do an early return using a null
>>> check. That's why I've asked for Nicolas' feedback, he knows this code
>>> much better than I do.
>> Hmm, right. Something like this should do:
>>
>> if (!request) {
>> GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,...;
>> gst_task_stop(task);
>> return;
>> }
> Something along those lines ?
>
> commit be081f85bbab0c8b3b5f08d73c5bda504be7f2e6
> Author: Marian Cichy <m.cichy at pengutronix.de>
> Date: Thu Mar 11 10:33:25 2021 +0100
>
> libcamera: gst: Fix double-free when acquire_buffer fails
>
> 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.
>
> Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
> Suggested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 636c14dff64e..7b9671200f87 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -52,19 +52,18 @@ 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 +73,6 @@ RequestWrap::~RequestWrap()
> if (item.second)
> gst_buffer_unref(item.second);
> }
> -
> - delete request_;
> }
>
> void RequestWrap::attachBuffer(GstBuffer *buffer)
> @@ -164,7 +161,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");
> @@ -269,7 +266,18 @@ gst_libcamera_src_task_run(gpointer user_data)
> GstLibcameraSrcState *state = self->state;
>
> std::unique_ptr<Request> request = state->cam_->createRequest();
> - auto wrap = std::make_unique<RequestWrap>(request.get());
> + if (!request) {
> + GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,
> + ("Failed to allocate request for camera '%s'.",
> + state->cam_->id().c_str()),
> + ("libcamera::Camera::createRequest() failed"));
> + gst_task_stop(self->task);
> + return;
> + }
> +
> + std::unique_ptr<RequestWrap> wrap =
> + std::make_unique<RequestWrap>(std::move(request));
> +
> for (GstPad *srcpad : state->srcpads_) {
> GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> GstBuffer *buffer;
> @@ -279,24 +287,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;
>
> Marian, would you be able to test this ?
>
> Should the same be done in case wrap becomes null, due to a call to
> gst_buffer_pool_acquire_buffer() failing, insted of continuing to
> processing the flow ? The lack of symmetry between the two codes paths
> bothers me, either because there's an issue there, or because I don't
> understand why they need to be different :-) This can be addressed in a
> subsequent patch, as it's not an issue introduced by this patch.
Hi Laurent,
I tested this, using:
@@ -267,6 +265,7 @@ gst_libcamera_src_task_run(gpointer user_data)
{
GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
GstLibcameraSrcState *state = self->state;
+ static int cnt = 0;
std::unique_ptr<Request> request = state->cam_->createRequest();
auto wrap = std::make_unique<RequestWrap>(request.get());
@@ -275,7 +274,10 @@ gst_libcamera_src_task_run(gpointer user_data)
GstBuffer *buffer;
GstFlowReturn ret;
- ret = gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),
+ if (++cnt == 10)
+ ret = GST_FLOW_ERROR;
+ else
+ ret =
gst_buffer_pool_acquire_buffer(GST_BUFFER_POOL(pool),
as expected, the code skips the failing buffer, than continues it works,
no problems whatsoever. So it's fine.
I see there is still ongoing discussion with you and Nicolas, so I'd say
I wait before sending V3 until it's clear?
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;
More information about the libcamera-devel
mailing list