| Message ID | 20210712215645.30478-12-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 12/07/2021 22:56, 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@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 fda6d9764ac5..aa3035fdf364 100644 > --- a/src/cam/options.cpp > +++ b/src/cam/options.cpp > @@ -631,27 +631,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 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; > >
diff --git a/src/cam/options.cpp b/src/cam/options.cpp index fda6d9764ac5..aa3035fdf364 100644 --- a/src/cam/options.cpp +++ b/src/cam/options.cpp @@ -631,27 +631,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 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;
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> --- Changes since v1: - assert() on undefined behaviour --- src/cam/options.cpp | 24 ++++++++++++------------ src/cam/options.h | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-)