[libcamera-devel] [PATCH 1/4] cam: options: fix access to uninit variable

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Oct 07, 2020 at 12:22:36PM +0300, Tomi Valkeinen wrote:
> operator[] doesn't check if the option exists in the values_ map, so it
> can return a pointer to location outside the map.

Is this a problem that you've encountered in practice ?

OptionsBase here is used as a map-like container, and our implementation
of operator[] doesn't match std::map as the latter will insert an
element if the key isn't found. I wonder if this could create confusion
as to what behaviour a user of OptionsBase::operator[] would expect.

> Fix by returning an empty OptionValue if the option is not found.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at iki.fi>
> ---
>  src/cam/options.cpp | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 77b3cc1..8fc1c42 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -61,7 +61,12 @@ bool OptionsBase<T>::isSet(const T &opt) const
>  template<typename T>
>  const OptionValue &OptionsBase<T>::operator[](const T &opt) const
>  {
> -	return values_.find(opt)->second;
> +	static const OptionValue s_empty;

That should be sEmpty to match the coding style, but I'd just call it
empty as we don't prefix static variables with a prefix to mean they're
static (the same way we have no m_ prefix).

> +
> +	auto it = values_.find(opt);
> +	if (it != values_.end())
> +		return it->second;
> +	return s_empty;
>  }
>  
>  template<typename T>

-- 
Regards,

Laurent Pinchart