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

[Dmitry Samersoff]

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

But I'm OK with existing errno check if you prefer it.

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.