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

[David Holmes]

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