RFR: 8159695: Arguments::atojulong() fails to detect overflows

[Marcus Larsson]

Hi,


On 06/29/2016 07:24 AM, David Holmes wrote:
> On 28/06/2016 10:29 PM, Dmitry Samersoff wrote:
>> David,
>>
>>>    n = strtoull(s, &remainder, (is_hex ? 16 : 10));
>>
>> n is ULLONG_MAX in case of overflow. Value ULLONG_MAX is not usable in
>> context of hotspot parameter so we may save couple of lines of code  and
>> just check for ULLONG_MAX at ll. 602
>
> Okay I see your point.
>
>> But I'm OK with existing errno check if you prefer it.
>
> Saves Marcus changing things :)

That sounds great to me. :)

Thanks for reviewing!
Marcus

>
> Thanks,
> David
>
>> -Dmitry
>>
>>
>> On 2016-06-28 14:55, David Holmes wrote:
>>> Hi Dmitry,
>>>
>>> On 28/06/2016 9:08 PM, Dmitry Samersoff wrote:
>>>> David,
>>>>
>>>> strtoull sets errno in two cases: unsupported base and overflow. As 
>>>> soon
>>>> as we don't provide error message and just return false, errno 
>>>> check is
>>>> redundant.
>>>
>>> Not sure what you mean. This code returns false on overflow:
>>>
>>>    n = strtoull(s, &remainder, (is_hex ? 16 : 10));
>>>    if (errno != 0) {
>>>      return false;
>>>    }
>>>
>>> as we know the base is okay. So errno should be ERANGE as the only 
>>> option.
>>>
>>> Cheers,
>>> David
>>>
>>>> But I'm OK to leave the code as is.
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2016-06-28 07:22, David Holmes wrote:
>>>>> Hi Marcus,
>>>>>
>>>>> On 27/06/2016 10:42 PM, Marcus Larsson wrote:
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> On 06/27/2016 11:25 AM, Dmitry Samersoff wrote:
>>>>>>> Marcus,
>>>>>>>
>>>>>>> Looks good for me beside some nits.
>>>>>>>
>>>>>>> 597: It might be better to don't check for errno but check for
>>>>>>> LLONG_MAX
>>>>>>> at 602
>>>>>>
>>>>>> Actually strtoull can return 0 if it reads no value, so to properly
>>>>>> detect all errors we should check errno.
>>>>>
>>>>> I agree. We check errno all over the VM. It is a legitimate and 
>>>>> useful
>>>>> thing to do - even if we can detect the errors in other ways.
>>>>>
>>>>>>>
>>>>>>> 593: s[0] == '0' && ((s[1] | 0x20) == 'x') might be slightly more
>>>>>>> readable.
>>>>>>>
>>>>>>> 602  strlen call could be replaced with
>>>>>>>        (*reminder != 0 && *(reminder+1) != 0)
>>>>>>
>>>>>> I'm not sure I think this makes it more readable, but I can change
>>>>>> it if
>>>>>> you want. Anyone else have an opinion on this?
>>>>>
>>>>> I prefer what you currently have - it is clear and explicit.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Thanks,
>>>>>> Marcus
>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>> On 2016-06-27 11:35, Marcus Larsson wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review the following patch to detect overflows in
>>>>>>>> atojulong. The
>>>>>>>> patch uses strtoull instead of sscanf, and adds a unit test for
>>>>>>>> atojulong.
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~mlarsson/8159695/webrev.00/
>>>>>>>>
>>>>>>>> Issue:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8159695
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>> JPRT testset hotspot and included unit test through RBT.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Marcus
>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>