| Message ID | 20210325134231.1400051-8-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > 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 :-) Actually, maybe we could use the d_ pointer and o_ pointer to replace the canary, but checking the point to each other ? > --- > 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() << ":"
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@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() << ":" >
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@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 ? > >> 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 :-) > > Actually, maybe we could use the d_ pointer and o_ pointer to replace > the canary, but checking the point to each other ? This is awkward, as the d pointer is stored in a unique_ptr, so performing the validation would itself cause an out-of-bounds access/segfault. The canary is supposed to spot the break, not be the break. I've dropped this from this series for the moment to allow the rest to progress. >> --- >> 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. Done. > >> + >> 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 ? Perhaps, I had wanted a visual representation when it failed. But a stack trace is that as well ;-) > >> + >> /* Example Output: Request(55:P:1/2:6523524) */ >> ss << "Request (" << sequence_ << ":" << statuses[status_] << ":" >> << pending_.size() << "/" << bufferMap_.size() << ":" >
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; + 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"; + /* Example Output: Request(55:P:1/2:6523524) */ ss << "Request (" << sequence_ << ":" << statuses[status_] << ":" << pending_.size() << "/" << bufferMap_.size() << ":"
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> --- 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. 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. --- include/libcamera/request.h | 2 ++ src/libcamera/request.cpp | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+)