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

[David Holmes]

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

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