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