[libcamera-devel] [PATCH] libcamera: gst: Fix double-free when acquire_buffer fails
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 10 19:44:43 CET 2021
Hi Marian
On Wed, Mar 10, 2021 at 06:53:30PM +0100, Marian Cichy wrote:
> On 3/10/21 6:30 PM, Laurent Pinchart wrote:
> > 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_;
*1 (marker for the discussion below)
> > 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.
Isn't it ? *1 changes request_ from a Request * to a
std::unique_ptr<Request>.
> 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.
Good idea.
Stupid question, how do you trigger that error ?
> > 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