[libcamera-devel] [PATCH 2/2] libcamera: request: A request canary

[Jacopo Mondi]

Hi Kieran

On Mon, Jan 30, 2023 at 06:02:44PM +0000, Kieran Bingham via libcamera-devel wrote:
> Request objects are created and owned by the application, but are
> utilised widely throughout the internals of libcamera.
>

I would have sworn me moved Request in and out from the Camera..

        unique_ptr<Request> Camera::createRequest();
        int Camera::queueRequest(unique_ptr<Request> r);

But we indeed pass it by referece...

        int Camera::queueRequest(Request *r);

I wonder

- Once a Request is queued and before it is signaled as complete,
  should applications be able to access it ?

- Request are signaled as complete through a signal, I guess a signal
  cannot transport a unique_ptr<> back ?

> 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
> extended assertions on the condition of these objects.
>
> Make sure the request fails an assertion immediately if used after free.
>
> Further more, this allows us to immediately reject invalid Request
> objects directly from the Camera queueRequest API.
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> ---
>
> v2?
>  - Added canary to both the public and private request objects.
>  - Added validation to the Camera queueRequest().
>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/request.h |  2 ++
>  include/libcamera/request.h          |  4 +++
>  src/libcamera/camera.cpp             |  5 +++
>  src/libcamera/request.cpp            | 54 ++++++++++++++++++++++++++--
>  4 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> index 8c92a27a95e5..981ad184fffa 100644
> --- a/include/libcamera/internal/request.h
> +++ b/include/libcamera/internal/request.h
> @@ -59,6 +59,8 @@ private:
>  	std::unordered_set<FrameBuffer *> pending_;
>  	std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
>  	std::unique_ptr<Timer> timer_;
> +
> +	uint32_t canary_;
>  };
>
>  } /* namespace libcamera */
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index dffde1536cad..8e377de14b12 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -65,6 +65,8 @@ public:
>
>  	std::string toString() const;
>
> +	bool canary() const;
> +
>  private:
>  	LIBCAMERA_DISABLE_COPY(Request)
>
> @@ -74,6 +76,8 @@ private:
>
>  	const uint64_t cookie_;
>  	Status status_;
> +
> +	int32_t canary_;
>  };
>
>  std::ostream &operator<<(std::ostream &out, const Request &r);
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 48bf19b28979..3b72098cce59 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -1116,6 +1116,11 @@ int Camera::queueRequest(Request *request)
>  {
>  	Private *const d = _d();
>
> +	if (request->canary()) {
> +		LOG(Camera, Error) << "Invalid request";
> +		return -EINVAL;
> +	}
> +
>  	int ret = d->isAccessAllowed(Private::CameraRunning);
>  	if (ret < 0)
>  		return ret;
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 949c556fa437..cfe732765f86 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -23,6 +23,8 @@
>  #include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/tracepoints.h"
>
> +#define REQUEST_CANARY 0x1F2E3D4C
> +
>  /**
>   * \file libcamera/request.h
>   * \brief Describes a frame capture request to be processed by a camera
> @@ -48,13 +50,14 @@ LOG_DEFINE_CATEGORY(Request)
>   * \param camera The Camera that creates the request
>   */
>  Request::Private::Private(Camera *camera)
> -	: camera_(camera), cancelled_(false)
> +	: camera_(camera), cancelled_(false), canary_(REQUEST_CANARY)
>  {
>  }
>
>  Request::Private::~Private()
>  {
>  	doCancelRequest();
> +	canary_ = 0;
>  }
>
>  /**
> @@ -114,6 +117,7 @@ void Request::Private::complete()
>  {
>  	Request *request = _o<Request>();
>
> +	ASSERT(canary_ == REQUEST_CANARY);
>  	ASSERT(request->status() == RequestPending);
>  	ASSERT(!hasPendingBuffers());
>
> @@ -128,6 +132,8 @@ void Request::Private::doCancelRequest()
>  {
>  	Request *request = _o<Request>();
>
> +	ASSERT(canary_ == REQUEST_CANARY);
> +
>  	for (FrameBuffer *buffer : pending_) {
>  		buffer->_d()->cancel();
>  		camera_->bufferCompleted.emit(request, buffer);
> @@ -149,6 +155,8 @@ void Request::Private::doCancelRequest()
>   */
>  void Request::Private::cancel()
>  {
> +	ASSERT(canary_ == REQUEST_CANARY);
> +
>  	LIBCAMERA_TRACEPOINT(request_cancel, this);
>
>  	Request *request = _o<Request>();
> @@ -346,7 +354,7 @@ void Request::Private::timeout()
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
>  	: Extensible(std::make_unique<Private>(camera)),
> -	  cookie_(cookie), status_(RequestPending)
> +	  cookie_(cookie), status_(RequestPending), canary_(REQUEST_CANARY)
>  {
>  	controls_ = new ControlList(controls::controls,
>  				    camera->_d()->validator());
> @@ -367,6 +375,8 @@ Request::~Request()
>
>  	delete metadata_;
>  	delete controls_;
> +
> +	canary_ = 0;
>  }
>
>  /**
> @@ -381,6 +391,11 @@ Request::~Request()
>   */
>  void Request::reuse(ReuseFlag flags)
>  {
> +	if (canary_ != REQUEST_CANARY) {
> +		LOG(Request, Error) << "Invalid Request object";
> +		return;
> +	}
> +
>  	LIBCAMERA_TRACEPOINT(request_reuse, this);
>
>  	_d()->reset();
> @@ -462,6 +477,11 @@ void Request::reuse(ReuseFlag flags)
>  int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,
>  		       std::unique_ptr<Fence> fence)
>  {
> +	if (canary_ != REQUEST_CANARY) {
> +		LOG(Request, Error) << "Attempt to add buffer to invalid request";
> +		return -EINVAL;
> +	}
> +
>  	if (!stream) {
>  		LOG(Request, Error) << "Invalid stream reference";
>  		return -EINVAL;
> @@ -509,6 +529,11 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer,
>   */
>  FrameBuffer *Request::findBuffer(const Stream *stream) const
>  {
> +	if (canary_ != REQUEST_CANARY) {
> +		LOG(Request, Error) << "Invalid Request object";
> +		return nullptr;
> +	}
> +
>  	const auto it = bufferMap_.find(stream);
>  	if (it == bufferMap_.end())
>  		return nullptr;
> @@ -571,6 +596,7 @@ uint32_t Request::sequence() const
>   */
>  bool Request::hasPendingBuffers() const
>  {
> +	ASSERT(canary_ == REQUEST_CANARY);
>  	return !_d()->pending_.empty();
>  }
>
> @@ -590,6 +616,25 @@ std::string Request::toString() const
>  	return ss.str();
>  }
>
> +/**
> + * \brief Identify if the Request object is valid and alive
> + *
> + * This provides a means of checking if the request is a valid request object.
> + * While Requests are constructed by libcamera, they are owned and may be freed
> + * by applications. It can be all to easy too release a Request object while it
> + * is still in use by libcamera - or attempt to requeue invalid or deleted
> + * requests.
> + *
> + * The canary provides an insight that the object is not valid and shall be
> + * rejected by libcamera API calls.
> + *
> + * \return True if the canary has died, and the object shall not be trusted
> + */
> +bool Request::canary() const
> +{
> +	return canary_ != REQUEST_CANARY;
> +}
> +
>  /**
>   * \brief Insert a text representation of a Request into an output stream
>   * \param[in] out The output stream
> @@ -601,6 +646,11 @@ std::ostream &operator<<(std::ostream &out, const Request &r)
>  	/* Pending, Completed, Cancelled(X). */
>  	static const char *statuses = "PCX";
>
> +	if (r.canary()) {
> +		out << "Request(Invalid Canary)";
> +		return out;
> +	}
> +
>  	/* Example Output: Request(55:P:1/2:6523524) */
>  	out << "Request(" << r.sequence() << ":" << statuses[r.status()] << ":"
>  	    << r._d()->pending_.size() << "/" << r.buffers().size() << ":"
> --
> 2.34.1
>