Jim, Here is an updated webrev:
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> 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 size: it is possible in Renderer: final int edgePtr = _edges.used; ... final long edgeNewSize = ArrayCache.getNewLargeSize(_edges.length, edgePtr + _SIZEOF_EDGE_BYTES); ( edgePtr + _SIZEOF_EDGE_BYTES ) is an integer so it can become negative ! I could also check size < 0L if you want but it is only possible if curSize < 0 (that should never happen) ! If you want, I could add few assertions. > Stroker.java line 1276 - "numCurves >= curveTypes.length" would also > work...? > Fixed. > > In the test cases that expect an exception, if no exception is thrown then > you pass the test. Is that right? > Fixed. > > On 12/3/15 1:23 PM, Laurent Bourgès wrote: > >> >> https://bugs.openjdk.java.net/browse/JDK-8144445 >> >> Please review that webrev that improves overflow checks in ArrayCache: >> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8144445.0/ >> >> Changes >> - check 2Gb overflow in both getNewSize() and getNewLargeSize() >> - check negative size / needSize (potential integer math overflow) >> - fixed Stroker to use substraction and avoid integer overflow >> - added ArrayCacheSizeTest class which now passes in jtreg >> >> Jim's previous comments: >> The change looks fine. >> Are the long modifiers (12L, 1L, etc) really needed on the shift >> parameters given that the first operand (needSize) is already a long? >> >> 202 size = ((needSize >> 12L) + 1L) << 12L; >> >> I prefer keeping the long modifiers to be more explicit. >> >> Regards, >> Laurent >> >
