[libcamera-devel,v3,13/33] cam: options: Avoid copies of OptionvValue and KeyValueParser::Options
diff mbox series

Message ID 20210715211459.19373-14-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Multi-camera support in the cam application
Related show

Commit Message

Laurent Pinchart July 15, 2021, 9:14 p.m. UTC 
The OptionValue toKeyValues() and toArray() functions return a copy of
the values. This is unnecessary, and can cause use-after-free issues
when taking references to the return values. Return references instead
to optimize the implementation and avoid issues.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
Changes since v1:

- assert() on undefined behaviour
---
 src/cam/options.cpp | 24 ++++++++++++------------
 src/cam/options.h   |  4 ++--
 2 files changed, 14 insertions(+), 14 deletions(-)

Comments

Jacopo Mondi July 22, 2021, 12:54 p.m. UTC | #1 
Hi Laurent,

On Fri, Jul 16, 2021 at 12:14:39AM +0300, Laurent Pinchart wrote:
> The OptionValue toKeyValues() and toArray() functions return a copy of
> the values. This is unnecessary, and can cause use-after-free issues
> when taking references to the return values. Return references instead
> to optimize the implementation and avoid issues.

It might be good to briefly mention that error types are now rising an
assertion.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> Changes since v1:
>
> - assert() on undefined behaviour
> ---
>  src/cam/options.cpp | 24 ++++++++++++------------
>  src/cam/options.h   |  4 ++--
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 481ac189f115..6e0d802cb986 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -638,27 +638,27 @@ std::string OptionValue::toString() const
>
>  /**
>   * \brief Retrieve the value as a key-value list
> - * \return The option value as a KeyValueParser::Options, or an empty list if
> - * the value type isn't ValueType::ValueKeyValue
> + *
> + * The behaviour is undefined if the value type isn't ValueType::ValueKeyValue.
> + *
> + * \return The option value as a KeyValueParser::Options
>   */
> -KeyValueParser::Options OptionValue::toKeyValues() const
> +const KeyValueParser::Options &OptionValue::toKeyValues() const
>  {
> -	if (type_ != ValueKeyValue)
> -		return KeyValueParser::Options();
> -
> +	assert(type_ == ValueKeyValue);
>  	return keyValues_;
>  }
>
>  /**
>   * \brief Retrieve the value as an array
> - * \return The option value as a std::vector of OptionValue, or an empty vector
> - * if the value type isn't ValueType::ValueArray
> + *
> + * The behaviour is undefined if the value type isn't ValueType::ValueArray.
> + *
> + * \return The option value as a std::vector of OptionValue
>   */
> -std::vector<OptionValue> OptionValue::toArray() const
> +const std::vector<OptionValue> &OptionValue::toArray() const
>  {
> -	if (type_ != ValueArray)
> -		return std::vector<OptionValue>{};
> -
> +	assert(type_ == ValueArray);
>  	return array_;
>  }
>
> diff --git a/src/cam/options.h b/src/cam/options.h
> index 83c409ae4d28..0047b4f220ed 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -142,8 +142,8 @@ public:
>
>  	int toInteger() const;
>  	std::string toString() const;
> -	KeyValueParser::Options toKeyValues() const;
> -	std::vector<OptionValue> toArray() const;
> +	const KeyValueParser::Options &toKeyValues() const;
> +	const std::vector<OptionValue> &toArray() const;
>
>  	const OptionsParser::Options &children() const;
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart July 22, 2021, 1:22 p.m. UTC | #2 
Hi Jacopo,

On Thu, Jul 22, 2021 at 02:54:48PM +0200, Jacopo Mondi wrote:
> On Fri, Jul 16, 2021 at 12:14:39AM +0300, Laurent Pinchart wrote:
> > The OptionValue toKeyValues() and toArray() functions return a copy of
> > the values. This is unnecessary, and can cause use-after-free issues
> > when taking references to the return values. Return references instead
> > to optimize the implementation and avoid issues.
> 
> It might be good to briefly mention that error types are now rising an
> assertion.

