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

[Kieran Bingham]

Request objects are created and owned by the application, but are
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
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