[Freeipa-devel] [PATCH] 275 Do not crash in Decimal parameter conversion

[Rob Crittenden]

Martin Kosek wrote:
> On Thu, 2012-06-14 at 09:05 -0400, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On Wed, 2012-06-13 at 18:05 -0400, Rob Crittenden wrote:
>>>> Martin Kosek wrote:
>>>>> On Thu, 2012-06-07 at 22:38 -0400, Rob Crittenden wrote:
>>>>>> Martin Kosek wrote:
>>>>>>> When invalid data is passed, an unhandled decimal exception could
>>>>>>> be raised in Decimal number conversion. Handle the exception
>>>>>>> more gracefully and report proper ipalib.errors.ConversionError.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/2705
>>>>>>
>>>>>> I'm being pedantic but I think the Decimal special values need to be
>>>>>> handled better. Using Infinity returns a rather odd message:
>>>>>>
>>>>>> $ ipa dnsrecord-add example.com
>>>>>> Record name: foo
>>>>>> Please choose a type of DNS resource record to be added
>>>>>> The most common types for this type of zone are: A, AAAA
>>>>>>
>>>>>> DNS resource record type: LOC
>>>>>> LOC Degrees Latitude: 90
>>>>>> [LOC Minutes Latitude]: 59
>>>>>> [LOC Seconds Latitude]:
>>>>>> 999999999999999999999999999999999999999999999999999999999999999999999
>>>>>>     >>>    LOC Seconds Latitude: quantize result has too many digits for
>>>>>> current context
>>>>>> [LOC Seconds Latitude]: Infinity
>>>>>>     >>>    LOC Seconds Latitude: quantize with one INF
>>>>>>
>>>>>> And using NaN raises an unhandled exception:
>>>>>>
>>>>>> [LOC Seconds Latitude]: NaN
>>>>>> ipa: ERROR: InvalidOperation: comparison involving NaN
>>>>>> Traceback (most recent call last):
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/cli.py", line 1263, in run
>>>>>>         sys.exit(api.Backend.cli.run(argv))
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/cli.py", line 1049, in run
>>>>>>         kw = self.argv_to_keyword_arguments(cmd, argv[1:])
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/cli.py", line 1035, in
>>>>>> argv_to_keyword_arguments
>>>>>>         self.prompt_interactively(cmd, kw)
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/cli.py", line 1199, in
>>>>>> prompt_interactively
>>>>>>         callback(kw)
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/plugins/dns.py", line 2147,
>>>>>> in interactive_prompt_callback
>>>>>>         user_options = param.prompt_parts(self.Backend)
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/plugins/dns.py", line 768, in
>>>>>> prompt_parts
>>>>>>         self.__get_part_param(backend, part, user_options, default)
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/plugins/dns.py", line 747, in
>>>>>> __get_part_param
>>>>>>         output_kw[name] = part(raw)
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/parameters.py", line 556, in
>>>>>> __call__
>>>>>>         self.validate(value, supplied=self.name in kw)
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/parameters.py", line 879, in
>>>>>> validate
>>>>>>         self._validate_scalar(value)
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/parameters.py", line 893, in
>>>>>> _validate_scalar
>>>>>>         error = rule(ugettext, value)
>>>>>>       File "/home/rcrit/redhat/freeipa/ipalib/parameters.py", line 1244, in
>>>>>> _rule_minvalue
>>>>>>         if value<    self.minvalue:
>>>>>>       File "/usr/lib64/python2.7/decimal.py", line 884, in __lt__
>>>>>>         ans = self._compare_check_nans(other, context)
>>>>>>       File "/usr/lib64/python2.7/decimal.py", line 786, in _compare_check_nans
>>>>>>         self)
>>>>>>       File "/usr/lib64/python2.7/decimal.py", line 3866, in _raise_error
>>>>>>         raise error(explanation)
>>>>>> InvalidOperation: comparison involving NaN
>>>>>> ipa: ERROR: an internal error has occurred
>>>>>>
>>>>>> Otherwise it does what it should.
>>>>>>
>>>>>> rob
>>>>>
>>>>> Thanks for being pedantic, I found out that Decimal number validation
>>>>> and normalization needs more care, dnsrecord-add would also fail with
>>>>> values such as "1E4" or "-0".
>>>>>
>>>>> Attached patch improves Decimal number validation a lot and adds
>>>>> optional exponent normalization. I also added missing tests for all
>>>>> Decimal Parameter attributes.
>>>>>
>>>>> Martin
>>>>
>>>> Getting some lint errors. Ran out of time to investigate but its strange
>>>> because AFAICT these are members.
>>>>
>>>> ipalib/parameters.py:1266: [E1101, Decimal._enforce_numberclass]
>>>> Instance of 'Decimal' has no 'numberclass' member
>>>> ipalib/parameters.py:1271: [E1101, Decimal._enforce_numberclass]
>>>> Instance of 'Decimal' has no 'numberclass' member
>>>> ipalib/parameters.py:1288: [E1101, Decimal._remove_exponent] Instance of
>>>> 'Decimal' has no 'exponential' member
>>>>
>>>
>>> I assume you run this lint:
>>>
>>> # ./make-lint ipalib/parameters.py
>>>
>>> This produces a lot of false positive lint errors... But if you run lint
>>> for the entire project, my patches will pass:
>>>
>>> # git
>>> apply /home/mkosek/freeipa-mkosek-275-2-decimal-parameter-conversion-and-normalization.patch
>>> # ./make-lint
>>> #
>>>
>>> That's why 275-2 includes a change for make-lint to not report these new
>>> attributes:
>>> diff --git a/make-lint b/make-lint
>>> index 7ecd59d7e8c5a644f812d4b8987866e7d06236b5..30c5e00c1f0606c75ff1f7fec675ff673a6b87a0 100755
>>> --- a/make-lint
>>> +++ b/make-lint
>>> @@ -61,7 +61,8 @@ class IPATypeChecker(TypeChecker):
>>>                'csv', 'csv_separator', 'csv_skipspace'],
>>>            'ipalib.parameters.Bool': ['truths', 'falsehoods'],
>>>            'ipalib.parameters.Int': ['minvalue', 'maxvalue'],
>>> -        'ipalib.parameters.Decimal': ['minvalue', 'maxvalue', 'precision'],
>>> +        'ipalib.parameters.Decimal': ['minvalue', 'maxvalue', 'precision',
>>> +                                      'numberclass', 'exponential'],
>>>            'ipalib.parameters.Data': ['minlength', 'maxlength', 'length',
>>>                'pattern', 'pattern_errmsg'],
>>>            'ipalib.parameters.Enum': ['values'],
>>>
>>>
>>> Bottom line - I do not think there is a lint problem with my patch :-)
>>>
>>> Martin
>>>
>>
>> This was output from './make-lint'. It blew up when I was trying to
>> build the rpms.
>>
>> rob
>
> Hm, I guess you have a different version of pylint, mine is error-less
> (I use pylint-0.25.1-1.fc17.noarch).
>
> Anyway, I have disabled the false positive pylint errors you reported,
> it should not hurt and will keep your build clean. Updated patch is
> attached.
>
> Martin

ACK, pushed to master

rob