[libcamera-devel] [PATCH v2] libcamera: gst: Fix double-free when acquire_buffer fails

[Nicolas Dufresne]

Le jeudi 11 mars 2021 à 13:59 +0200, Laurent Pinchart a écrit :
> Hello,
> 
> On Thu, Mar 11, 2021 at 05:05:39PM +0530, Umang Jain wrote:
> > 
> > Hi Marian,
> > 
> > I noticed a few potential null/segfault issues with this approach
> > 
> > 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 =

Please keep seperate, and add missing nullptr check in between.

> > > 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 ?

This way we don't have to be defensive in so many places.

> 
> > >                 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;
>