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

[Kieran Bingham]

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.

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.
---
 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;
+
 	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";
+
 	/* Example Output: Request(55:P:1/2:6523524) */
 	ss << "Request (" << sequence_ << ":" << statuses[status_] << ":"
 	   << pending_.size() << "/" << bufferMap_.size() << ":"
-- 
2.25.1