[libcamera-devel] [PATCH 11/30] cam: options: Avoid copies of OptionvValue and KeyValueParser::Options
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 12 20:31:16 CEST 2021
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 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.
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;
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list