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
