[libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free when acquire_buffer fails
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 12 00:14:58 CET 2021
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.
> > > > > > 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