| Message ID | 20210309143518.31405-1-m.cichy@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Marian Thank you for the patch. 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@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. Thanks! -Umang
Hi Marian Thank you for the patch. 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@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_; 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()); 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;
On 3/10/21 6:30 PM, Laurent Pinchart wrote: > Hi Marian > > Thank you for the patch. > > 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@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_; > 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. 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. Regards, Marian > 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; > >
On 3/10/21 6:53 PM, Marian Cichy wrote: > > On 3/10/21 6:30 PM, Laurent Pinchart wrote: >> Hi Marian >> >> Thank you for the patch. >> >> 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@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_; >> 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. > > > 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. > > > Regards, > > Marian > > Forget about this. My Mail reader decided to clip half of the message. Works fine, but there should still be an Error message if buffer acquisition failed. Regards, Marian >> 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; >> >>
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@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;
On 3/10/21 7:44 PM, Laurent Pinchart wrote: > 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@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 ? I had a situation with my special camera sensor and driver which triggered it at first, some time ago, but I forgot what it was. Now I just triggered it by hard-coding ret = GST_FLOW_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;
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(); break; }
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@pengutronix.de> --- src/gstreamer/gstlibcamerasrc.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)