Re: [patch net-next] tools: ynl: fix attr_space variable to exist even if processing unknown attribute

[Jiri Pirko <jiri@resnulli.us>]

Thu, Feb 15, 2024 at 11:59:08AM CET, pabeni@redhat.com wrote:
>On Tue, 2024-02-13 at 08:04 +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> If message contains unknown attribute and user passes
>> "--process-unknown" command line option, _decode() gets called with space
>> arg set to None. In that case, attr_space variable is not initialized
>> used which leads to following trace:
>> 
>> Traceback (most recent call last):
>>   File "./tools/net/ynl/cli.py", line 77, in <module>
>>     main()
>>   File "./tools/net/ynl/cli.py", line 68, in main
>>     reply = ynl.dump(args.dump, attrs)
>>             ^^^^^^^^^^^^^^^^^^^^^^^^^^
>>   File "tools/net/ynl/lib/ynl.py", line 909, in dump
>>     return self._op(method, vals, [], dump=True)
>>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>   File "tools/net/ynl/lib/ynl.py", line 894, in _op
>>     rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name)
>>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>   File "tools/net/ynl/lib/ynl.py", line 639, in _decode
>>     self._rsp_add(rsp, attr_name, None, self._decode_unknown(attr))
>>                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^
>>   File "tools/net/ynl/lib/ynl.py", line 569, in _decode_unknown
>>     return self._decode(NlAttrs(attr.raw), None)
>>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>   File "tools/net/ynl/lib/ynl.py", line 630, in _decode
>>     search_attrs = SpaceAttrs(attr_space, rsp, outer_attrs)
>>                               ^^^^^^^^^^
>> UnboundLocalError: cannot access local variable 'attr_space' where it is not associated with a value
>> 
>> Fix this by setting attr_space to None in case space is arg None.
>> 
>> Fixes: bf8b832374fb ("tools/net/ynl: Support sub-messages in nested attribute spaces")
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>>  tools/net/ynl/lib/ynl.py | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
>> index 03c7ca6aaae9..b16d24b7e288 100644
>> --- a/tools/net/ynl/lib/ynl.py
>> +++ b/tools/net/ynl/lib/ynl.py
>> @@ -588,10 +588,12 @@ class YnlFamily(SpecFamily):
>>          revalue = search_attrs.lookup(selector)turn decoded
>>  
>>      def _decode(self, attrs, space, outer_attrs = None):
>> +        rsp = dict()
>>          if space:
>>              attr_space = self.attr_sets[space]
>> -        rsp = dict()
>> -        search_attrs = SpaceAttrs(attr_space, rsp, outer_attrs)
>> +            search_attrs = SpaceAttrs(attr_space, rsp, outer_attrs)
>> +        else:
>> +            search_attrs = None
>
>It looks like that later-on the code could call self._decode_sub_msg()
>-> self._resolve_selector() with search_attrs == None, and the latter
>will unconditionally do:
>
>	value = search_attrs.lookup(selector)

How exactly you can reach this? You won't get past:
            try:
                attr_spec = attr_space.attrs_by_val[attr.type]
            except (KeyError, UnboundLocalError):
                if not self.process_unknown:
                    raise Exception(f"Space '{space}' has no attribute with value '{attr.type}'")
                attr_name = f"UnknownAttr({attr.type})"
                self._rsp_add(rsp, attr_name, None, self._decode_unknown(attr))
                continue



>
>I think we need to explicitly handle the None value there.
>
>Thanks,
>
>Paolo
>