[PATCH] apps: cam: Fix colorSpace access crash in KMSSink::configure

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> Quoting Milan Zamazal (2025-03-14 13:30:18)
>> Hi Kieran,
>> 
>
>> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
>> 
>> > Quoting Milan Zamazal (2025-03-10 11:06:30)
>> >> cfg.colorSpace may be unset in KMSSink::configure, resulting in a crash
>> >> when it is accessed.  If cfg.colorSpace is unset, simply return, the
>> >> same way as when YcbcrEncoding is set to None.
>> >
>> > I think this is something that we should ensure is trapped by
>> > lc-compliance in fact.
>> >
>> > I believe pipeline handlers /must/ always set the correct colorSpace
>> > after validate - so it's incorrect for applications to ever hit an
>> > undefined color space ...
>> 
>> I'm not sure whether all the pipelines do that; at least `simple'
>> doesn't.  I can fix `simple' pipeline but maybe some others have the
>> problem too.
>
> That's why I think we need to do better on lc-complicance tests to
> ensure 'rules' are always validated.
>
>> > Of course crashing isn't nice either ... 
>> 
>> Let's have an assertion there then to still expose the problem while
>> crashing in a civilized way?
>
> In kmssink? I'm fine adding an assertion here - but what about the other
> sinks ? What about the other applciations. That's why 'this' should be
> validated for /all/ pipeline handlers.

Well, I posted a patch that sets colorSpace in the simple pipeline.  It
fixes the crash for me.

As for the assertion, I realized it wouldn't be a good idea to add it
now.  Since the crash seems to be environment dependent in some way,
adding the assertion might cause previously unmet crashes with pipelines
not setting colorSpace.

Up to you how to proceed with the rest.

> There should be a contract somewhere that states a pipeline handler
> /must/ fill in specific fields.
>
> I have an upcoming proposal that could let us add a validation layer to
> all runtimes too which could be another place to add runtime
> validations.
>
> Hopefully I'll get that out next week as an initial RFC.
>
>> > but is this occuring in SoftISP/simple pipeline handler ?
>> 
>> Yes.  It started happening to me once I reinstalled my development
>> system to Fedora 41.  I can't see any obvious reason why it crashes now
>> and not before (maybe some change in gcc? -- using 14.2.1).
>> 
>
> It will be curious if this only appears as a result of changing external
> tooling though !

It looks compiler version dependent.  It doesn't crash on my Debix but
when I add an assertion there then it fails.  So the bug was there but
it got exposed to me only in current Fedora.

>> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.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 672c985a..aa9459cf 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 ||
>> >> +           cfg.colorSpace->ycbcrEncoding == libcamera::ColorSpace::YcbcrEncoding::None)
>> >>                 return 0;
>> >>  
>> >>         /*
>> >> -- 
>> >> 2.48.1
>> >>
>>