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

[Jim Graham]

Looks good.

And verified that the test fails before the fix and passes after...

			...jim

On 12/9/15 1:38 PM, Laurent Bourgès wrote:
> Hi Jim,
>
> I took me some time to understand your detailled analysis and make a new
> webrev:
> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144445.2/
>
> I think I adopted all your change proposals, see below:
>
>     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.
>
>
> I followed your approach and I agree it is more clear.
>
>
>     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.
>
>
> Fixed.
>
>     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.
>
>
> Fixed.
>
>     general note:
>
>     Also, "(longval < 0L || longval > MAX_INT)" can be calculated as
>     "((longval >> 31) != 0)".
>
>
> Thanks for the tip: I use it now.
>
>         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.
>
>            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);
>     }
>
>
> Fixed.
>
> Cheers,
> Laurent