From bbdf61591c50f28083750eec8c86239a7af3592d Mon Sep 17 00:00:00 2001 From: Benjamin Root Date: Mon, 6 May 2024 17:01:42 -0400 Subject: [PATCH 1/4] Fix and extend Geotiff IFD handling to allow all integer types * Follows specs from pg. 15-16 of v6.0 TIFF: https://www.itu.int/itudoc/itu-t/com16/tiff-fx/docs/tiff6.pdf * Added roundtrip tests of the private Geotiff method via reflection * This does reveal an existing API flaw that will cause overflow of large unsigned integers --- .../main/java/ucar/nc2/geotiff/GeoTiff.java | 33 +++++- .../java/ucar/nc2/geotiff/TestIFDEntry.java | 104 ++++++++++++++++++ 2 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java diff --git a/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java b/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java index 4678b7c9f5..bab1d8c72a 100644 --- a/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java +++ b/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java @@ -357,13 +357,23 @@ private int writeValues(ByteBuffer buffer, IFDEntry ifd) { private int writeIntValue(ByteBuffer buffer, IFDEntry ifd, int v) { switch (ifd.type.code) { case 1: + case 2: + case 6: + case 7: + // unsigned byte and ascii + // signed byte and undefined (usually treated as raw binary) buffer.put((byte) v); return 1; case 3: + case 8: + // unsigned and signed short buffer.putShort((short) v); return 2; case 4: case 5: + case 9: + case 10: + // unsigned and signed rational and 32-bit integer buffer.putInt(v); return 4; } @@ -548,13 +558,32 @@ private void readValues(ByteBuffer buffer, IFDEntry ifd) { private int readIntValue(ByteBuffer buffer, IFDEntry ifd) { switch (ifd.type.code) { case 1: + // unsigned byte + //return (int) buffer.get() & 0xff; case 2: - return (int) buffer.get(); + // unsigned ascii + return (int) buffer.get() & 0xff; case 3: + // unsigned short return readUShortValue(buffer); case 4: case 5: - return buffer.getInt(); + // unsigned rational and 32-bit integer + // Yes, this can lead to truncation. This is a bug + // in the design of the IFDEntry API + return (int) (buffer.getInt() & 0xffffffffL); + //return buffer.getInt(); + case 6: + case 7: + // signed byte & "undefined" (usually treated as binary data + return (int) buffer.get(); + case 8: + // signed short + return (int) buffer.getShort(); + case 9: + case 10: + // signed rational and 32-bit integer + return buffer.getInt(); } return 0; } diff --git a/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java b/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java new file mode 100644 index 0000000000..e293bdb3db --- /dev/null +++ b/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java @@ -0,0 +1,104 @@ +/* + * Copyright (c) 1998-2024 University Corporation for Atmospheric Research/Unidata + * See LICENSE for license information. + */ + +package ucar.nc2.geotiff; + +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import java.io.*; +import java.lang.ReflectiveOperationException; +import java.lang.invoke.MethodHandles; +import java.lang.reflect.Method; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * IFDEntry read/write + * + * @author Ben Root + * @since 5/6/2024 + */ +@RunWith(Parameterized.class) +public class TestIFDEntry { + private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + public IFDEntry ifd; + // Because we are dealing with reflection + public int testValue; + + @Parameterized.Parameters(name = "{0}_{1}") + public static List getTestParameters() { + List result = new ArrayList<>(); + // Unsigned + result.add(new Object[] {new IFDEntry(null, FieldType.BYTE, 1), 0}); + result.add(new Object[] {new IFDEntry(null, FieldType.BYTE, 1), Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.BYTE, 1), 255}); + result.add(new Object[] {new IFDEntry(null, FieldType.ASCII, 1), 0}); + result.add(new Object[] {new IFDEntry(null, FieldType.ASCII, 1), Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.ASCII, 1), 255}); + result.add(new Object[] {new IFDEntry(null, FieldType.SHORT, 1), 0}); + result.add(new Object[] {new IFDEntry(null, FieldType.SHORT, 1), Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SHORT, 1), Short.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SHORT, 1), 65535}); + result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), 0}); + result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), Short.MAX_VALUE}); + // NOTE: because of the API design, unsigned longs can't be properly read or written + // for all possible values because Java's integer is signed. + result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), Integer.MAX_VALUE}); + + //Signed + result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), 0}); + result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), Byte.MIN_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), 0}); + result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), Byte.MIN_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), -Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), Short.MIN_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SSHORT, 1), Short.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), 0}); + result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), -Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), Byte.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), -Short.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), Short.MAX_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), Integer.MIN_VALUE}); + result.add(new Object[] {new IFDEntry(null, FieldType.SLONG, 1), Integer.MAX_VALUE}); + + return result; + } + + public TestIFDEntry(IFDEntry ifd, int testValue) { + this.ifd = ifd; + this.testValue = testValue; + } + + @Test + public void testRoundtrip() throws ReflectiveOperationException { + GeoTiff geotiff = new GeoTiff("foobar"); + // 16 bytes should be more than enough + ByteBuffer buffer = ByteBuffer.allocate(16); + ByteOrder byteOrder = ByteOrder.BIG_ENDIAN; + buffer.order(byteOrder); + + logger.info("geotiff methods: {}", geotiff.getClass().getDeclaredMethods()); + Method writeMethod = geotiff.getClass().getDeclaredMethod("writeIntValue", ByteBuffer.class, ifd.getClass(), int.class); + Method readMethod = geotiff.getClass().getDeclaredMethod("readIntValue", ByteBuffer.class, ifd.getClass()); + readMethod.setAccessible(true); + writeMethod.setAccessible(true); + int writeSize = (int) writeMethod.invoke(geotiff, buffer, ifd, testValue); + buffer.position(0); + + int readValue = (int) readMethod.invoke(geotiff, buffer, ifd); + + Assert.assertEquals(testValue, readValue); + } +} From 07e9ec32372c00fcaada585306f48aa8d9872d87 Mon Sep 17 00:00:00 2001 From: Benjamin Root Date: Mon, 6 May 2024 17:12:48 -0400 Subject: [PATCH 2/4] style check and some other cleanups --- .../main/java/ucar/nc2/geotiff/GeoTiff.java | 27 +++++++++---------- .../java/ucar/nc2/geotiff/TestIFDEntry.java | 6 ++--- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java b/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java index bab1d8c72a..41c11ca5b8 100644 --- a/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java +++ b/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java @@ -360,20 +360,20 @@ private int writeIntValue(ByteBuffer buffer, IFDEntry ifd, int v) { case 2: case 6: case 7: - // unsigned byte and ascii - // signed byte and undefined (usually treated as raw binary) + // unsigned byte and ascii + // signed byte and undefined (usually treated as raw binary) buffer.put((byte) v); return 1; case 3: case 8: - // unsigned and signed short + // unsigned and signed short buffer.putShort((short) v); return 2; case 4: case 5: case 9: case 10: - // unsigned and signed rational and 32-bit integer + // unsigned and signed rational and 32-bit integer buffer.putInt(v); return 4; } @@ -558,32 +558,29 @@ private void readValues(ByteBuffer buffer, IFDEntry ifd) { private int readIntValue(ByteBuffer buffer, IFDEntry ifd) { switch (ifd.type.code) { case 1: - // unsigned byte - //return (int) buffer.get() & 0xff; case 2: - // unsigned ascii + // unsigned byte and unsigned ascii return (int) buffer.get() & 0xff; case 3: - // unsigned short + // unsigned short return readUShortValue(buffer); case 4: case 5: - // unsigned rational and 32-bit integer + // unsigned rational and unsigned 32-bit integer // Yes, this can lead to truncation. This is a bug // in the design of the IFDEntry API return (int) (buffer.getInt() & 0xffffffffL); - //return buffer.getInt(); case 6: case 7: - // signed byte & "undefined" (usually treated as binary data + // signed byte and "undefined" (usually treated as binary data) return (int) buffer.get(); case 8: - // signed short - return (int) buffer.getShort(); + // signed short + return (int) buffer.getShort(); case 9: case 10: - // signed rational and 32-bit integer - return buffer.getInt(); + // signed rational and signed 32-bit integer + return buffer.getInt(); } return 0; } diff --git a/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java b/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java index e293bdb3db..fdd3873944 100644 --- a/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java +++ b/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java @@ -55,7 +55,7 @@ public static List getTestParameters() { // for all possible values because Java's integer is signed. result.add(new Object[] {new IFDEntry(null, FieldType.LONG, 1), Integer.MAX_VALUE}); - //Signed + // Signed result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), 0}); result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), Byte.MIN_VALUE}); result.add(new Object[] {new IFDEntry(null, FieldType.SBYTE, 1), Byte.MAX_VALUE}); @@ -89,8 +89,8 @@ public void testRoundtrip() throws ReflectiveOperationException { ByteOrder byteOrder = ByteOrder.BIG_ENDIAN; buffer.order(byteOrder); - logger.info("geotiff methods: {}", geotiff.getClass().getDeclaredMethods()); - Method writeMethod = geotiff.getClass().getDeclaredMethod("writeIntValue", ByteBuffer.class, ifd.getClass(), int.class); + Method writeMethod = + geotiff.getClass().getDeclaredMethod("writeIntValue", ByteBuffer.class, ifd.getClass(), int.class); Method readMethod = geotiff.getClass().getDeclaredMethod("readIntValue", ByteBuffer.class, ifd.getClass()); readMethod.setAccessible(true); writeMethod.setAccessible(true); From 610b1fb0bba6affca7263d90264ccba2b354f6de Mon Sep 17 00:00:00 2001 From: Benjamin Root Date: Tue, 7 May 2024 14:31:57 -0400 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Tara Drwenski --- cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java b/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java index fdd3873944..8c8ab2a8d2 100644 --- a/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java +++ b/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java @@ -11,14 +11,12 @@ import org.junit.runners.Parameterized; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.*; import java.lang.ReflectiveOperationException; import java.lang.invoke.MethodHandles; import java.lang.reflect.Method; import java.nio.ByteBuffer; import java.nio.ByteOrder; import java.util.ArrayList; -import java.util.Collections; import java.util.List; /** @@ -30,9 +28,8 @@ @RunWith(Parameterized.class) public class TestIFDEntry { private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - public IFDEntry ifd; - // Because we are dealing with reflection - public int testValue; + private final IFDEntry ifd; + private final int testValue; @Parameterized.Parameters(name = "{0}_{1}") public static List getTestParameters() { From ec4f996dc2604a546179cc66ec84966d27a8ee06 Mon Sep 17 00:00:00 2001 From: Benjamin Root Date: Tue, 7 May 2024 15:05:44 -0400 Subject: [PATCH 4/4] Change some of the GeoTiff methods to package-private static * This allows us to drop reflection in the TestIFDEntry tests * Simplifies the test setup and execution --- .../src/main/java/ucar/nc2/geotiff/GeoTiff.java | 14 +++++++------- .../test/java/ucar/nc2/geotiff/TestIFDEntry.java | 15 ++++----------- 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java b/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java index 41c11ca5b8..2bbdbf0731 100644 --- a/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java +++ b/cdm/misc/src/main/java/ucar/nc2/geotiff/GeoTiff.java @@ -326,7 +326,7 @@ private void writeIFDEntry(FileChannel channel, IFDEntry ifd, int start) throws } } - private int writeValues(ByteBuffer buffer, IFDEntry ifd) { + static int writeValues(ByteBuffer buffer, IFDEntry ifd) { int done = 0; if (ifd.type == FieldType.ASCII) { @@ -354,7 +354,7 @@ private int writeValues(ByteBuffer buffer, IFDEntry ifd) { return done; } - private int writeIntValue(ByteBuffer buffer, IFDEntry ifd, int v) { + static int writeIntValue(ByteBuffer buffer, IFDEntry ifd, int v) { switch (ifd.type.code) { case 1: case 2: @@ -380,7 +380,7 @@ private int writeIntValue(ByteBuffer buffer, IFDEntry ifd, int v) { return 0; } - private int writeSValue(ByteBuffer buffer, IFDEntry ifd) { + static int writeSValue(ByteBuffer buffer, IFDEntry ifd) { buffer.put(ifd.valueS.getBytes(StandardCharsets.UTF_8)); int size = ifd.valueS.length(); if ((size & 1) != 0) @@ -527,7 +527,7 @@ private IFDEntry readIFDEntry(FileChannel channel, int start) throws IOException return ifd; } - private void readValues(ByteBuffer buffer, IFDEntry ifd) { + static void readValues(ByteBuffer buffer, IFDEntry ifd) { if (ifd.type == FieldType.ASCII) { ifd.valueS = readSValue(buffer, ifd); @@ -555,7 +555,7 @@ private void readValues(ByteBuffer buffer, IFDEntry ifd) { } - private int readIntValue(ByteBuffer buffer, IFDEntry ifd) { + static int readIntValue(ByteBuffer buffer, IFDEntry ifd) { switch (ifd.type.code) { case 1: case 2: @@ -585,11 +585,11 @@ private int readIntValue(ByteBuffer buffer, IFDEntry ifd) { return 0; } - private int readUShortValue(ByteBuffer buffer) { + static int readUShortValue(ByteBuffer buffer) { return buffer.getShort() & 0xffff; } - private String readSValue(ByteBuffer buffer, IFDEntry ifd) { + static String readSValue(ByteBuffer buffer, IFDEntry ifd) { byte[] dst = new byte[ifd.count]; buffer.get(dst); return new String(dst, StandardCharsets.UTF_8); diff --git a/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java b/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java index 8c8ab2a8d2..5ed03353d9 100644 --- a/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java +++ b/cdm/misc/src/test/java/ucar/nc2/geotiff/TestIFDEntry.java @@ -11,9 +11,7 @@ import org.junit.runners.Parameterized; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.lang.ReflectiveOperationException; import java.lang.invoke.MethodHandles; -import java.lang.reflect.Method; import java.nio.ByteBuffer; import java.nio.ByteOrder; import java.util.ArrayList; @@ -79,22 +77,17 @@ public TestIFDEntry(IFDEntry ifd, int testValue) { } @Test - public void testRoundtrip() throws ReflectiveOperationException { - GeoTiff geotiff = new GeoTiff("foobar"); + public void testRoundtrip() { // 16 bytes should be more than enough ByteBuffer buffer = ByteBuffer.allocate(16); ByteOrder byteOrder = ByteOrder.BIG_ENDIAN; buffer.order(byteOrder); - Method writeMethod = - geotiff.getClass().getDeclaredMethod("writeIntValue", ByteBuffer.class, ifd.getClass(), int.class); - Method readMethod = geotiff.getClass().getDeclaredMethod("readIntValue", ByteBuffer.class, ifd.getClass()); - readMethod.setAccessible(true); - writeMethod.setAccessible(true); - int writeSize = (int) writeMethod.invoke(geotiff, buffer, ifd, testValue); + int writeSize = GeoTiff.writeIntValue(buffer, ifd, testValue); + Assert.assertEquals(ifd.type.size, writeSize); buffer.position(0); - int readValue = (int) readMethod.invoke(geotiff, buffer, ifd); + int readValue = GeoTiff.readIntValue(buffer, ifd); Assert.assertEquals(testValue, readValue); }