[libcamera-devel] [PATCH v2 13/17] v4l2: v4l2_camera: Clear pending requests on freeBuffers and streamOff

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Jun 19, 2020 at 02:41:19PM +0900, Paul Elder wrote:
> V4L2 allows buffer queueing before streamon while libcamera does not.
> The compatibility layer thus saves these buffers in a pending queue
> until streamon, and then automatically queues them. However, this
> pending queue is not cleared when the buffers are freed, so the
> following sequence of actions will cause a use-after-free:
> 
> 1. queue buffers
> 2. free buffers
>    - buffers from 1. stay in pending queue but have been freed
> 3. queue buffers
> 4. streamon
>    - buffers from 1. are enqueued, then the buffers from 3. are
>      enqueued. Use-after-free segfault when libcamera tries to handle
>      the enqueued buffers from 1.
> 
> Fix this by clearing the pending request queue upon buffers being freed.
> Also clear the pending request queue on streamOff, for correctness.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> ---
> Changes in v2:
> - also clear pending request queue on streamOff
> - clarify the issue in changelog
> ---
>  src/v4l2/v4l2_camera.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 99d34b9..301a80e 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -148,6 +148,7 @@ void V4L2Camera::freeBuffers()
>  	Stream *stream = *camera_->streams().begin();
>  
>  	bufferAllocator_->free(stream);
> +	pendingRequests_.clear();

Wouldn't it be safer to first clear pendingRequests_, as the requests
reference the buffers ? The Request destructor should no access the
buffers, so in practice it should be fine, but inverting the order would
seem less fragile to me.

>  }
>  
>  FileDescriptor V4L2Camera::getBufferFd(unsigned int index)
> @@ -187,7 +188,8 @@ int V4L2Camera::streamOn()
>  
>  int V4L2Camera::streamOff()
>  {
> -	/* \todo Restore buffers to reqbufs state? */
> +	pendingRequests_.clear();
> +

Should this be moved after the isRunning_ check ?

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	if (!isRunning_)
>  		return 0;
>  

-- 
Regards,

Laurent Pinchart