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.gra...@oracle.com
<mailto:james.gra...@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
