get_names method in asnobj_str.py uses a set, hence the decoding order is not consistent.

[dbressan2]

Hi

I am using Python 3.13 and Pycrate 0.7.8

I would have a suggestion for the following method from asnobj_str.py:

def get_names(self):
    """Returns the set of names from the NamedBitList corresponding to the 
    internal value currently set
    """
    if isinstance(self._val, set):
        return self._val
    names = set()
    if self._cont is None or not isinstance(self._val, tuple) \
    or len(self._val) != 2 or not isinstance(self._val[0], integer_types):
        # self._val has not the correct format
        return names
    for off, bit in enumerate(uint_to_bitstr(self._val[0], self._val[1])):
        if bit == '1' and off in self._cont_rev:
            names.add(self._cont_rev[off])
    return names

get_names returns a Python set. Unlike a list or a Python dictionary (from Python 3.7 onward), a Python set is not ordered. This causes 2 problems:

1. the decoding order may not match the ASN.1 description from the standard.
2. more importantly, the decoding result can vary from run to run, with the ordering of the names changing. This prevents us from writing non-regression test cases with unittest where the decoding result is expected to be strictly equal to a stored value.

I have modified the code from get_names and _safechk_val from asnobj_str.py to use list instead of set. This way, the decoding results are fully predictable. Would it be possible to do a similar change ? Is there any good reason why set should be used for get_names ?

Thank you in advance Best regards.

[mitshell]

Can't remember the good reason to return a set instead of a list (this was done 8 years ago), but I guess there was one. Looking at the code right now, I agree returning a list could make more sense, even if you generally don't need to know the offset or order of your flags (what named bits are generally used for). If you want to provide a PR, while ensuring all the test cases keep passing, I'll be glad to review it.

[mitshell]

I implemented the change in https://github.com/pycrate-org/pycrate/commit/91b7713c7a53a9b3c1ee2cf58dc5fc79a066a205. Let me know if this fits what you were expecting @dbressan2?

[dbressan2]

Hi Thank you for the fix proposal. It did not work for me. I fixed it in a different way. In asnobj_str.py, I modified 2 methods so that names is a list instead of a set

def _safechk_val(self, val):
    if isinstance(val, tuple) and len(val) == 2:
        if isinstance(val[0], integer_types):
            # raw value
            if not isinstance(val[1], integer_types):
                raise(ASN1ObjErr('{0}: invalid value, {1!r}'.format(self.fullname(), val)))
        else:
            # CONTAINING value
            self._get_val_obj(val[0])._safechk_val(val[1])
    elif isinstance(val, set) or isinstance(val, list):
        # named bits
        if not self._cont:
            raise(ASN1ObjErr('{0}: invalid named bits, {1!r}'.format(self.fullname(), val)))
        elif any([nb not in self._cont for nb in val]):
            raise(ASN1ObjErr('{0}: invalid named bits, {1!r}'.format(self.fullname(), val)))
    else:
        raise(ASN1ObjErr('{0}: invalid value, {1!r}'.format(self.fullname(), val)))
        

 def get_names(self):
    """Returns the set of names from the NamedBitList corresponding to the 
    internal value currently set
    """
    if isinstance(self._val, set):
        return self._val
    names = []
    if self._cont is None or not isinstance(self._val, tuple) \
    or len(self._val) != 2 or not isinstance(self._val[0], integer_types):
        # self._val has not the correct format
        return names
    for off, bit in enumerate(uint_to_bitstr(self._val[0], self._val[1])):
        if bit == '1' and off in self._cont_rev:
            names.append(self._cont_rev[off])
    return names 

[mitshell]

Sorry, I copy/pasted the wrong commit ref. Here is the fix: https://github.com/pycrate-org/pycrate/commit/3e11fabaa894c03eb5863b30a42b1de01184d1ea

[dbressan2]

OK, the fix works perfectly for me. Thank you very much for taking the request into account.