[libcamera-devel,1/2] libcamera: Request: validate state on complete

11445 diff mbox series

Requests should only be completed from the RequestPending state.

Requests which are completed from the RequestCancelled, or RequestComplete
state, will indicate that a double-complete has been called on the Request,
or that it has been used internally after it has been given back to the
application.

Ensure that this can be caught early if it occurs by enforcing the state
required with an assert.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
This issue has been seen while developing the IPA for the IPU3.

Requests are completed, and then completed again while closing.
It is likely that this occured, where a request was completed, and was
then re-used.

A use case where the Request was deleted instead of re-used was also
seen which caused a segmentation fault instead, due to the use of the
memory after free().

 src/libcamera/request.cpp | 2 ++
 1 file changed, 2 insertions(+)

Hi Kieran,

Thank you for the patch.

On Tue, Mar 02, 2021 at 12:23:40PM +0000, Kieran Bingham wrote:
> Requests should only be completed from the RequestPending state.
> 
> Requests which are completed from the RequestCancelled, or RequestComplete
> state, will indicate that a double-complete has been called on the Request,
> or that it has been used internally after it has been given back to the
> application.
> 
> Ensure that this can be caught early if it occurs by enforcing the state
> required with an assert.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

> ---
> This issue has been seen while developing the IPA for the IPU3.
> 
> Requests are completed, and then completed again while closing.
> It is likely that this occured, where a request was completed, and was
> then re-used.
> 
> A use case where the Request was deleted instead of re-used was also
> seen which caused a segmentation fault instead, due to the use of the
> memory after free().
> 
>  src/libcamera/request.cpp | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index e561ce1d5d0e..24c3694de5fc 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -262,7 +262,9 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>   */
>  void Request::complete()
>  {
> +	ASSERT(status_ == RequestPending);
>  	ASSERT(!hasPendingBuffers());
> +
>  	status_ = cancelled_ ? RequestCancelled : RequestComplete;
>  
>  	LOG(Request, Debug)

Hi Kieran

On 3/2/21 5:53 PM, Kieran Bingham wrote:
> Requests should only be completed from the RequestPending state.
>
> Requests which are completed from the RequestCancelled, or RequestComplete
> state, will indicate that a double-complete has been called on the Request,
> or that it has been used internally after it has been given back to the
> application.
>
> Ensure that this can be caught early if it occurs by enforcing the state
> required with an assert.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Umang Jain <email@uajain.com>
>
> ---
> This issue has been seen while developing the IPA for the IPU3.
>
> Requests are completed, and then completed again while closing.
> It is likely that this occured, where a request was completed, and was
> then re-used.
>
> A use case where the Request was deleted instead of re-used was also
> seen which caused a segmentation fault instead, due to the use of the
> memory after free().
>
>   src/libcamera/request.cpp | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index e561ce1d5d0e..24c3694de5fc 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -262,7 +262,9 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>    */
>   void Request::complete()
>   {
> +	ASSERT(status_ == RequestPending);
>   	ASSERT(!hasPendingBuffers());
> +
>   	status_ = cancelled_ ? RequestCancelled : RequestComplete;
>   
>   	LOG(Request, Debug)

Hi Kieran,

Thanks for your work.

On 2021-03-02 12:23:40 +0000, Kieran Bingham wrote:
> Requests should only be completed from the RequestPending state.
> 
> Requests which are completed from the RequestCancelled, or RequestComplete
> state, will indicate that a double-complete has been called on the Request,
> or that it has been used internally after it has been given back to the
> application.
> 
> Ensure that this can be caught early if it occurs by enforcing the state
> required with an assert.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> ---
> This issue has been seen while developing the IPA for the IPU3.
> 
> Requests are completed, and then completed again while closing.
> It is likely that this occured, where a request was completed, and was
> then re-used.
> 
> A use case where the Request was deleted instead of re-used was also
> seen which caused a segmentation fault instead, due to the use of the
> memory after free().
> 
>  src/libcamera/request.cpp | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index e561ce1d5d0e..24c3694de5fc 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -262,7 +262,9 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>   */
>  void Request::complete()
>  {
> +	ASSERT(status_ == RequestPending);
>  	ASSERT(!hasPendingBuffers());
> +
>  	status_ = cancelled_ ? RequestCancelled : RequestComplete;
>  
>  	LOG(Request, Debug)
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel