[libcamera-devel] [PATCH 2/2] libcamera: request: A request canary
Paul Elder
paul.elder at ideasonboard.com
Tue Feb 7 11:11:30 CET 2023
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.
>
> 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_;
It's unsigned here...
> };
>
> } /* 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_;
...while it's signed here :D
Other than that, I think the canary is a good thing to have.
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> };
>
> 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
>
More information about the libcamera-devel
mailing list