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

[Naushir Patuck]

On Wed, 2 Apr 2025 at 10:12, Barnabás Pőcze
<barnabas.pocze at ideasonboard.com> wrote:
>
> 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`.
> """

Thanks for clarifying!  Change looks good to me:

Reviewed-by: Naushir Patuck <naush at raspberrypi.com>


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