[OpenJDK 2D-Dev] RFR 8144445: Maximum size checking in Marlin ArrayCache utility methods is not optimal

[Laurent Bourgès]

Jim,

Here is an updated webrev:

http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144445.1/

See my comments below:
Le 5 déc. 2015 00:13, "Jim Graham" <james.graham at oracle.com> a écrit :

> ArrayCache.java line 214 - was that supposed to be size or needSize on
> that line?
>
 214         if (needSize < 0L || size > Integer.MAX_VALUE) {

I think it is correct:
- check size > MAX_INT to detect integer overflow
- check needSize is negative to detect integer overflow on the needed size:
it is possible in Renderer:

final int edgePtr = _edges.used;
...
final long edgeNewSize = ArrayCache.getNewLargeSize(_edges.length,
                                        edgePtr + _SIZEOF_EDGE_BYTES);

( edgePtr + _SIZEOF_EDGE_BYTES )  is an integer so it can become negative !

I could also check size < 0L if you want but it is only possible if curSize
< 0 (that should never happen) !

If you want, I could add few assertions.


> Stroker.java line 1276 - "numCurves >= curveTypes.length" would also
> work...?
>

Fixed.


>
> In the test cases that expect an exception, if no exception is thrown then
> you pass the test.  Is that right?
>

Fixed.


>
> On 12/3/15 1:23 PM, Laurent Bourgès wrote:
>
>>
>> https://bugs.openjdk.java.net/browse/JDK-8144445
>>
>> Please review that webrev that improves overflow checks in ArrayCache:
>> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144445.0/
>>
>> Changes
>> - check 2Gb overflow in both getNewSize() and getNewLargeSize()
>> - check negative size / needSize (potential integer math overflow)
>> - fixed Stroker to use substraction and avoid integer overflow
>> - added ArrayCacheSizeTest class which now passes in jtreg
>>
>> Jim's previous comments:
>> The change looks fine.
>> Are the long modifiers (12L, 1L, etc) really needed on the shift
>> parameters given that the first operand (needSize) is already a long?
>>
>>   202             size = ((needSize >> 12L) + 1L) << 12L;
>>
>> I prefer keeping the long modifiers to be more explicit.
>>
>> Regards,
>> Laurent
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20151207/2a0886f8/attachment.html>