From f2cb10aeac3c1dd628eb166fce3db86ccf7327f6 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 09:34:43 +0000 Subject: [PATCH 01/10] Remove unused imports --- .../com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java index 35ed04e..112f8b2 100644 --- a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java +++ b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java @@ -45,8 +45,6 @@ import picocli.CommandLine; import com.glencoesoftware.bioformats2raw.Converter; -import com.glencoesoftware.omero.zarr.ZarrPixelsService; -import com.glencoesoftware.omero.zarr.ZarrPixelBuffer; import loci.formats.FormatTools; import loci.formats.in.FakeReader; From 3638127a96c00f752b31303f97461ac7253ea6f0 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 11:35:46 +0000 Subject: [PATCH 02/10] Add test case for integer overflow --- .../omero/zarr/ZarrPixelBufferTest.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java index 112f8b2..9f3675b 100644 --- a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java +++ b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java @@ -41,6 +41,7 @@ import com.bc.zarr.ZarrArray; import com.bc.zarr.ZarrGroup; +import com.fasterxml.jackson.databind.ObjectMapper; import com.github.benmanes.caffeine.cache.Caffeine; import picocli.CommandLine; @@ -501,6 +502,36 @@ public void testGetTileLargerThanImage() } } + @Test + public void testIntegerOverflow() + throws IOException, InvalidRangeException { + int sizeT = 1; + int sizeC = 3; + int sizeZ = 1; + int sizeY = 1; + int sizeX = 1; + int resolutions = 1; + Pixels pixels = new Pixels( + null, null, sizeX, sizeY, sizeZ, sizeC, sizeT, "", null); + Path output = writeTestZarr( + sizeT, sizeC, sizeZ, sizeY, sizeX, "uint16", + resolutions); + + ObjectMapper mapper = new ObjectMapper(); + HashMap zArray = mapper.readValue( + Files.readAllBytes(output.resolve("0/0/.zarray")), + HashMap.class); + List shape = (List) zArray.get("shape"); + shape.set(3, 50000); + shape.set(4, 50000); + mapper.writeValue(output.resolve("0/0/.zarray").toFile(), zArray); + try (ZarrPixelBuffer zpbuf = + createPixelBuffer(pixels, output.resolve("0"), 32, 32)) { + PixelData pixelData = zpbuf.getTile(0, 0, 0, 0, 0, 50000, 50000); + Assert.assertNull(pixelData); + } + } + @Test public void testTileExceedsMax() throws IOException, InvalidRangeException { int sizeT = 1; From bcecc20c5cede4f7b4cb8c60d6e748247fbc6630 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 11:37:52 +0000 Subject: [PATCH 03/10] Add missing override annotation --- .../java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java index 3a8abe4..b13da1c 100644 --- a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java +++ b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java @@ -326,6 +326,7 @@ public void close() throws IOException { * Implemented as specified by {@link PixelBuffer} I/F. * @see PixelBuffer#checkBounds(Integer, Integer, Integer, Integer, Integer) */ + @Override public void checkBounds(Integer x, Integer y, Integer z, Integer c, Integer t) throws DimensionsOutOfBoundsException { From 1056d95b764417d3d1e90a37fac62812ff9748be Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 13:57:29 +0000 Subject: [PATCH 04/10] Update test name and add some documentation --- .../com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java index 9f3675b..e1c4357 100644 --- a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java +++ b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java @@ -503,7 +503,7 @@ public void testGetTileLargerThanImage() } @Test - public void testIntegerOverflow() + public void testTileIntegerOverflow() throws IOException, InvalidRangeException { int sizeT = 1; int sizeC = 3; @@ -517,6 +517,8 @@ public void testIntegerOverflow() sizeT, sizeC, sizeZ, sizeY, sizeX, "uint16", resolutions); + // Hack the .zarray so we can appear as though we have more data than + // we actually have written above. ObjectMapper mapper = new ObjectMapper(); HashMap zArray = mapper.readValue( Files.readAllBytes(output.resolve("0/0/.zarray")), From 756e356d278b5bc7e0f74968bd8204d43d1c5891 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 13:58:19 +0000 Subject: [PATCH 05/10] Expand test case to also check for min overflow --- .../omero/zarr/ZarrPixelBufferTest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java index e1c4357..1268208 100644 --- a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java +++ b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java @@ -534,8 +534,9 @@ public void testTileIntegerOverflow() } } - @Test - public void testTileExceedsMax() throws IOException, InvalidRangeException { + @Test(expected = DimensionsOutOfBoundsException.class) + public void testTileExceedsMinMax() + throws IOException, InvalidRangeException { int sizeT = 1; int sizeC = 3; int sizeZ = 1; @@ -550,8 +551,9 @@ public void testTileExceedsMax() throws IOException, InvalidRangeException { try (ZarrPixelBuffer zpbuf = createPixelBuffer(pixels, output.resolve("0"), 32, 32)) { - PixelData pixelData = zpbuf.getTile(0, 0, 0, 0, 0, 32, 33); - Assert.assertNull(pixelData); + Assert.assertNull(zpbuf.getTile(0, 0, 0, 0, 0, 32, 33)); + // Throws exception + zpbuf.getTile(0, 0, 0, -1, 0, 1, 1); } } From 8abbf8b7bc68d382dd769140231306a40e611bb6 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 14:00:21 +0000 Subject: [PATCH 06/10] Simplify read size checks when reading --- .../omero/zarr/ZarrPixelBuffer.java | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java index b13da1c..f7ec08b 100644 --- a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java +++ b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java @@ -138,7 +138,7 @@ public ZarrPixelBuffer(Pixels pixels, Path root, Integer maxPlaneWidth, int h = key.get(7); int[] shape = new int[] { 1, 1, 1, h, w }; byte[] innerBuffer = - new byte[length(shape) * getByteWidth()]; + new byte[(int) length(shape) * getByteWidth()]; setResolutionLevel(resolutionLevel); return getTileDirect(z, c, t, x, y, w, h, innerBuffer); }); @@ -181,25 +181,17 @@ public int getPixelsType() { * "https://numpy.org/doc/stable/reference/generated/numpy.shape.html"> * numpy.shape documentation */ - private int length(int[] shape) { - return IntStream.of(shape).reduce(1, Math::multiplyExact); + private long length(int[] shape) { + return IntStream.of(shape) + .mapToLong(a -> (long) a) + .reduce(1, Math::multiplyExact); } private void read(byte[] buffer, int[] shape, int[] offset) throws IOException { - Integer sizeX = shape[4]; - Integer sizeY = shape[3]; - if((sizeX * sizeY) > (maxPlaneWidth*maxPlaneHeight)) { - throw new IllegalArgumentException(String.format( - "Requested Region Size %d * %d > max plane size %d * %d", sizeX, - sizeY, maxPlaneWidth, maxPlaneHeight)); - } - if (sizeX < 0) { - throw new IllegalArgumentException("width < 0"); - } - if (sizeY < 0) { - throw new IllegalArgumentException("height < 0"); - } + // Check planar read size (sizeX and sizeY only) + checkReadSize(Arrays.copyOfRange(shape, 3, 5)); + try { ByteBuffer asByteBuffer = ByteBuffer.wrap(buffer); DataType dataType = array.getDataType(); @@ -351,7 +343,17 @@ public void checkBounds(Integer x, Integer y, Integer z, Integer c, if (t != null && (t > getSizeT() - 1 || t < 0)) { throw new DimensionsOutOfBoundsException("T '" + t - + "' greater than sizeT '" + getSizeT() + "'."); + + "' greater than sizeT '" + getSizeT() + "' or < '0'."); + } + } + + public void checkReadSize(int[] shape) { + long length = length(shape); + long maxLength = maxPlaneWidth * maxPlaneHeight; + if (length > maxLength) { + throw new IllegalArgumentException(String.format( + "Requested shape %s > max plane size %d * %d", + Arrays.toString(shape), maxPlaneWidth, maxPlaneHeight)); } } From 4313e95d24d741b89a26a5ac7e4a37bf2061432a Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 14:01:53 +0000 Subject: [PATCH 07/10] Check read size before allocating inner buffer --- .../java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java index f7ec08b..071fa3b 100644 --- a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java +++ b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java @@ -137,6 +137,8 @@ public ZarrPixelBuffer(Pixels pixels, Path root, Integer maxPlaneWidth, int w = key.get(6); int h = key.get(7); int[] shape = new int[] { 1, 1, 1, h, w }; + // Check planar read size (sizeX and sizeY only) + checkReadSize(Arrays.copyOfRange(shape, 3, 5)); byte[] innerBuffer = new byte[(int) length(shape) * getByteWidth()]; setResolutionLevel(resolutionLevel); From cb1ca8ce828c85d3992fac4cf667ff82209428a5 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 14:02:11 +0000 Subject: [PATCH 08/10] Update exception error string to include < 0 --- .../com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java index 071fa3b..2169c0c 100644 --- a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java +++ b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java @@ -326,21 +326,21 @@ public void checkBounds(Integer x, Integer y, Integer z, Integer c, throws DimensionsOutOfBoundsException { if (x != null && (x > getSizeX() - 1 || x < 0)) { throw new DimensionsOutOfBoundsException("X '" + x - + "' greater than sizeX '" + getSizeX() + "'."); + + "' greater than sizeX '" + getSizeX() + "' or < '0'."); } if (y != null && (y > getSizeY() - 1 || y < 0)) { throw new DimensionsOutOfBoundsException("Y '" + y - + "' greater than sizeY '" + getSizeY() + "'."); + + "' greater than sizeY '" + getSizeY() + "' or < '0'."); } if (z != null && (z > getSizeZ() - 1 || z < 0)) { throw new DimensionsOutOfBoundsException("Z '" + z - + "' greater than sizeZ '" + getSizeZ() + "'."); + + "' greater than sizeZ '" + getSizeZ() + "' or < '0'."); } if (c != null && (c > getSizeC() - 1 || c < 0)) { throw new DimensionsOutOfBoundsException("C '" + c - + "' greater than sizeC '" + getSizeC() + "'."); + + "' greater than sizeC '" + getSizeC() + "' or < '0'."); } if (t != null && (t > getSizeT() - 1 || t < 0)) { From e9313f5d465d0051746d012770637ab81c9bf7cb Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 1 Oct 2024 14:56:39 +0000 Subject: [PATCH 09/10] Better exception dispatch when arguments out of bounds --- .../omero/zarr/ZarrPixelBuffer.java | 15 ++++++++++----- .../omero/zarr/ZarrPixelBufferTest.java | 8 ++++---- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java index 2169c0c..29c2e98 100644 --- a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java +++ b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java @@ -28,7 +28,7 @@ import java.util.Arrays; import java.util.List; import java.util.Map; -import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; import java.util.stream.IntStream; @@ -481,10 +481,15 @@ public PixelData getTile( // of channels can be unpredictable. tileCache.synchronous().invalidateAll(); } - CompletableFuture, byte[]>> future = - tileCache.getAll(keys); - Map, byte[]> values = future.join(); - return toPixelData(values.get(key)); + try { + return toPixelData(tileCache.getAll(keys).join().get(key)); + } catch (CompletionException e) { + if (e.getCause() instanceof IllegalArgumentException) { + // Request arguments out of bounds + throw (IllegalArgumentException) e.getCause(); + } + throw e; + } } @Override diff --git a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java index 1268208..85ec303 100644 --- a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java +++ b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java @@ -32,6 +32,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CompletionException; import java.util.stream.IntStream; import org.junit.Assert; @@ -502,7 +503,7 @@ public void testGetTileLargerThanImage() } } - @Test + @Test(expected = IllegalArgumentException.class) public void testTileIntegerOverflow() throws IOException, InvalidRangeException { int sizeT = 1; @@ -534,9 +535,8 @@ public void testTileIntegerOverflow() } } - @Test(expected = DimensionsOutOfBoundsException.class) - public void testTileExceedsMinMax() - throws IOException, InvalidRangeException { + @Test(expected = IllegalArgumentException.class) + public void testTileExceedsMinMax() throws IOException { int sizeT = 1; int sizeC = 3; int sizeZ = 1; From 8a695fa03c80476576cfed6fe15643699d3f7a6e Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Wed, 2 Oct 2024 11:02:31 +0000 Subject: [PATCH 10/10] Do the read size check earlier --- .../omero/zarr/ZarrPixelBuffer.java | 18 ++++++------------ .../omero/zarr/ZarrPixelBufferTest.java | 4 +--- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java index 29c2e98..fc5fd54 100644 --- a/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java +++ b/src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java @@ -28,7 +28,6 @@ import java.util.Arrays; import java.util.List; import java.util.Map; -import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; import java.util.stream.IntStream; @@ -137,8 +136,6 @@ public ZarrPixelBuffer(Pixels pixels, Path root, Integer maxPlaneWidth, int w = key.get(6); int h = key.get(7); int[] shape = new int[] { 1, 1, 1, h, w }; - // Check planar read size (sizeX and sizeY only) - checkReadSize(Arrays.copyOfRange(shape, 3, 5)); byte[] innerBuffer = new byte[(int) length(shape) * getByteWidth()]; setResolutionLevel(resolutionLevel); @@ -459,6 +456,11 @@ public PixelData getTile( checkBounds(x, y, z, c, t); //Check check bottom-right of tile in bounds checkBounds(x + w - 1, y + h - 1, z, c, t); + //Check planar read size (sizeX and sizeY only), essential that this + //happens before the similar check in read(). Otherwise we will + //potentially allocate massive inner buffers in the tile cache + //asynchronous entry builder that will never be used. + checkReadSize(new int[] { w, h }); List> keys = new ArrayList>(); List key = null; @@ -481,15 +483,7 @@ public PixelData getTile( // of channels can be unpredictable. tileCache.synchronous().invalidateAll(); } - try { - return toPixelData(tileCache.getAll(keys).join().get(key)); - } catch (CompletionException e) { - if (e.getCause() instanceof IllegalArgumentException) { - // Request arguments out of bounds - throw (IllegalArgumentException) e.getCause(); - } - throw e; - } + return toPixelData(tileCache.getAll(keys).join().get(key)); } @Override diff --git a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java index 85ec303..1618c9e 100644 --- a/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java +++ b/src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java @@ -32,7 +32,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.CompletionException; import java.util.stream.IntStream; import org.junit.Assert; @@ -530,8 +529,7 @@ public void testTileIntegerOverflow() mapper.writeValue(output.resolve("0/0/.zarray").toFile(), zArray); try (ZarrPixelBuffer zpbuf = createPixelBuffer(pixels, output.resolve("0"), 32, 32)) { - PixelData pixelData = zpbuf.getTile(0, 0, 0, 0, 0, 50000, 50000); - Assert.assertNull(pixelData); + zpbuf.getTile(0, 0, 0, 0, 0, 50000, 50000); } }