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

[Laurent Pinchart]

Hello,

On Mon, Jan 30, 2023 at 08:38:47PM +0100, Jacopo Mondi via libcamera-devel wrote:
> 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.
> 
> I would have sworn me moved Request in and out from the Camera..
> 
>         unique_ptr<Request> Camera::createRequest();
>         int Camera::queueRequest(unique_ptr<Request> r);
> 
> But we indeed pass it by referece...
> 
>         int Camera::queueRequest(Request *r);
> 
> I wonder
> 
> - Once a Request is queued and before it is signaled as complete,
>   should applications be able to access it ?

Possibly in read-only mode, but even that will likely be racy, and I
don't see any compelling use case right now.

> - Request are signaled as complete through a signal, I guess a signal
>   cannot transport a unique_ptr<> back ?

Correct. This is due to the fact that you can connect a signal to
multiple slots, which would break the ownership transfer requirements of
std::unique_ptr<>.

I'm more and more tempted to experiment with passing requests by value
(although we'll have a similar problem with framebuffers). It feels like
we're working around a badly designed API, we should fix the API
instead.

> > If the application free's the requests while they are still active

s/free's/fress/

> > 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

s/Further more/Furthermore/

> > 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_;

We're trying to move away from storing private data in the public class
:-S I really dislike exposing this to the public API, including the
public canary() function.

> >  };
> >
> >  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;

I'd make everything fatal, using ASSERT(). If the request passed to this
function has been freed, it's game over anyway. Same below.

> > +	}
> > +
> >  	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

Lower-case hex constants.

> > +
> >  /**
> >   * \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() << ":"

-- 
Regards,

Laurent Pinchart