| Message ID | 20241217095137.132370-1-antoine.bouyer@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Antoine Bouyer (2024-12-17 09:51:37) > This patch fixes below crash, with styhead's gcc compiler (version 14.2.0), > which occurs when optional colorSpace parameter is not filled. I find https://cbea.ms/git-commit/ a good reference for writing clear commit messages. We prefer not to say things like "this patch fixes", but instead explain the problem and the solution. In this case, the problem is that the KMS sink does not correctly check the optional colorSpace parameter of the stream configuration before dereferencing it. The solution is to ensure that the std::optional colorSpace has a value before accessing it. And explaining that it was identified with the specific toolcahin and environement is all helpful additional information that could follow. I would also recommend shortening the subject line. The 50,72 chars mentioned in https://cbea.ms/git-commit/ is a good target, but not always achieveable especially with the tags in front. But it makes a big difference to the readability of the commit logs, and also the subject here becomes part of the release notes. """ apps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior """ could be something shorter like: """ apps: cam: kms_sink: Verify colorSpace definition before dereference """ > > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp > ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai > led. > Aborted (core dumped) > > As detailed in the "operator->" page: > (https://en.cppreference.com/w/cpp/utility/optional/operator*) > > "This operator does not check whether the optional contains a value!" > > Use has_value() as a fix to make sure this property exists and prevent crash. > Lets add this tag here too: Fixes: 6d7ddace5278 ("cam: Add KMS sink class") > Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com> > --- > src/apps/cam/kms_sink.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp > index 672c985..8f3b867 100644 > --- a/src/apps/cam/kms_sink.cpp > +++ b/src/apps/cam/kms_sink.cpp > @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > colorEncoding_ = std::nullopt; > colorRange_ = std::nullopt; > > - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > + if (!cfg.colorSpace.has_value() || > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) cfg.colorSpace is indeed a std::optional so this makes sense. I'm surprised this hasn't been hit before ? Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > return 0; > > /* > -- > 2.34.1 >
On Tue, Dec 17, 2024 at 10:42:50AM +0000, Kieran Bingham wrote: > Quoting Antoine Bouyer (2024-12-17 09:51:37) > > This patch fixes below crash, with styhead's gcc compiler (version 14.2.0), > > which occurs when optional colorSpace parameter is not filled. > > I find https://cbea.ms/git-commit/ a good reference for writing clear > commit messages. > > We prefer not to say things like "this patch fixes", but instead explain > the problem and the solution. > > In this case, the problem is that the KMS sink does not correctly > check the optional colorSpace parameter of the stream configuration > before dereferencing it. > > The solution is to ensure that the std::optional colorSpace has a value > before accessing it. > > And explaining that it was identified with the specific toolcahin and > environement is all helpful additional information that could follow. > > I would also recommend shortening the subject line. The 50,72 chars > mentioned in https://cbea.ms/git-commit/ is a good target, but not > always achieveable especially with the tags in front. But it makes a big > difference to the readability of the commit logs, and also the subject > here becomes part of the release notes. > > """ > apps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior > """ > > could be something shorter like: > > """ > apps: cam: kms_sink: Verify colorSpace definition before dereference > """ Maybe s/definition/presence/ > > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 > > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp > > ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai > > led. > > Aborted (core dumped) > > > > As detailed in the "operator->" page: > > (https://en.cppreference.com/w/cpp/utility/optional/operator*) > > > > "This operator does not check whether the optional contains a value!" > > > > Use has_value() as a fix to make sure this property exists and prevent crash. > > > > Lets add this tag here too: > > Fixes: 6d7ddace5278 ("cam: Add KMS sink class") > > > Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com> > > --- > > src/apps/cam/kms_sink.cpp | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp > > index 672c985..8f3b867 100644 > > --- a/src/apps/cam/kms_sink.cpp > > +++ b/src/apps/cam/kms_sink.cpp > > @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > colorEncoding_ = std::nullopt; > > colorRange_ = std::nullopt; > > > > - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > > + if (!cfg.colorSpace.has_value() || > > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > > cfg.colorSpace is indeed a std::optional so this makes sense. > > I'm surprised this hasn't been hit before ? The member is optional primarily to allow applications to get the default color space. We also allow pipeline handlers to leave it unset when they don't know what color space is produced, but that shouldn't be the rule. Actually, except for the simple pipeline handler (and possibly for UVC, I haven't checked), I would make it mandatory for pipeline handlers to return a valid color space. We could also require them to report ColorSpace::Raw when they don't know. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > return 0; > > > > /*
Quoting Laurent Pinchart (2024-12-17 12:08:40) > On Tue, Dec 17, 2024 at 10:42:50AM +0000, Kieran Bingham wrote: > > Quoting Antoine Bouyer (2024-12-17 09:51:37) > > > This patch fixes below crash, with styhead's gcc compiler (version 14.2.0), > > > which occurs when optional colorSpace parameter is not filled. Antoine, did you fix this in your pipeline handler in the end? Is this commit still applicable otherwise? Perhaps what we actually need is a check in lc-compliance to make sure the colorSpace is always defined by pipeline handlers. Ultimately I'd like to know how to close this patch - will we have a v2 - or do we reject this and instead supply a validation in the compliance tests? -- Kieran > > > > I find https://cbea.ms/git-commit/ a good reference for writing clear > > commit messages. > > > > We prefer not to say things like "this patch fixes", but instead explain > > the problem and the solution. > > > > In this case, the problem is that the KMS sink does not correctly > > check the optional colorSpace parameter of the stream configuration > > before dereferencing it. > > > > The solution is to ensure that the std::optional colorSpace has a value > > before accessing it. > > > > And explaining that it was identified with the specific toolcahin and > > environement is all helpful additional information that could follow. > > > > I would also recommend shortening the subject line. The 50,72 chars > > mentioned in https://cbea.ms/git-commit/ is a good target, but not > > always achieveable especially with the tags in front. But it makes a big > > difference to the readability of the commit logs, and also the subject > > here becomes part of the release notes. > > > > """ > > apps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior > > """ > > > > could be something shorter like: > > > > """ > > apps: cam: kms_sink: Verify colorSpace definition before dereference > > """ > > Maybe s/definition/presence/ > > > > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 > > > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp > > > ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai > > > led. > > > Aborted (core dumped) > > > > > > As detailed in the "operator->" page: > > > (https://en.cppreference.com/w/cpp/utility/optional/operator*) > > > > > > "This operator does not check whether the optional contains a value!" > > > > > > Use has_value() as a fix to make sure this property exists and prevent crash. > > > > > > > Lets add this tag here too: > > > > Fixes: 6d7ddace5278 ("cam: Add KMS sink class") > > > > > Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com> > > > --- > > > src/apps/cam/kms_sink.cpp | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp > > > index 672c985..8f3b867 100644 > > > --- a/src/apps/cam/kms_sink.cpp > > > +++ b/src/apps/cam/kms_sink.cpp > > > @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > colorEncoding_ = std::nullopt; > > > colorRange_ = std::nullopt; > > > > > > - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > > > + if (!cfg.colorSpace.has_value() || > > > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > > > > cfg.colorSpace is indeed a std::optional so this makes sense. > > > > I'm surprised this hasn't been hit before ? > > The member is optional primarily to allow applications to get the > default color space. We also allow pipeline handlers to leave it unset > when they don't know what color space is produced, but that shouldn't be > the rule. Actually, except for the simple pipeline handler (and possibly > for UVC, I haven't checked), I would make it mandatory for pipeline > handlers to return a valid color space. We could also require them to > report ColorSpace::Raw when they don't know. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > return 0; > > > > > > /* > > -- > Regards, > > Laurent Pinchart
Quoting Kieran Bingham (2025-06-27 09:21:20) > Quoting Laurent Pinchart (2024-12-17 12:08:40) > > On Tue, Dec 17, 2024 at 10:42:50AM +0000, Kieran Bingham wrote: > > > Quoting Antoine Bouyer (2024-12-17 09:51:37) > > > > This patch fixes below crash, with styhead's gcc compiler (version 14.2.0), > > > > which occurs when optional colorSpace parameter is not filled. > > Antoine, did you fix this in your pipeline handler in the end? > > Is this commit still applicable otherwise? > > Perhaps what we actually need is a check in lc-compliance to make sure > the colorSpace is always defined by pipeline handlers. > > Ultimately I'd like to know how to close this patch - will we have a v2 > - or do we reject this and instead supply a validation in the compliance > tests? > And now I work up through my mails I instantly see a v2. Sorry for the noise - I'll just mark this as superceeded. > -- > Kieran > > > > > > > > I find https://cbea.ms/git-commit/ a good reference for writing clear > > > commit messages. > > > > > > We prefer not to say things like "this patch fixes", but instead explain > > > the problem and the solution. > > > > > > In this case, the problem is that the KMS sink does not correctly > > > check the optional colorSpace parameter of the stream configuration > > > before dereferencing it. > > > > > > The solution is to ensure that the std::optional colorSpace has a value > > > before accessing it. > > > > > > And explaining that it was identified with the specific toolcahin and > > > environement is all helpful additional information that could follow. > > > > > > I would also recommend shortening the subject line. The 50,72 chars > > > mentioned in https://cbea.ms/git-commit/ is a good target, but not > > > always achieveable especially with the tags in front. But it makes a big > > > difference to the readability of the commit logs, and also the subject > > > here becomes part of the release notes. > > > > > > """ > > > apps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior > > > """ > > > > > > could be something shorter like: > > > > > > """ > > > apps: cam: kms_sink: Verify colorSpace definition before dereference > > > """ > > > > Maybe s/definition/presence/ > > > > > > /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 > > > > 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp > > > > ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai > > > > led. > > > > Aborted (core dumped) > > > > > > > > As detailed in the "operator->" page: > > > > (https://en.cppreference.com/w/cpp/utility/optional/operator*) > > > > > > > > "This operator does not check whether the optional contains a value!" > > > > > > > > Use has_value() as a fix to make sure this property exists and prevent crash. > > > > > > > > > > Lets add this tag here too: > > > > > > Fixes: 6d7ddace5278 ("cam: Add KMS sink class") > > > > > > > Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com> > > > > --- > > > > src/apps/cam/kms_sink.cpp | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp > > > > index 672c985..8f3b867 100644 > > > > --- a/src/apps/cam/kms_sink.cpp > > > > +++ b/src/apps/cam/kms_sink.cpp > > > > @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) > > > > colorEncoding_ = std::nullopt; > > > > colorRange_ = std::nullopt; > > > > > > > > - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > > > > + if (!cfg.colorSpace.has_value() || > > > > + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) > > > > > > cfg.colorSpace is indeed a std::optional so this makes sense. > > > > > > I'm surprised this hasn't been hit before ? > > > > The member is optional primarily to allow applications to get the > > default color space. We also allow pipeline handlers to leave it unset > > when they don't know what color space is produced, but that shouldn't be > > the rule. Actually, except for the simple pipeline handler (and possibly > > for UVC, I haven't checked), I would make it mandatory for pipeline > > handlers to return a valid color space. We could also require them to > > report ColorSpace::Raw when they don't know. > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > return 0; > > > > > > > > /* > > > > -- > > Regards, > > > > Laurent Pinchart
diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp index 672c985..8f3b867 100644 --- a/src/apps/cam/kms_sink.cpp +++ b/src/apps/cam/kms_sink.cpp @@ -153,7 +153,8 @@ int KMSSink::configure(const libcamera::CameraConfiguration &config) colorEncoding_ = std::nullopt; colorRange_ = std::nullopt; - if (cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) + if (!cfg.colorSpace.has_value() || + cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None) return 0; /*
This patch fixes below crash, with styhead's gcc compiler (version 14.2.0), which occurs when optional colorSpace parameter is not filled. /opt/fsl-imx-internal-xwayland/6.12-styhead/sysroots/armv8a-poky-linux/usr/include/c++/14.2.0/optional:48 2: constexpr const _Tp& std::_Optional_base_impl<_Tp, _Dp>::_M_get() const [with _Tp = libcamera::ColorSp ace; _Dp = std::_Optional_base<libcamera::ColorSpace, true, true>]: Assertion 'this->_M_is_engaged()' fai led. Aborted (core dumped) As detailed in the "operator->" page: (https://en.cppreference.com/w/cpp/utility/optional/operator*) "This operator does not check whether the optional contains a value!" Use has_value() as a fix to make sure this property exists and prevent crash. Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com> --- src/apps/cam/kms_sink.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)