[libcamera-devel] [PATCH 8/8] [RFC-Only] libcamera: request: A request canary
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 15 12:24:45 CET 2021
Hi Laurent,
On 14/03/2021 02:13, Laurent Pinchart wrote:
> Hi Kieran,
>
> Thank you for the patch.
>
> 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 :-)
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.
>> };
>>
>> } /* 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
--
Kieran
More information about the libcamera-devel
mailing list