On Fri, 21 May 2021 13:02:14 GMT, Jose Pereda <jper...@openjdk.org> wrote:

> This PR limits the `tableIndex` value, used by the LZWDecoder algorithm in 
> `GIFImageLoader2`, to avoid a potential AIOOB exception that happens on some 
> animated GIFs, to the maximum size of the tables used (4096). 
> 
> In some occasions loading an animated GIF like the one used in the included 
> test, doesn't throw such exception, because we `allow partially loaded 
> animated images` in `ImageStorage`, but only a few frames are loaded.
> 
> In theory, greater values of such index would operate over completely full 
> tables, so there is no need to add new values in this case, and therefore, 
> there is no risk in limiting the value to 4096.
> 
> This PR will prevent the exception and all the frames should load. The 
> included test passes now (and fails loading only 10 frames out of 44 without 
> the proposed fix).

The fix seems safe enough, although should have a second reviewer. The test 
will need changes.

modules/javafx.graphics/src/test/java/test/com/sun/javafx/iio/ImageStorageTest.java
 line 91:

> 89:     @Test
> 90:     public void testLoadAllFramesAnimatedGIF() throws 
> ImageStorageException {
> 91:         String path = 
> "https://upload.wikimedia.org/wikipedia/commons/2/2c/Rotating_earth_%28large%29.gif";;

Except for specific network tests we don't want to load anything from the 
internet during a run. For one thing this will fail behind a firewall (without 
setting a proxy, which we don't want to require). For another, the content 
could change out from under us.

The best solution would be to generate a synthetic image with properties that 
will cause it to fail (we can't use a preexisting image due to copyright 
issues).

-------------

Changes requested by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/513

Reply via email to