[libcamera-devel] [PATCH v3 05/11] libcamera: request: A request canary

[Kieran Bingham]

Hi Laurent,

On 26/03/2021 02:51, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Thu, Mar 25, 2021 at 01:42:25PM +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>
>>
>> ---
>> I've removed the RFC from this, and I actually even more so believe this
>> is a good facility to provide on the Request object.
>>
>> Mostly because the Request object is the only object which is given to
>> libcamera from the application (yes, created by libcamera, but that's
>> separate), and is expected to be free'd by the application.
>>
>> If an application free's requests while they are still in use within
>> libcamera, the symptoms can be distinctly misleading and lead to rabbit
>> holes.
>>
>> Therefore, I think the Request object is the one place where extra
>> safety checking (in debug builds) is worth while.
> 
> Should the canary_ member be compiled out in non-debug builds ?

It could be. It's small so I don't think it's an expensive waste of
memory - but I don't expect it to be used in Release builds.

This is just to satisfy the assertions.


>> Yes, of course if an application free's a request while it's in use with
>> libcamera - then it's the applications fault, not ours - but because of
>> the nature of requests, this could be an easy trap to fall into - and I
>> don't want to find that reported as bugs in libcamera.
> 
> When Request will become Extensible, do you think the canary_ field
> could be moved to the Private class ? That would make me feel better
> about this feature :-)

Certainly, I'd have no issue with that, as long as we guarantee that
when the Request is released, so is the Private, but I think we can say
that's fine :-D

> Actually, maybe we could use the d_ pointer and o_ pointer to replace
> the canary, but checking the point to each other ?

Ah, I think I see what you mean. Yes, I think there is a possibility
there. As long as it doesn't then mean we try to access an invalid
unique_ptr and crash before we can ASSERT() ;-)

It may be that this then gives us a flexible method to apply the same
assertions on any public object (which is why we would use extensible in
the first place) and could add a layer of protection to other public
objects. Perhaps things like Buffers where the application might have
ownership, and could release them at the wrong time ...

I'll investigate when I get back to getting Request Extensible.


>> ---
>>  include/libcamera/request.h |  2 ++
>>  src/libcamera/request.cpp   | 18 ++++++++++++++++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>> index 4cf5ff3f7d3b..965ffa6b45b2 100644
>> --- a/include/libcamera/request.h
>> +++ b/include/libcamera/request.h
>> @@ -79,6 +79,8 @@ private:
>>  	const uint64_t cookie_;
>>  	Status status_;
>>  	bool cancelled_;
>> +
>> +	uint32_t canary_;
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>> index 7b7ef6814686..c4258480b12b 100644
>> --- a/src/libcamera/request.cpp
>> +++ b/src/libcamera/request.cpp
>> @@ -19,6 +19,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
>> @@ -90,6 +92,8 @@ Request::Request(Camera *camera, uint64_t cookie)
>>  
>>  	LIBCAMERA_TRACEPOINT(request_construct, this);
>>  
>> +	canary_ = REQUEST_CANARY;
> 
> You can move this to the member initializer list.
> 
>> +
>>  	LOG(Request, Debug) << "Created request - cookie: " << cookie_;
>>  }
>>  
>> @@ -100,6 +104,8 @@ Request::~Request()
>>  	delete metadata_;
>>  	delete controls_;
>>  	delete validator_;
>> +
>> +	canary_ = 0;
>>  }
>>  
>>  /**
>> @@ -114,6 +120,8 @@ Request::~Request()
>>   */
>>  void Request::reuse(ReuseFlag flags)
>>  {
>> +	ASSERT(canary_ == REQUEST_CANARY);
>> +
>>  	LIBCAMERA_TRACEPOINT(request_reuse, this);
>>  
>>  	pending_.clear();
>> @@ -179,6 +187,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;
>> @@ -214,6 +224,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;
>> @@ -281,6 +293,7 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
>>   */
>>  void Request::complete()
>>  {
>> +	ASSERT(canary_ == REQUEST_CANARY);
>>  	ASSERT(status_ == RequestPending);
>>  	ASSERT(!hasPendingBuffers());
>>  
>> @@ -306,6 +319,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);
>> @@ -326,6 +341,9 @@ std::string Request::toString() const
>>  	/* Pending, Completed, Cancelled(X). */
>>  	static const char *statuses = "PCX";
>>  
>> +	if (canary_ != REQUEST_CANARY)
>> +		return "Invalid Canary on Request";
> 
> As all bets are off in this case, should we ASSERT() to catch this
> condition as early as possible ?
> 
>> +
>>  	/* Example Output: Request(55:P:1/2:6523524) */
>>  	ss << "Request (" << sequence_ << ":" << statuses[status_] << ":"
>>  	   << pending_.size() << "/" << bufferMap_.size() << ":"
> 

-- 
Regards
--
Kieran