This is an automated email from the ASF dual-hosted git repository.
tballison pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tika.git
The following commit(s) were added to refs/heads/main by this push:
new 63284c17c0 TIKA-4936: write the largest image of an icon group first
(#3309)
63284c17c0 is described below
commit 63284c17c0ee28223c4e8b736b49188fb2585ec7
Author: Dominik Schmidt <[email protected]>
AuthorDate: Wed Oct 7 22:01:52 2026 +0200
TIKA-4936: write the largest image of an icon group first (#3309)
* TIKA-4936: write the largest image of an icon group first
A reader that takes an .ico for a multi-page image hands out its first
entry, and the order a resource compiler wrote carries no meaning: Windows
looks up the size it needs rather than reading in order. Of the four
binaries I looked at, two list their icons ascending and two descending,
so whoever renders the first entry gets an arbitrary size - a thumbnailer
asking for 256 pixels can end up with a 16 pixel image blown up.
The entries are now ordered by descending pixel count, then colour depth.
The size comes from the image itself, since a directory entry holds one
byte per side and reads 0 for 256 or above, which is what a large PNG
entry uses.
* TIKA-4936: read a bitmap's size from the image too
The size of an image was meant to come from the image itself, but only 29
bytes were read, short of the 40 a BITMAPINFOHEADER takes, so a bitmap was
always ordered by its directory entry. The full header is read now.
A size no image can have, that is a side of zero or below or beyond what
PNG allows, falls back to the directory entry, which also keeps the pixel
count from overflowing. A PNG has to start with an IHDR chunk to be read.
New tests cover a PNG above 256 pixels next to a 256 pixel bitmap, a
directory entry that disagrees with its bitmap, the colour depth as the
tie break and the fallback.
* TIKA-4936: let the fallback test tell the fallback from its absence
The implausible sizes happened to sort the same way with and without the
fallback to the directory entry, so the test passed either way. The
directory entries now rank the images the other way round than those sizes
would, and a PNG signature that no IHDR chunk follows joins them.
* TIKA-4936: say what the icon order and the tie break cover
The class comment did not mention that the rebuilt file lists its images
largest first, and the note on the stable sort left out that the colour
depth has to agree as well. The round-trip test compares bytes with a
fixture rather than with the source icon, so it keeps the name it had.
---
.../tika/parser/executable/PEIconExtractor.java | 102 ++++++++++++++-
.../parser/executable/PEIconExtractorTest.java | 141 ++++++++++++++++++++-
.../testWindows-icons-app-rebuilt.ico | Bin 0 -> 7065 bytes
.../testWindows-icons-doc-rebuilt.ico | Bin 0 -> 10806 bytes
4 files changed, 231 insertions(+), 12 deletions(-)
diff --git
a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/main/java/org/apache/tika/parser/executable/PEIconExtractor.java
b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/main/java/org/apache/tika/parser/executable/PEIconExtractor.java
index f3747046f3..5aa08427dc 100644
---
a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/main/java/org/apache/tika/parser/executable/PEIconExtractor.java
+++
b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/main/java/org/apache/tika/parser/executable/PEIconExtractor.java
@@ -59,7 +59,7 @@ import org.apache.tika.sax.XHTMLContentHandler;
* The {@code .ico} file format is nearly identical to the group resource;
* the only difference is that the group refers to its images by resource id
* whereas the file refers to them by file offset. This class rebuilds the
- * file from the two resource types.
+ * file from the two resource types, with the largest image first.
* <p>
* File-backed input is read where the resource tree points, so only the
* directories and the icons themselves are touched. Anything else has to be
@@ -85,6 +85,12 @@ class PEIconExtractor {
private static final int RESOURCE_DIRECTORY_SIZE = 16;
private static final int RESOURCE_DIRECTORY_ENTRY_SIZE = 8;
private static final int RESOURCE_DATA_ENTRY_SIZE = 16;
+ private static final byte[] PNG_SIGNATURE =
+ {(byte) 0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a};
+ private static final int PNG_IHDR = 0x49484452;
+ /** the signature, the IHDR chunk's length and type, then its width and
height */
+ private static final int PNG_DIMENSIONS_END = 8 + 8 + 8;
+ private static final int BITMAP_INFO_HEADER_SIZE = 40;
private static final int GRP_ICON_DIR_SIZE = 6;
private static final int GRP_ICON_DIR_ENTRY_SIZE = 14;
private static final int ICON_DIR_ENTRY_SIZE = 16;
@@ -231,8 +237,7 @@ class PEIconExtractor {
}
List<Resource> images = new ArrayList<>(count);
for (int i = 0; i < count; i++) {
- int id = EndianUtils.getUShortLE(dir,
- GRP_ICON_DIR_SIZE + i * GRP_ICON_DIR_ENTRY_SIZE + 12);
+ int id = EndianUtils.getUShortLE(dir, groupEntry(i) + 12);
Resource image = tree.findIcon(id, group.language);
if (image == null) {
return null;
@@ -285,18 +290,20 @@ class PEIconExtractor {
int imageOffset = GRP_ICON_DIR_SIZE + count * ICON_DIR_ENTRY_SIZE;
ByteBuffer ico = ByteBuffer.allocate((int) icoSize(icon.images))
.order(ByteOrder.LITTLE_ENDIAN);
+ List<Integer> order = largestFirst(icon, section);
// ICONDIR: reserved, type, count - identical to the GRPICONDIR
ico.put(icon.dir, 0, GRP_ICON_DIR_SIZE);
- for (int i = 0; i < count; i++) {
- int size = (int) icon.images.get(i).size;
+ for (int index : order) {
+ int size = (int) icon.images.get(index).size;
// width, height, colours, reserved, planes and bit count are
shared
- ico.put(icon.dir, GRP_ICON_DIR_SIZE + i * GRP_ICON_DIR_ENTRY_SIZE,
8);
+ ico.put(icon.dir, groupEntry(index), 8);
// the group's BytesInRes may disagree with the actual resource;
trust the resource
ico.putInt(size);
ico.putInt(imageOffset);
imageOffset += size;
}
- for (Resource image : icon.images) {
+ for (int index : order) {
+ Resource image = icon.images.get(index);
if (!section.read(image.offset, ico.array(), ico.position(), (int)
image.size)) {
return null;
}
@@ -305,6 +312,72 @@ class PEIconExtractor {
return ico.array();
}
+ /**
+ * Orders a group's images, largest first. The order a resource compiler
+ * wrote carries no meaning - Windows looks up the size it needs - but a
+ * reader that treats an icon as a multi-page image hands out the first
+ * entry, and for a single image out of an icon that should be the best
one.
+ *
+ * @return indices into the group, by descending pixel count and colour
depth
+ */
+ private static List<Integer> largestFirst(Icon icon, Section section)
throws IOException {
+ int count = icon.images.size();
+ long[] pixels = new long[count];
+ List<Integer> order = new ArrayList<>(count);
+ for (int i = 0; i < count; i++) {
+ pixels[i] = pixelCount(icon, i, section);
+ order.add(i);
+ }
+ // a stable sort, so images of one size and depth keep the order the
group has
+ order.sort(Comparator.<Integer>comparingLong(index -> pixels[index])
+ .thenComparingInt(icon::bitCount).reversed());
+ return order;
+ }
+
+ /**
+ * The pixels an image covers, read from the image itself: a directory
entry
+ * holds one byte per side and reads 0 for 256 or above, so a 1024 pixel
PNG
+ * and a 256 pixel bitmap are indistinguishable there.
+ *
+ * @return the pixel count, falling back to what the directory entry claims
+ */
+ private static long pixelCount(Icon icon, int index, Section section)
throws IOException {
+ Resource image = icon.images.get(index);
+ byte[] head = section.read(image.offset,
+ (int) Math.min(image.size, BITMAP_INFO_HEADER_SIZE));
+ long width = 0;
+ long height = 0;
+ if (head != null && startsWithPngHeader(head)) {
+ width = EndianUtils.getUIntBE(head, 16);
+ height = EndianUtils.getUIntBE(head, 20);
+ } else if (head != null && head.length >= BITMAP_INFO_HEADER_SIZE &&
+ EndianUtils.getUIntLE(head, 0) == BITMAP_INFO_HEADER_SIZE) {
+ width = EndianUtils.getIntLE(head, 4);
+ // the height covers the XOR bitmap and the AND mask below it; a
+ // negative one means the rows are stored top-down
+ height = Math.abs((long) EndianUtils.getIntLE(head, 8)) / 2;
+ }
+ if (width <= 0 || height <= 0 || width > Integer.MAX_VALUE ||
+ height > Integer.MAX_VALUE) {
+ return icon.claimedPixelCount(index);
+ }
+ return width * height;
+ }
+
+ private static boolean startsWithPngHeader(byte[] head) {
+ return head.length >= PNG_DIMENSIONS_END &&
+ Arrays.equals(head, 0, PNG_SIGNATURE.length, PNG_SIGNATURE, 0,
+ PNG_SIGNATURE.length) &&
+ EndianUtils.getIntBE(head, 12) == PNG_IHDR;
+ }
+
+ /**
+ * @return the offset of an entry in a {@code GRPICONDIR}
+ */
+ private static int groupEntry(int index) {
+ return GRP_ICON_DIR_SIZE + index * GRP_ICON_DIR_ENTRY_SIZE;
+ }
+
/**
* The three level resource tree (type / name / language), reduced to its
* icons and icon groups.
@@ -585,6 +658,21 @@ class PEIconExtractor {
* A usable icon group: its {@code GRPICONDIR} and the images it lists.
*/
private record Icon(byte[] dir, List<Resource> images) {
+
+ int bitCount(int index) {
+ return EndianUtils.getUShortLE(dir, groupEntry(index) + 6);
+ }
+
+ long claimedPixelCount(int index) {
+ return (long) claimedSide(dir[groupEntry(index)]) *
+ claimedSide(dir[groupEntry(index) + 1]);
+ }
+
+ /** A side of 0 in a directory entry means 256 or above. */
+ private static int claimedSide(byte side) {
+ int value = side & 0xff;
+ return value == 0 ? 256 : value;
+ }
}
/**
diff --git
a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/java/org/apache/tika/parser/executable/PEIconExtractorTest.java
b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/java/org/apache/tika/parser/executable/PEIconExtractorTest.java
index c5d66be4bf..c940135fbf 100644
---
a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/java/org/apache/tika/parser/executable/PEIconExtractorTest.java
+++
b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/java/org/apache/tika/parser/executable/PEIconExtractorTest.java
@@ -40,6 +40,7 @@ import java.util.Arrays;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
+import java.util.stream.IntStream;
import org.apache.commons.io.input.CountingInputStream;
import org.junit.jupiter.api.Test;
@@ -95,6 +96,9 @@ import org.apache.tika.sax.XHTMLContentHandler;
* x86_64-w64-mingw32-gcc -Os -s -shared -nostdlib -o
testWindows-x86-64-icons-lang.dll \
* dll.c icons-lang64.o
* </pre>
+ * The -rebuilt.ico files hold the same images as the .ico files the resources
+ * were compiled from, ordered largest first, which is what the extractor
writes.
+ *
* Layout of testWindows-x86-32-icons.exe (file offsets, from pefile):
* .rsrc raw 0x3400-0x7c00 at RVA 0xa000; root directory 0x3400 with the
* type entries at 0x3410 (RT_ICON) and 0x3418 (RT_GROUP_ICON); data entries
@@ -107,8 +111,10 @@ public class PEIconExtractorTest extends TikaTest {
private static final String EXE = "testWindows-x86-32-icons.exe";
private static final String DLL = "testWindows-x86-64-icons.dll";
private static final String LANG_DLL = "testWindows-x86-64-icons-lang.dll";
- private static final String APP_ICO = "testWindows-icons-app.ico";
- private static final String DOC_ICO = "testWindows-icons-doc.ico";
+ private static final int ICON_DIR_ENTRY_SIZE = 16;
+ private static final String APP_ICO_SOURCE = "testWindows-icons-app.ico";
+ private static final String APP_ICO = "testWindows-icons-app-rebuilt.ico";
+ private static final String DOC_ICO = "testWindows-icons-doc-rebuilt.ico";
private static final String THUMBNAIL =
TikaCoreProperties.EmbeddedResourceType.THUMBNAIL.toString();
@@ -156,9 +162,82 @@ public class PEIconExtractorTest extends TikaTest {
}
/**
- * The resource compiler copies the images verbatim, so the rebuilt
- * .ico files must be identical to the ones that went in. Checks the
- * metadata as handed to the extractor, before any re-detection.
+ * A reader that takes an icon group for a multi-page image hands out the
+ * first entry, so the largest image goes there. The app icon is compiled
+ * from a 16, a 32 and a 256 pixel image, in that order; a width of 0 in a
+ * directory entry stands for 256.
+ */
+ @Test
+ public void testLargestIconComesFirst() throws Exception {
+ assertArrayEquals(new int[]{16, 32, 0},
widths(readTestResource(APP_ICO_SOURCE)),
+ "the icon the resources were compiled from");
+
+ RecordingExtractor extractor = parse(readTestResource(EXE));
+ assertArrayEquals(new int[]{0, 32, 16},
widths(extractor.contents.get(1)));
+ }
+
+ /**
+ * A side of 0 stands for anything from 256 up, so only the images tell a
+ * 1024 pixel PNG from a 256 pixel bitmap, and an entry that claims 16
+ * pixels for a 48 pixel bitmap is not believed either. The images differ
+ * in length, which is how the result tells them apart.
+ */
+ @Test
+ public void testOrderFollowsTheImagesNotTheDirectory() throws Exception {
+ byte[] pe = groupPe(new int[][]{{0, 32}, {0, 32}, {16, 32}, {32, 32}},
+ bitmap(41, 256, 256), png(42, 1024, 1024), bitmap(43, 48, 48),
+ bitmap(44, 32, 32));
+ RecordingExtractor extractor = parse(pe);
+ assertEquals(1, extractor.contents.size());
+ assertArrayEquals(new int[]{42, 41, 43, 44},
imageLengths(extractor.contents.get(0)));
+ }
+
+ /**
+ * Among images of one size the deeper one comes first, and those that
+ * agree in both keep the order of the group.
+ */
+ @Test
+ public void testColourDepthBreaksTies() throws Exception {
+ byte[] pe = groupPe(new int[][]{{32, 4}, {32, 32}, {32, 8}, {32, 32}},
+ bitmap(41, 32, 32), bitmap(42, 32, 32), bitmap(43, 32, 32),
+ bitmap(44, 32, 32));
+ RecordingExtractor extractor = parse(pe);
+ assertEquals(1, extractor.contents.size());
+ assertArrayEquals(new int[]{42, 44, 43, 41},
imageLengths(extractor.contents.get(0)));
+ }
+
+ /**
+ * A size no image can have - sides beyond what PNG allows, a negative
+ * width, no height at all - is not sorted by, and neither is what follows
+ * a PNG signature without an IHDR chunk; the directory entry is. The
+ * entries rank the images the other way round than those sizes would.
+ */
+ @Test
+ public void testImplausibleImageSizeFallsBackToTheDirectory() throws
Exception {
+ byte[] noIhdr = png(45, 1024, 1024);
+ noIhdr[12] = 'J';
+ byte[] pe = groupPe(new int[][]{{16, 32}, {8, 32}, {48, 32}, {32, 32},
{24, 32}},
+ bitmap(44, 16, 16), noIhdr, png(41, -1, -1), bitmap(42, -4096,
4096),
+ bitmap(43, 4096, 0));
+ RecordingExtractor extractor = parse(pe);
+ assertEquals(1, extractor.contents.size());
+ assertArrayEquals(new int[]{41, 42, 43, 44, 45},
+ imageLengths(extractor.contents.get(0)));
+ }
+
+ /** The width of every directory entry of an .ico, in file order. */
+ private static int[] widths(byte[] ico) {
+ int[] widths = new int[ico[4] & 0xff];
+ for (int i = 0; i < widths.length; i++) {
+ widths[i] = ico[6 + i * ICON_DIR_ENTRY_SIZE] & 0xff;
+ }
+ return widths;
+ }
+
+ /**
+ * The resource compiler copies the images verbatim, so the rebuilt .ico
+ * files hold the images of the ones that went in, ordered largest first.
+ * Checks the metadata as handed to the extractor, before any re-detection.
*/
@Test
public void testReconstructedIcoIsByteIdentical() throws Exception {
@@ -863,6 +942,58 @@ public class PEIconExtractorTest extends TikaTest {
return dir.array();
}
+ /**
+ * @param entries the side and the bit count each directory entry claims
+ * @return a file with one group of the given images, in the given order
+ */
+ private static byte[] groupPe(int[][] entries, byte[]... images) {
+ byte[] dir = grpIconDir(IntStream.rangeClosed(1,
images.length).toArray());
+ int size = dir.length;
+ for (byte[] image : images) {
+ size += image.length;
+ }
+ ByteBuffer data =
ByteBuffer.allocate(size).order(ByteOrder.LITTLE_ENDIAN);
+ SyntheticResources resources = new SyntheticResources(data.array());
+ resources.group(0, dir.length);
+ data.put(dir);
+ for (int i = 0; i < images.length; i++) {
+ data.put(6 + 14 * i, (byte) entries[i][0]).put(6 + 14 * i + 1,
(byte) entries[i][0]);
+ data.putShort(6 + 14 * i + 6, (short) entries[i][1]);
+ resources.icon(data.position(), images[i].length);
+ data.put(images[i]);
+ }
+ return SyntheticPE.build(resources.build(), 0);
+ }
+
+ /**
+ * @return the start of a PNG of the given size, padded to the given length
+ */
+ private static byte[] png(int length, int width, int height) {
+ ByteBuffer png = ByteBuffer.allocate(length);
+ png.putLong(0x89504e470d0a1a0aL).putInt(13).putInt(0x49484452);
+ png.putInt(width).putInt(height);
+ return png.array();
+ }
+
+ /**
+ * @return the BITMAPINFOHEADER of an icon image of the given size, its
+ * height doubled for the mask, padded to the given length
+ */
+ private static byte[] bitmap(int length, int width, int height) {
+ ByteBuffer bitmap =
ByteBuffer.allocate(length).order(ByteOrder.LITTLE_ENDIAN);
+ bitmap.putInt(40).putInt(width).putInt(2 * height);
+ return bitmap.array();
+ }
+
+ /** The length of every image of an .ico, in file order. */
+ private static int[] imageLengths(byte[] ico) {
+ int[] lengths = new int[ico[4] & 0xff];
+ for (int i = 0; i < lengths.length; i++) {
+ lengths[i] = EndianUtils.getIntLE(ico, 6 + i * ICON_DIR_ENTRY_SIZE
+ 8);
+ }
+ return lengths;
+ }
+
/**
* @return a file in which the given number of groups all list one image
*/
diff --git
a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/resources/test-documents/testWindows-icons-app-rebuilt.ico
b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/resources/test-documents/testWindows-icons-app-rebuilt.ico
new file mode 100644
index 0000000000..1b81b3dca6
Binary files /dev/null and
b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/resources/test-documents/testWindows-icons-app-rebuilt.ico
differ
diff --git
a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/resources/test-documents/testWindows-icons-doc-rebuilt.ico
b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/resources/test-documents/testWindows-icons-doc-rebuilt.ico
new file mode 100644
index 0000000000..f8c49f7a0b
Binary files /dev/null and
b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-code-module/src/test/resources/test-documents/testWindows-icons-doc-rebuilt.ico
differ
