[libcamera-devel] [PATCH 11/30] cam: options: Avoid copies of OptionvValue and KeyValueParser::Options

[Kieran Bingham]

On 07/07/2021 03:19, 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.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/cam/options.cpp | 22 ++++++++++------------
>  src/cam/options.h   |  4 ++--
>  2 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 319fab6143c5..9de3dd77fd15 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -632,27 +632,25 @@ 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();

I guess you could have a static empty list here to return a reference
of, but if it's not supposed to happen - perhaps it doesn't matter.

But should you at least put in an ASSERT() in here?


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

Same comment, if we're not going to return a static local - I'd put in
an assert to be sure.


> -
>  	return array_;
>  }
>  
> diff --git a/src/cam/options.h b/src/cam/options.h
> index 210e502a24e1..09cbc8339dd0 100644
> --- a/src/cam/options.h
> +++ b/src/cam/options.h
> @@ -141,8 +141,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;
>  
>