[PATCH v2] pipeline: rpi: Fix potential empty optional read

[Barnabás Pőcze]

Hi


2025. 04. 02. 11:04 keltezéssel, Naushir Patuck írta:
> Hi Barnabás,
> 
> Thank you for this fix
> 
> On Tue, 1 Apr 2025 at 14:11, Barnabás Pőcze
> <barnabas.pocze at ideasonboard.com> wrote:
>>
>> If `!target`, then `*target` is undefined behaviour, so check if the optional
>> is empty when printing the error message. Simplify the check as well.
>>
>> Fixes: 6c71ee1f153051 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class")
>> Fixes: 841ef2b4bb08ba ("pipeline: rpi: Add support for Raspberry Pi 5")
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> ---
>> Changes in v2:
>>    * fix the same issue in pisp as well
>> ---
>>   src/libcamera/pipeline/rpi/pisp/pisp.cpp | 4 ++--
>>   src/libcamera/pipeline/rpi/vc4/vc4.cpp   | 4 ++--
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
>> index 42ca7c80b..91e7f4c94 100644
>> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
>> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
>> @@ -1350,9 +1350,9 @@ int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject>
>>          }
>>
>>          std::optional<std::string> target = (*root)["target"].get<std::string>();
>> -       if (!target || *target != "pisp") {
>> +       if (target != "pisp") {
> 
> I'm a bit curious about this change.
> 
> Should we keep the if (!target || ...) test?  If "target" key is not
> present in the yaml, the optional will be a nullopt, and we then deref
> for the string comparison.  Does this work out correctly by compiler
> magic, or is it undefined behavior?  Ditto for the change below.

Relational operators are all well-defined for `std::optional` in every state.
So `==`, `!=`, `<`, etc. can be safely used on an empty optional. Naturally,
an empty optional only compares equal to another empty optional, so if `target`
is empty, then `target != "pisp"` is true.

See https://en.cppreference.com/w/cpp/utility/optional/operator_cmp:

"""
template< class T, class U >
constexpr bool operator!=( const optional<T>& opt, const U& value );	(23) 	(since C++17)

[...]

21-33) Compares `opt` with a value. The values are compared (using the corresponding operator of T)
        only if `opt` contains a value. Otherwise, `opt` is considered less than `value`.
"""

Regards,
Barnabás Pőcze

> 
> Regards,
> Naush
> 
> 
>>                  LOG(RPI, Error) << "Unexpected target reported: expected \"pisp\", got "
>> -                               << *target;
>> +                               << (target ? target->c_str() : "(unknown)");
>>                  return -EINVAL;
>>          }
>>
>> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>> index fd8d84b14..fe910bdf2 100644
>> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
>> @@ -510,9 +510,9 @@ int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &
>>          }
>>
>>          std::optional<std::string> target = (*root)["target"].get<std::string>();
>> -       if (!target || *target != "bcm2835") {
>> +       if (target != "bcm2835") {
>>                  LOG(RPI, Error) << "Unexpected target reported: expected \"bcm2835\", got "
>> -                               << *target;
>> +                               << (target ? target->c_str() : "(unknown)");
>>                  return -EINVAL;
>>          }
>>
>> --
>> 2.49.0