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

[Kieran Bingham]

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 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 ?

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>                 return 0;
>  
>         /*
> -- 
> 2.34.1
>