[libcamera-devel] [PATCH 8/8] [RFC-Only] 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>

---
RFC only, as I was going to dispose of this patch. We added it while
debugging the IPU3 stability, and it may prove useful to catch errors if
requests are used after they are released.

However this may be redundant as pipeline handlers should guarantee that
requests are fully completed when they stop(). Of course the IPU3 wasn't
meeting that requirement, and we do not have a specific assertion that
validates that requirement on all pipeline handlers. So perhaps this
canary might serve as a beneficial guard?

If not, at least posting it will mean it can be used in the future if it
comes up again, or the concept could be applied to other objects if
appropriate.

 include/libcamera/request.h |  2 ++
 src/libcamera/request.cpp   | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/libcamera/request.h b/include/libcamera/request.h
index 59d7f4bac0d2..fc56d63c8c67 100644
--- a/include/libcamera/request.h
+++ b/include/libcamera/request.h
@@ -78,6 +78,8 @@ private:
 	const uint64_t cookie_;
 	Status status_;
 	bool cancelled_;
+
+	int canary_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index 12c2e7d425f9..83169a11e1e5 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -18,6 +18,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
@@ -89,6 +91,8 @@ Request::Request(Camera *camera, uint64_t cookie)
 
 	LIBCAMERA_TRACEPOINT(request_construct, this);
 
+	canary_ = REQUEST_CANARY;
+
 	LOG(Request, Debug) << "Created request - cookie: " << cookie_;
 }
 
@@ -99,6 +103,8 @@ Request::~Request()
 	delete metadata_;
 	delete controls_;
 	delete validator_;
+
+	canary_ = 0;
 }
 
 /**
@@ -113,6 +119,8 @@ Request::~Request()
  */
 void Request::reuse(ReuseFlag flags)
 {
+	ASSERT(canary_ == REQUEST_CANARY);
+
 	LIBCAMERA_TRACEPOINT(request_reuse, this);
 
 	pending_.clear();
@@ -176,6 +184,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;
@@ -211,6 +221,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;
@@ -262,6 +274,7 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
  */
 void Request::complete()
 {
+	ASSERT(canary_ == REQUEST_CANARY);
 	ASSERT(status_ == RequestPending);
 	ASSERT(!hasPendingBuffers());
 
@@ -289,6 +302,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);
-- 
2.25.1