Good point. I'll mention it in the commit message.

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - assert() on undefined behaviour
> > ---
> >  src/cam/options.cpp | 24 ++++++++++++------------
> >  src/cam/options.h   |  4 ++--
> >  2 files changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> > index 481ac189f115..6e0d802cb986 100644
> > --- a/src/cam/options.cpp
> > +++ b/src/cam/options.cpp
> > @@ -638,27 +638,27 @@ std::string OptionValue::toString() const
> >
> >  /**
> >   * \brief Retrieve the value as a key-value list
> > - * \return The option value as a KeyValueParser::Options, or an empty list if
> > - * the value type isn't ValueType::ValueKeyValue
> > + *
> > + * The behaviour is undefined if the value type isn't ValueType::ValueKeyValue.
> > + *
> > + * \return The option value as a KeyValueParser::Options
> >   */
> > -KeyValueParser::Options OptionValue::toKeyValues() const
> > +const KeyValueParser::Options &OptionValue::toKeyValues() const
> >  {
> > -	if (type_ != ValueKeyValue)
> > -		return KeyValueParser::Options();
> > -
> > +	assert(type_ == ValueKeyValue);
> >  	return keyValues_;
> >  }
> >
> >  /**
> >   * \brief Retrieve the value as an array
> > - * \return The option value as a std::vector of OptionValue, or an empty vector
> > - * if the value type isn't ValueType::ValueArray
> > + *
> > + * The behaviour is undefined if the value type isn't ValueType::ValueArray.
> > + *
> > + * \return The option value as a std::vector of OptionValue
> >   */
> > -std::vector<OptionValue> OptionValue::toArray() const
> > +const std::vector<OptionValue> &OptionValue::toArray() const
> >  {
> > -	if (type_ != ValueArray)
> > -		return std::vector<OptionValue>{};
> > -
> > +	assert(type_ == ValueArray);
> >  	return array_;
> >  }
> >
> > diff --git a/src/cam/options.h b/src/cam/options.h
> > index 83c409ae4d28..0047b4f220ed 100644
> > --- a/src/cam/options.h
> > +++ b/src/cam/options.h
> > @@ -142,8 +142,8 @@ public:
> >
> >  	int toInteger() const;
> >  	std::string toString() const;
> > -	KeyValueParser::Options toKeyValues() const;
> > -	std::vector<OptionValue> toArray() const;
> > +	const KeyValueParser::Options &toKeyValues() const;
> > +	const std::vector<OptionValue> &toArray() const;
> >
> >  	const OptionsParser::Options &children() const;
> >

Patch
diff mbox series 

diff --git a/src/cam/options.cpp b/src/cam/options.cpp
index 481ac189f115..6e0d802cb986 100644
--- a/src/cam/options.cpp
+++ b/src/cam/options.cpp
@@ -638,27 +638,27 @@  std::string OptionValue::toString() const
 
 /**
  * \brief Retrieve the value as a key-value list
- * \return The option value as a KeyValueParser::Options, or an empty list if
- * the value type isn't ValueType::ValueKeyValue
+ *
+ * The behaviour is undefined if the value type isn't ValueType::ValueKeyValue.
+ *
+ * \return The option value as a KeyValueParser::Options
  */
-KeyValueParser::Options OptionValue::toKeyValues() const
+const KeyValueParser::Options &OptionValue::toKeyValues() const
 {
-	if (type_ != ValueKeyValue)
-		return KeyValueParser::Options();
-
+	assert(type_ == ValueKeyValue);
 	return keyValues_;
 }
 
 /**
  * \brief Retrieve the value as an array
- * \return The option value as a std::vector of OptionValue, or an empty vector
- * if the value type isn't ValueType::ValueArray
+ *
+ * The behaviour is undefined if the value type isn't ValueType::ValueArray.
+ *
+ * \return The option value as a std::vector of OptionValue
  */
-std::vector<OptionValue> OptionValue::toArray() const
+const std::vector<OptionValue> &OptionValue::toArray() const
 {
-	if (type_ != ValueArray)
-		return std::vector<OptionValue>{};
-
+	assert(type_ == ValueArray);
 	return array_;
 }
 
diff --git a/src/cam/options.h b/src/cam/options.h
index 83c409ae4d28..0047b4f220ed 100644
--- a/src/cam/options.h
+++ b/src/cam/options.h
@@ -142,8 +142,8 @@  public:
 
 	int toInteger() const;
 	std::string toString() const;
-	KeyValueParser::Options toKeyValues() const;
-	std::vector<OptionValue> toArray() const;
+	const KeyValueParser::Options &toKeyValues() const;
+	const std::vector<OptionValue> &toArray() const;
 
 	const OptionsParser::Options &children() const;