[libcamera-devel] [PATCH v3 05/11] libcamera: request: A request canary

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Mar 25, 2021 at 01:42:25PM +0000, Kieran Bingham wrote:
> Request objects are created and owned by the application, but are of
> course utilised widely throughout the internals of libcamera.
> 
> If the application free's the requests while they are still active
> within libcamera a use after free will occur. While this can be trapped
> by tools such as valgrind, given the importance of this object and the
> relationship of external ownership, it may have some value to provide
> Debug build (disabled at Release build) assertions on the condition of
> these objects.
> 
> Make sure the request fails an assertion immediately if used after free.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> I've removed the RFC from this, and I actually even more so believe this
> is a good facility to provide on the Request object.
> 
> Mostly because the Request object is the only object which is given to
> libcamera from the application (yes, created by libcamera, but that's
> separate), and is expected to be free'd by the application.
> 
> If an application free's requests while they are still in use within
> libcamera, the symptoms can be distinctly misleading and lead to rabbit
> holes.
> 
> Therefore, I think the Request object is the one place where extra
> safety checking (in debug builds) is worth while.

Should the canary_ member be compiled out in non-debug builds ?

> Yes, of course if an application free's a request while it's in use with
> libcamera - then it's the applications fault, not ours - but because of
> the nature of requests, this could be an easy trap to fall into - and I
> don't want to find that reported as bugs in libcamera.

When Request will become Extensible, do you think the canary_ field
could be moved to the Private class ? That would make me feel better
about this feature :-)

Actually, maybe we could use the d_ pointer and o_ pointer to replace
the canary, but checking the point to each other ?

> ---
>  include/libcamera/request.h |  2 ++
>  src/libcamera/request.cpp   | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 4cf5ff3f7d3b..965ffa6b45b2 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -79,6 +79,8 @@ private:
>  	const uint64_t cookie_;
>  	Status status_;
>  	bool cancelled_;
> +
> +	uint32_t canary_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 7b7ef6814686..c4258480b12b 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -19,6 +19,8 @@
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/tracepoints.h"
>  
> +#define REQUEST_CANARY 0x1F2E3D4C
> +
>  /**
>   * \file request.h
>   * \brief Describes a frame capture request to be processed by a camera
> @@ -90,6 +92,8 @@ Request::Request(Camera *camera, uint64_t cookie)
>  
>  	LIBCAMERA_TRACEPOINT(request_construct, this);
>  
> +	canary_ = REQUEST_CANARY;

You can move this to the member initializer list.

> +
>  	LOG(Request, Debug) << "Created request - cookie: " << cookie_;
>  }
>  
> @@ -100,6 +104,8 @@ Request::~Request()
>  	delete metadata_;
>  	delete controls_;
>  	delete validator_;
> +
> +	canary_ = 0;
>  }
>  
>  /**
> @@ -114,6 +120,8 @@ Request::~Request()
>   */
>  void Request::reuse(ReuseFlag flags)
>  {
> +	ASSERT(canary_ == REQUEST_CANARY);
> +
>  	LIBCAMERA_TRACEPOINT(request_reuse, this);
>  
>  	pending_.clear();
> @@ -179,6 +187,8 @@ void Request::reuse(ReuseFlag flags)
>   */
>  int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>  {
> +	ASSERT(canary_ == REQUEST_CANARY);
> +
>  	if (!stream) {
>  		LOG(Request, Error) << "Invalid stream reference";
>  		return -EINVAL;
> @@ -214,6 +224,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
>   */
>  FrameBuffer *Request::findBuffer(const Stream *stream) const
>  {
> +	ASSERT(canary_ == REQUEST_CANARY);
> +
>  	const auto it = bufferMap_.find(stream);
>  	if (it == bufferMap_.end())
>  		return nullptr;
> @@ -281,6 +293,7 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>   */
>  void Request::complete()
>  {
> +	ASSERT(canary_ == REQUEST_CANARY);
>  	ASSERT(status_ == RequestPending);
>  	ASSERT(!hasPendingBuffers());
>  
> @@ -306,6 +319,8 @@ void Request::complete()
>   */
>  bool Request::completeBuffer(FrameBuffer *buffer)
>  {
> +	ASSERT(canary_ == REQUEST_CANARY);
> +
>  	LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);
>  
>  	int ret = pending_.erase(buffer);
> @@ -326,6 +341,9 @@ std::string Request::toString() const
>  	/* Pending, Completed, Cancelled(X). */
>  	static const char *statuses = "PCX";
>  
> +	if (canary_ != REQUEST_CANARY)
> +		return "Invalid Canary on Request";

As all bets are off in this case, should we ASSERT() to catch this
condition as early as possible ?

> +
>  	/* Example Output: Request(55:P:1/2:6523524) */
>  	ss << "Request (" << sequence_ << ":" << statuses[status_] << ":"
>  	   << pending_.size() << "/" << bufferMap_.size() << ":"

-- 
Regards,

Laurent Pinchart