[libcamera-devel] [PATCH 2/2] libcamera: request: A request canary
Dorota Czaplejewicz
dorota.czaplejewicz at puri.sm
Tue Mar 14 11:01:18 CET 2023
On Mon, 13 Mar 2023 18:43:17 +0100
Dorota Czaplejewicz <dorota.czaplejewicz at puri.sm> wrote:
> On Mon, 30 Jan 2023 18:02:44 +0000
> Kieran Bingham <kieran.bingham at ideasonboard.com> 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>
> > Reviewed-by: Paul Elder <paul.elder 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() << ":"
>
>
> Hi,
>
> I ran into a version of this problem where some code refactored using a few smaller code stopped being correct when taken as a whole. This kind of a canary would have probably prevented that. A small reproducer is here:
>
> https://source.puri.sm/dorota.czaplejewicz/simplecam/-/commits/bug_time
>
> In this case, the problem is accessing the local reference to the request after the (same) owner just released it. While I prefer to steer people to not make mistakes in the first place (like I mentioned in https://bugs.libcamera.org/show_bug.cgi?id=186 ), I'm not familiar enough with C++ to know if such a use-after-free can be caught at compile time.
Thinking about this a bit harder, the problem can be avoided by breaking up the connection between the local reference, and the freed reference. It was already proposed to pass the request by value. The local value would be standalone, so it would be harder to free it.
Another way to solve it would be to notice that the same request object comes from two sources to the buggy handler: on one side it's accessible due to the vector which actually owns the object, and then again, libcamera passes its own reference to the handler. A solution would make it impossible to free the object unilaterally from the handler as long as the reference exists. This can be done by wrapping the owned Request in a shared_ptr, and keeping another shared reference in libcamera for as long as it's needed for signals. (Once that's done, the "owning" vector will probably be unnecessary, and this will end up looking very similar to the pass-by-value case).
Neither would make misuse impossible, but they would make it hard to do by accident.
--Dorota
>
> Cheers,
> Dorota
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230314/5d881a4c/attachment.sig>
More information about the libcamera-devel
mailing list