[v2,1/1] apps: cam: kms_sink: Verify colorSpace presence before dereference
diff mbox series

Message ID 20241217140238.215572-2-antoine.bouyer@nxp.com
State New
Headers show
Series
  • Verify colorSpace presence during kms_sink configuration
Related show

Commit Message

Antoine Bouyer Dec. 17, 2024, 2:02 p.m. UTC 
During configuration stage, kms_sink doesn't check optional colorSpace
parameter is valid while dereferencing it. Then it leads to a crash
with below signature if parameter is not defined:

  /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)

This behavior is confirmed in the "operator->" page:
 (https://en.cppreference.com/w/cpp/utility/optional/operator*)

"This operator does not check whether the optional contains a value!"

This crash is observed with styhead's gcc compiler (version 14.2.0),
while it was not with previous scarthgap's gcc compiler (version 13.2.0).

Use has_value() as a fix to make sure this property exists and prevent
crash.

Fixes: 6d7ddace5278 ("cam: Add KMS sink class")
Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/apps/cam/kms_sink.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Dec. 17, 2024, 4:23 p.m. UTC | #1 
Quoting Antoine Bouyer (2024-12-17 14:02:39)
> During configuration stage, kms_sink doesn't check optional colorSpace
> parameter is valid while dereferencing it. Then it leads to a crash
> with below signature if parameter is not defined:
> 
>   /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)
> 
> This behavior is confirmed in the "operator->" page:
>  (https://en.cppreference.com/w/cpp/utility/optional/operator*)
> 
> "This operator does not check whether the optional contains a value!"
> 
> This crash is observed with styhead's gcc compiler (version 14.2.0),
> while it was not with previous scarthgap's gcc compiler (version 13.2.0).
> 
> Use has_value() as a fix to make sure this property exists and prevent
> crash.
> 
> Fixes: 6d7ddace5278 ("cam: Add KMS sink class")
> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks for the updates that I suggested, however I'm weary that perhaps
Laurent was suggesting this /shouldn't/ be possible to happen and should
be fixed in the pipeline handler? (and perhaps enforced?, and maybe
guaranteed to be set to RAW if unknown?)

We could perhaps have a common path in the configuration that sets RAW
when the pipeline handler didn't set it ? Or maybe we enforce it must be
set by lc-compliance or such...



> ---
>  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)
>                 return 0;
>  
>         /*
> -- 
> 2.34.1
>
Laurent Pinchart Dec. 17, 2024, 4:46 p.m. UTC | #2 
On Tue, Dec 17, 2024 at 04:23:27PM +0000, Kieran Bingham wrote:
> Quoting Antoine Bouyer (2024-12-17 14:02:39)
> > During configuration stage, kms_sink doesn't check optional colorSpace
> > parameter is valid while dereferencing it. Then it leads to a crash
> > with below signature if parameter is not defined:
> > 
> >   /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)
> > 
> > This behavior is confirmed in the "operator->" page:
> >  (https://en.cppreference.com/w/cpp/utility/optional/operator*)
> > 
> > "This operator does not check whether the optional contains a value!"
> > 
> > This crash is observed with styhead's gcc compiler (version 14.2.0),
> > while it was not with previous scarthgap's gcc compiler (version 13.2.0).
> > 
> > Use has_value() as a fix to make sure this property exists and prevent
> > crash.
> > 
> > Fixes: 6d7ddace5278 ("cam: Add KMS sink class")
> > Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Thanks for the updates that I suggested, however I'm weary that perhaps
> Laurent was suggesting this /shouldn't/ be possible to happen and should
> be fixed in the pipeline handler? (and perhaps enforced?, and maybe
> guaranteed to be set to RAW if unknown?)
> 
> We could perhaps have a common path in the configuration that sets RAW
> when the pipeline handler didn't set it ? Or maybe we enforce it must be
> set by lc-compliance or such...

Let's see first, Antoine, which pipeline handler have you encountered
this issue with ?

> > ---
> >  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)
> >                 return 0;
> >  
> >         /*
Antoine Bouyer Dec. 17, 2024, 4:59 p.m. UTC | #3 
On 17/12/2024 17:46, Laurent Pinchart wrote:

> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
>
>
> On Tue, Dec 17, 2024 at 04:23:27PM +0000, Kieran Bingham wrote:
>> Quoting Antoine Bouyer (2024-12-17 14:02:39)
>>> During configuration stage, kms_sink doesn't check optional colorSpace
>>> parameter is valid while dereferencing it. Then it leads to a crash
>>> with below signature if parameter is not defined:
>>>
>>>    /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)
>>>
>>> This behavior is confirmed in the "operator->" page:
>>>   (https://en.cppreference.com/w/cpp/utility/optional/operator*)
>>>
>>> "This operator does not check whether the optional contains a value!"
>>>
>>> This crash is observed with styhead's gcc compiler (version 14.2.0),
>>> while it was not with previous scarthgap's gcc compiler (version 13.2.0).
>>>
>>> Use has_value() as a fix to make sure this property exists and prevent
>>> crash.
>>>
>>> Fixes: 6d7ddace5278 ("cam: Add KMS sink class")
>>> Signed-off-by: Antoine Bouyer<antoine.bouyer@nxp.com>
>>> Reviewed-by: Kieran Bingham<kieran.bingham@ideasonboard.com>
>> Thanks for the updates that I suggested, however I'm weary that perhaps
>> Laurent was suggesting this /shouldn't/ be possible to happen and should
>> be fixed in the pipeline handler? (and perhaps enforced?, and maybe
>> guaranteed to be set to RAW if unknown?)
>>
>> We could perhaps have a common path in the configuration that sets RAW
>> when the pipeline handler didn't set it ? Or maybe we enforce it must be
>> set by lc-compliance or such...
> Let's see first, Antoine, which pipeline handler have you encountered
> this issue with ?

This is with imx8-isi pipeline.
Actually I was thinking about such approach initially, then changed to has_value() fix
because of "optional" flag.

>
>>> ---
>>>   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)
>>>                  return 0;
>>>
>>>          /*
> --
> Regards,
>
> Laurent Pinchart

Best regards

Antoine

Patch
diff mbox series 

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;
 
 	/*