[libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free when acquire_buffer fails
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Mar 11 13:22:42 CET 2021
Hi Umang,
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.
> >>> 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