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

9988 diff mbox series

operator[] doesn't check if the option exists in the values_ map, so it
can return a pointer to location outside the map.

Fix by returning an empty OptionValue if the option is not found.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@iki.fi>
---
 src/cam/options.cpp | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Hi Tomi,

On 07/10/2020 10:22, 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.
> 
> Fix by returning an empty OptionValue if the option is not found.
> 

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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;
> +
> +	auto it = values_.find(opt);
> +	if (it != values_.end())
> +		return it->second;
> +	return s_empty;
>  }
>  
>  template<typename T>
>

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@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>

On 07/10/2020 16:25, Laurent Pinchart wrote:
> 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 ?

Yes, cam does e.g.:

roles = StreamKeyValueParser::roles(options_[OptStream]);

If OptStream is not set, roles() will get uninitialized data.

> 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@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).

Oh, right. Yeah, I really don't like libcamera's style here, but I'll try to remember =)

 Tomi