Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and expand range checks when performing tile retrieval #11

Merged
merged 10 commits into from
Oct 3, 2024
Merged

Fix and expand range checks when performing tile retrieval #11

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f2cb10a
Remove unused imports
chris-allan Oct 1, 2024
3638127
Add test case for integer overflow
chris-allan Oct 1, 2024
bcecc20
Add missing override annotation
chris-allan Oct 1, 2024
1056d95
Update test name and add some documentation
chris-allan Oct 1, 2024
756e356
Expand test case to also check for min overflow
chris-allan Oct 1, 2024
8abbf8b
Simplify read size checks when reading
chris-allan Oct 1, 2024
4313e95
Check read size before allocating inner buffer
chris-allan Oct 1, 2024
cb1ca8c
Update exception error string to include < 0
chris-allan Oct 1, 2024
e9313f5
Better exception dispatch when arguments out of bounds
chris-allan Oct 1, 2024
8a695fa
Do the read size check earlier
chris-allan Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 36 additions & 26 deletions src/main/java/com/glencoesoftware/omero/zarr/ZarrPixelBuffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -137,8 +137,10 @@ 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));
sbesson marked this conversation as resolved.
Show resolved Hide resolved
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);
});
Expand Down Expand Up @@ -181,25 +183,17 @@ public int getPixelsType() {
* "https://numpy.org/doc/stable/reference/generated/numpy.shape.html">
* numpy.shape</a> 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();
Expand Down Expand Up @@ -326,31 +320,42 @@ 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 {
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)) {
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth doing long multiplication here too? Or do we know for sure that maxPlaneWidth * maxPlaneHeight will always be less than Integer.MAX_VALUE?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know for sure but if you were to configure your server like that all sorts of other stuff would break spectacularly.

if (length > maxLength) {
throw new IllegalArgumentException(String.format(
"Requested shape %s > max plane size %d * %d",
Arrays.toString(shape), maxPlaneWidth, maxPlaneHeight));
}
}

Expand Down Expand Up @@ -476,10 +481,15 @@ public PixelData getTile(
// of channels can be unpredictable.
tileCache.synchronous().invalidateAll();
}
CompletableFuture<Map<List<Integer>, byte[]>> future =
tileCache.getAll(keys);
Map<List<Integer>, 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
Expand Down
45 changes: 39 additions & 6 deletions src/test/java/com/glencoesoftware/omero/zarr/ZarrPixelBufferTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -41,12 +42,11 @@

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;
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;
Expand Down Expand Up @@ -503,8 +503,40 @@ public void testGetTileLargerThanImage()
}
}

@Test
public void testTileExceedsMax() throws IOException, InvalidRangeException {
@Test(expected = IllegalArgumentException.class)
public void testTileIntegerOverflow()
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);

// Hack the .zarray so we can appear as though we have more data than
// we actually have written above.
Copy link
Member

@sbesson sbesson Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the rationale if that is we want to generate an OME-Zarr of a given dimensions without actually writing the underlying test data (which increases the test time for no value).

@melissalinkert this makes me wonder whether we could introduce an option in bioformats2raw to generate Zarr datasets with sparse data

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the rationale if that is we want to generate an OME-Zarr of a given dimensions without actually writing the underlying test data (which increases the test time for no value).

Correct. Creating an array large enough to not end up with earlier out of bounds checks can take minutes.

ObjectMapper mapper = new ObjectMapper();
HashMap<String, Object> zArray = mapper.readValue(
Files.readAllBytes(output.resolve("0/0/.zarray")),
HashMap.class);
List<Integer> shape = (List<Integer>) 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);
chris-allan marked this conversation as resolved.
Show resolved Hide resolved
}
}

@Test(expected = IllegalArgumentException.class)
public void testTileExceedsMinMax() throws IOException {
int sizeT = 1;
int sizeC = 3;
int sizeZ = 1;
Expand All @@ -519,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);
}
}

Expand Down