[libcamera-devel] [PATCH 8/8] [RFC-Only] libcamera: request: A request canary

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 15, 2021 at 11:24:45AM +0000, Kieran Bingham wrote:
> On 14/03/2021 02:13, Laurent Pinchart wrote:
> > On Fri, Mar 12, 2021 at 06:11:31AM +0000, Kieran Bingham wrote:
> >> 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?
> > 
> > I think we should move that check from the IPU3 pipeline handler to the
> > core, as commented on the corresponding patch, so this patch wouldn't
> > really be needed.
> > 
> >> 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_;
> > 
> > If we want to do this, couldn't we add a RequestFreed status to avoid
> > adding a separate field ?
> 
> It's tempting to add/handle this to the state machine indeed, but we are
> limited on the guarantees we can make there.
> 
> After a free - we can't guarantee that Request->status() == RequestFreed :-)

Can't we, if we set status_ to RequestFreed in the destructor ? If
you're concerned that someone would override the status, you could add
an assertion in setStatus().

> The point of the canary is to assert that it has not been free'd and
> that the values match exactly.
> 
> Maybe a RequestFreed state will add some protection though - but as this
> is RFC only - we can leave it to think about if we end up digging in
> here again.

Sounds good to me.

> >>  };
> >>  
> >>  } /* 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);

-- 
Regards,

Laurent Pinchart