[PATCH] apps: cam: kms_sink: Verify colorSpace is defined before dereferencing to avoid undefined behavior

[Laurent Pinchart]

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 at 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 at ideasonboard.com>
> 
> >                 return 0;
> >  
> >         /*

-- 
Regards,

Laurent Pinchart