| Message ID | 20210707021941.20804-12-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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; > >
Hi Kieran, On Mon, Jul 12, 2021 at 04:10:47PM +0100, Kieran Bingham wrote: > 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@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. Note that if type_ != ValueKeyValue, the keyValues_ member will be default-initialized, so the function will return an empty list. Same for the array case below. > But should you at least put in an ASSERT() in here? I'll do that. > > - > > 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; > >
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(); - 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>{}; - 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> --- src/cam/options.cpp | 22 ++++++++++------------ src/cam/options.h | 4 ++-- 2 files changed, 12 insertions(+), 14 deletions(-)