[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR 8144445: Maximum size checking in Marlin ArrayCache utility methods is not 
From:       Philip Race <philip.race () oracle ! com>
Date:       2015-12-10 23:38:47
Message-ID: 566A0D07.50000 () oracle ! com
[Download RAW message or body]

+1

-phil.

On 12/9/15, 2:08 PM, Jim Graham wrote:
> 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
[prev in list] [next in list] [prev in thread] [next in thread]