[libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free when acquire_buffer fails
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 12 21:56:57 CET 2021
Hi Marian,
On Fri, Mar 12, 2021 at 06:59:11PM +0100, Marian Cichy wrote:
> On 3/12/21 12:14 AM, Laurent Pinchart wrote:
> > 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?
Nicolas gave a Reviewed-by on this, so I think the other issues can be
addressed on top. I'll push this.
> >>>>>>> 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
More information about the libcamera-devel
mailing list