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

[Umang Jain]

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

Thanks!

-Umang