| Message ID | 20210312061131.854849-9-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > }; > > } /* 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);
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@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); >
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@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);
Hi Laurent, On 15/03/2021 16:28, Laurent Pinchart wrote: > 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@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(). No, we simply can't trust status after the request has been freed. That's the point I was making above. Asserting in setStatus() is irrelevant because the case I'm referring to, the memory is not going to be changed through the Request interface. Note that the canary asserts a variable IS a single specific value while it's valid. Setting the Request->status_ == RequestFreed, means that we have to assert that it 'is not' that value. (Or we assert that it's one of the valid states of course, but that's an ever so slightly larger scope/risk of false positives). Once the object is free'd the memory may as well be considered volatile, and changeable to anything else. That memory is no longer owned. So then - this would merrilly pass along the way... Request R = new(Request); pipelinehandler->queueRequest(R); delete(R); /* This is not allowed, hence the assertion below */ MyObject = new(object); /* Same memory occupied by R is now MyObject */ (meanwhile... over in the pipeline handler) ASSERT(Request->status != RequestFreed); /* Passed because Request->status == random value in MyObject */ We can handle this by asserting that the request status is in a valid state of course, but that's slightly different. >> 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); >
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_; }; } /* 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);
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@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? 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(+)