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

[Jim Graham]

Hi Laurent,

On 12/7/15 2:17 AM, Laurent Bourgès wrote:
>
> 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
> <mailto: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

My point is that needSize is checked on the following line so why is it 
also checked here?  The entanglement of the size and needSize tests on 
214 and 215 is not straightforward and worrisome.

In looking this over again I notice that really the exceptions are only 
thrown on "out of range" needSize values and they are probably always 
thrown in that case, but sometimes we only discover those range issues 
if other things fail and many of the tests that lead to getting to that 
statement may or may not operate correctly depending on whether they are 
dealing with a valid needSize (for instance testing against "< needSize" 
when needSize might be negative, etc.).  It makes verifying this code 
much more difficult due to having to deal with cases where multiple 
variables might be out of range when they are computed or compared 
against each other.  To that end, it might be more straightforward to 
simply test needSize at the top and then the rest of the function can 
know it is dealing with a proper value for needSize and only has to 
worry about returning a non-overflowing number.  Knowing that needSize 
is a valid number makes any computations with it or compares against it 
have fewer "failure conditions" to note and vet.

For example:

first function:

185-189 move to the top of the function and only test needSize and then 
later on line 177 we capture any overflow since we know that needSize 
cannot be negative as well.  180,182 are then sufficient to make sure 
that the value calculated in that case will be >= needSize.

second function:

215-220 also move to the top of that function and 214 (if it compares 
size in both cases) is sufficient to make sure we are returning a value 
 >= needSize in all cases.  As it stands 210 and 212 could be computing 
random values and the tests at 214 and later are no longer complicated 
by having to consider the case that needSize itself is wrong - they only 
have to deal with whether the returned size bumped out of range.

general note:

Also, "(longval < 0L || longval > MAX_INT)" can be calculated as 
"((longval >> 31) != 0)".

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

That may be true at line 209, but then you test it against needSize and 
do more calculations where the new value of size depends only on 
needSize and so its dependence on curSize no longer offers any 
protection.  At 214 you might not be able to assert that size>=0 any 
more as a result.

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

That works.  I was thinking more along the lines of this which is more 
straightforward:

try {
     do test;
     throw new RuntimeException("AIOBException not thrown");
} catch (AIOBException e) (
     return;
} catch (Throwable t) {
     throw new RuntimeException("Wrong exception (not AIOB) thrown", t);
}

			...jim