[v2] pipeline: rpi: Fix potential empty optional read
diff mbox series

Message ID 20250401131105.748536-1-barnabas.pocze@ideasonboard.com
State Accepted
Headers show
Series
  • [v2] pipeline: rpi: Fix potential empty optional read
Related show

Commit Message

Barnabás Pőcze April 1, 2025, 1:11 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

--
2.49.0

Comments

Naushir Patuck April 2, 2025, 9:04 a.m. UTC | #1 
Hi Barnabás,

Thank you for this fix

On Tue, 1 Apr 2025 at 14:11, Barnabás Pőcze
<barnabas.pocze@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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.

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
Barnabás Pőcze April 2, 2025, 9:12 a.m. UTC | #2 
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@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@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
Naushir Patuck April 2, 2025, 9:16 a.m. UTC | #3 
On Wed, 2 Apr 2025 at 10:12, Barnabás Pőcze
<barnabas.pocze@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@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@ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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@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
>

Patch
diff mbox series 

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") {
 		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;
 	}