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

[Tomi Valkeinen]

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

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

 Tomi