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

18226 diff mbox series

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@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@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(-)

Hi Kieran

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 ?

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

> 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@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@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() << ":"
> --
> 2.34.1
>

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

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@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@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() << ":"

On Mon, 30 Jan 2023 18:02:44 +0000
Kieran Bingham <kieran.bingham@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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@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@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.

Cheers,
Dorota

On Mon, 13 Mar 2023 18:43:17 +0100
Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm> wrote:

> On Mon, 30 Jan 2023 18:02:44 +0000
> Kieran Bingham <kieran.bingham@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@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@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@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