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

[Dmitry Samersoff]

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.

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.