This is an automated email from the ASF dual-hosted git repository. tballison pushed a commit to branch TIKA-4784-id3-utf16-bom in repository https://gitbox.apache.org/repos/asf/tika.git
commit b969f2af37060bf7eaf5ffad2aa3b3386044bdad Author: tallison <[email protected]> AuthorDate: Fri Jul 10 10:17:31 2026 -0400 TIKA-4784: fix unicode in id3 --- .../org/apache/tika/parser/mp3/ID3v22Handler.java | 12 +- .../org/apache/tika/parser/mp3/ID3v23Handler.java | 12 +- .../org/apache/tika/parser/mp3/ID3v24Handler.java | 12 +- .../org/apache/tika/parser/mp3/ID3v2Frame.java | 77 ++++++-- .../org/apache/tika/parser/mp3/ID3v2FrameTest.java | 193 +++++++++++++++++++++ .../org/apache/tika/parser/mp3/Mp3ParserTest.java | 63 +++++++ 6 files changed, 356 insertions(+), 13 deletions(-) diff --git a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v22Handler.java b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v22Handler.java index 5f0102cd03..eed4d31079 100644 --- a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v22Handler.java +++ b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v22Handler.java @@ -72,7 +72,7 @@ public class ID3v22Handler implements ID3Tags { copyright = getTagString(tag.data, 0, tag.data.length); break; case "COM": - comments.add(getComment(tag.data, 0, tag.data.length)); + addComment(getComment(tag.data, 0, tag.data.length)); break; case "TRK": trackNumber = getTagString(tag.data, 0, tag.data.length); @@ -113,6 +113,16 @@ public class ID3v22Handler implements ID3Tags { return ID3v2Frame.getComment(data, offset, length); } + /** + * Malformed comment frames decode to null and are skipped rather than + * being added to the list, where they would trip up the consumers. + */ + private void addComment(ID3Comment comment) { + if (comment != null) { + comments.add(comment); + } + } + public boolean getTagsPresent() { return true; } diff --git a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v23Handler.java b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v23Handler.java index 9d10a5ac29..959b6519a6 100644 --- a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v23Handler.java +++ b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v23Handler.java @@ -73,7 +73,7 @@ public class ID3v23Handler implements ID3Tags { copyright = getTagString(tag.data, 0, tag.data.length); break; case "COMM": - comments.add(getComment(tag.data, 0, tag.data.length)); + addComment(getComment(tag.data, 0, tag.data.length)); break; case "TRCK": trackNumber = getTagString(tag.data, 0, tag.data.length); @@ -99,6 +99,16 @@ public class ID3v23Handler implements ID3Tags { return ID3v2Frame.getComment(data, offset, length); } + /** + * Malformed comment frames decode to null and are skipped rather than + * being added to the list, where they would trip up the consumers. + */ + private void addComment(ID3Comment comment) { + if (comment != null) { + comments.add(comment); + } + } + public boolean getTagsPresent() { return true; } diff --git a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v24Handler.java b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v24Handler.java index f9d7b106a8..5b2de1d052 100644 --- a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v24Handler.java +++ b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v24Handler.java @@ -79,7 +79,7 @@ public class ID3v24Handler implements ID3Tags { copyright = getTagString(tag.data, 0, tag.data.length); break; case "COMM": - comments.add(getComment(tag.data, 0, tag.data.length)); + addComment(getComment(tag.data, 0, tag.data.length)); break; case "TRCK": trackNumber = getTagString(tag.data, 0, tag.data.length); @@ -105,6 +105,16 @@ public class ID3v24Handler implements ID3Tags { return ID3v2Frame.getComment(data, offset, length); } + /** + * Malformed comment frames decode to null and are skipped rather than + * being added to the list, where they would trip up the consumers. + */ + private void addComment(ID3Comment comment) { + if (comment != null) { + comments.add(comment); + } + } + public boolean getTagsPresent() { return true; } diff --git a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v2Frame.java b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v2Frame.java index 1dddd1410b..d1dee624ca 100644 --- a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v2Frame.java +++ b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v2Frame.java @@ -230,25 +230,80 @@ public class ID3v2Frame implements MP3Frame { // (return empty string), because new String(..) // gives different results on different JVMs if (encoding.encoding.equals("UTF-16") && actualLength == 2 && - ((data[offset] == (byte) 0xff && data[offset + 1] == (byte) 0xfe) || - (data[offset] == (byte) 0xfe && data[offset + 1] == (byte) 0xff))) { + hasBOM(data, offset, actualLength)) { return ""; } try { // Build the base string - return new String(data, offset, actualLength, encoding.encoding); + return decodeText(data, offset, actualLength, encoding); } catch (UnsupportedEncodingException e) { throw new RuntimeException("Core encoding " + encoding.encoding + " is not available", e); } } + /** + * Decodes text in the frame's declared encoding. + * <p> + * Encoding {@code $01} is defined as UTF-16 <em>with</em> a BOM, but some taggers omit it. + * Java's UTF-16 charset then falls back to big-endian, which silently turns little-endian + * text into CJK mojibake (the ASCII 'T' {@code 0x54 0x00} decodes to U+5400). When the BOM + * is missing, recover the byte order from the position of the NUL bytes instead, and keep + * the big-endian fallback when the bytes carry no signal either way. + */ + private static String decodeText(byte[] data, int offset, int length, TextEncoding encoding) + throws UnsupportedEncodingException { + String charset = encoding.encoding; + if ("UTF-16".equals(charset) && !hasBOM(data, offset, length)) { + charset = guessUTF16ByteOrder(data, offset, length); + } + return new String(data, offset, length, charset); + } + + /** + * Does the text at the given offset start with a UTF-16 byte order mark? + */ + private static boolean hasBOM(byte[] data, int offset, int length) { + if (length < 2) { + return false; + } + int first = data[offset] & 0xff; + int second = data[offset + 1] & 0xff; + return (first == 0xff && second == 0xfe) || (first == 0xfe && second == 0xff); + } + + /** + * Guesses the byte order of BOM-less UTF-16 text by counting which half of each code + * unit is NUL. Characters below U+0100 - which dominate the tags this has to rescue - + * encode as {@code 0x00 lo} big-endian and {@code lo 0x00} little-endian. Text with no + * NUL bytes at all (eg CJK) gives no signal, so it stays on the big-endian default. + */ + private static String guessUTF16ByteOrder(byte[] data, int offset, int length) { + int bigEndian = 0; + int littleEndian = 0; + for (int i = 0; i + 1 < length; i += 2) { + if (data[offset + i] == 0) { + bigEndian++; + } + if (data[offset + i + 1] == 0) { + littleEndian++; + } + } + return littleEndian > bigEndian ? "UTF-16LE" : "UTF-16BE"; + } + /** * Builds up the ID3 comment, by parsing and extracting * the comment string parts from the given data. + * Returns null if the frame is too short or malformed to hold a comment. */ protected static ID3Comment getComment(byte[] data, int offset, int length) { + // A comment is at minimum an encoding flag and a 3 byte language + if (length < 4) { + return null; + } + // Comments must have an encoding int encodingFlag = data[offset]; if (encodingFlag >= 0 && encodingFlag < encodings.length) { @@ -264,6 +319,7 @@ public class ID3v2Frame implements MP3Frame { String lang = getString(data, offset + 1, 3); // After that we have [Desc]\0(\0)[Text] + int end = offset + length; int descStart = offset + 4; int textStart = -1; String description = null; @@ -271,29 +327,30 @@ public class ID3v2Frame implements MP3Frame { // Find where the description ends try { - for (int i = descStart; i < offset + length; i++) { - if (encoding.doubleByte && data[i] == 0 && data[i + 1] == 0) { + for (int i = descStart; i < end; i++) { + // A double byte terminator needs two bytes to be present + if (encoding.doubleByte && i + 1 < end && data[i] == 0 && data[i + 1] == 0) { // Handle LE vs BE on low byte text - if (i + 2 < offset + length && data[i + 1] == 0 && data[i + 2] == 0) { + if (i + 2 < end && data[i + 2] == 0) { i++; } textStart = i + 2; - description = new String(data, descStart, i - descStart, encoding.encoding); + description = decodeText(data, descStart, i - descStart, encoding); break; } if (!encoding.doubleByte && data[i] == 0) { textStart = i + 1; - description = new String(data, descStart, i - descStart, encoding.encoding); + description = decodeText(data, descStart, i - descStart, encoding); break; } } // Did we find the end? if (textStart > -1) { - text = new String(data, textStart, offset + length - textStart, encoding.encoding); + text = decodeText(data, textStart, end - textStart, encoding); } else { // Assume everything is the text - text = new String(data, descStart, offset + length - descStart, encoding.encoding); + text = decodeText(data, descStart, end - descStart, encoding); } // Return diff --git a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/ID3v2FrameTest.java b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/ID3v2FrameTest.java new file mode 100644 index 0000000000..6091d6031c --- /dev/null +++ b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/ID3v2FrameTest.java @@ -0,0 +1,193 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tika.parser.mp3; + +import static java.nio.charset.StandardCharsets.ISO_8859_1; +import static java.nio.charset.StandardCharsets.UTF_16BE; +import static java.nio.charset.StandardCharsets.UTF_16LE; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.nio.charset.Charset; + +import org.junit.jupiter.api.Test; + +import org.apache.tika.parser.mp3.ID3Tags.ID3Comment; + +/** + * Tests the shared ID3v2 text decoding used by every ID3v2.2/2.3/2.4 text frame. + */ +public class ID3v2FrameTest { + + private static final byte ISO_8859_1_FLAG = 0; + private static final byte UTF_16_BOM_FLAG = 1; + private static final byte UTF_16BE_FLAG = 2; + private static final byte UTF_8_FLAG = 3; + + private static final byte[] BOM_LE = {(byte) 0xff, (byte) 0xfe}; + private static final byte[] BOM_BE = {(byte) 0xfe, (byte) 0xff}; + + /** + * Latin text, which encodes to a NUL byte per character in UTF-16 and so carries a + * byte-order signal, and CJK text, which does not. + */ + private static final String LATIN = "Test Copyright"; + private static final String CJK = "日本語"; + + private static byte[] frame(byte encodingFlag, byte[]... parts) { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + out.write(encodingFlag); + for (byte[] part : parts) { + out.write(part, 0, part.length); + } + return out.toByteArray(); + } + + private static byte[] bytes(String text, Charset charset) { + return text.getBytes(charset); + } + + private static String tagString(byte[] data) { + return ID3v2Frame.getTagString(data, 0, data.length); + } + + @Test + public void testSingleByteEncodings() { + assertEquals(LATIN, tagString(frame(ISO_8859_1_FLAG, bytes(LATIN, ISO_8859_1)))); + assertEquals(CJK, tagString(frame(UTF_8_FLAG, bytes(CJK, UTF_8)))); + } + + @Test + public void testUTF16WithBOM() { + assertEquals(LATIN, tagString(frame(UTF_16_BOM_FLAG, BOM_LE, bytes(LATIN, UTF_16LE)))); + assertEquals(LATIN, tagString(frame(UTF_16_BOM_FLAG, BOM_BE, bytes(LATIN, UTF_16BE)))); + assertEquals(CJK, tagString(frame(UTF_16_BOM_FLAG, BOM_LE, bytes(CJK, UTF_16LE)))); + assertEquals(CJK, tagString(frame(UTF_16_BOM_FLAG, BOM_BE, bytes(CJK, UTF_16BE)))); + } + + @Test + public void testUTF16BEWithoutBOMFlag() { + assertEquals(LATIN, tagString(frame(UTF_16BE_FLAG, bytes(LATIN, UTF_16BE)))); + assertEquals(CJK, tagString(frame(UTF_16BE_FLAG, bytes(CJK, UTF_16BE)))); + } + + /** + * Encoding $01 promises a BOM. Taggers that omit it used to be decoded as big-endian, + * turning little-endian text into CJK mojibake ('T' 0x54 0x00 becomes U+5400). + */ + @Test + public void testUTF16WithoutBOMRecoversByteOrder() { + assertEquals(LATIN, tagString(frame(UTF_16_BOM_FLAG, bytes(LATIN, UTF_16LE)))); + assertEquals(LATIN, tagString(frame(UTF_16_BOM_FLAG, bytes(LATIN, UTF_16BE)))); + } + + /** + * A BOM-less frame whose characters are all above U+00FF has no NUL bytes and so gives + * no byte-order signal at all. Nothing can rescue that, so the big-endian default of + * Java's UTF-16 charset must be preserved rather than guessed away. + */ + @Test + public void testUTF16WithoutBOMKeepsBigEndianDefaultWhenNoSignal() { + assertEquals(CJK, tagString(frame(UTF_16_BOM_FLAG, bytes(CJK, UTF_16BE)))); + } + + @Test + public void testNullTerminationIsTrimmed() { + byte[] doubleNul = {0, 0}; + byte[] singleNul = {0}; + assertEquals(LATIN, tagString(frame(UTF_16_BOM_FLAG, bytes(LATIN, UTF_16LE), doubleNul))); + assertEquals(LATIN, + tagString(frame(UTF_16_BOM_FLAG, BOM_LE, bytes(LATIN, UTF_16LE), doubleNul))); + assertEquals(LATIN, tagString(frame(ISO_8859_1_FLAG, bytes(LATIN, ISO_8859_1), singleNul))); + } + + /** + * TIKA-1024: a frame holding nothing but a BOM decodes to the empty string. + */ + @Test + public void testNakedBOM() { + assertEquals("", tagString(frame(UTF_16_BOM_FLAG, BOM_LE))); + assertEquals("", tagString(frame(UTF_16_BOM_FLAG, BOM_BE))); + } + + /** + * COMM frames decode a description and a text part separately, so both have to recover + * the byte order independently. + */ + @Test + public void testCommentWithoutBOMRecoversByteOrder() throws IOException { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + out.write(UTF_16_BOM_FLAG); + out.write(bytes("eng", ISO_8859_1)); + out.write(bytes("Desc", UTF_16LE)); + out.write(new byte[]{0, 0}); + out.write(bytes(LATIN, UTF_16LE)); + byte[] data = out.toByteArray(); + + ID3Comment comment = ID3v2Frame.getComment(data, 0, data.length); + assertEquals("eng", comment.getLanguage()); + assertEquals("Desc", comment.getDescription()); + assertEquals(LATIN, comment.getText()); + } + + @Test + public void testCommentWithBOM() throws IOException { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + out.write(UTF_16_BOM_FLAG); + out.write(bytes("eng", ISO_8859_1)); + out.write(BOM_LE); + out.write(bytes("Desc", UTF_16LE)); + out.write(new byte[]{0, 0}); + out.write(BOM_LE); + out.write(bytes(LATIN, UTF_16LE)); + byte[] data = out.toByteArray(); + + ID3Comment comment = ID3v2Frame.getComment(data, 0, data.length); + assertEquals("eng", comment.getLanguage()); + assertEquals("Desc", comment.getDescription()); + assertEquals(LATIN, comment.getText()); + } + + /** + * A comment frame too short to hold an encoding flag and a 3 byte language, or one + * carrying an unknown encoding flag, decodes to null rather than reading off the end + * of the frame. + */ + @Test + public void testMalformedCommentsReturnNull() { + assertNull(ID3v2Frame.getComment(new byte[0], 0, 0)); + assertNull(ID3v2Frame.getComment(new byte[]{UTF_16_BOM_FLAG}, 0, 1)); + assertNull(ID3v2Frame.getComment(new byte[]{ISO_8859_1_FLAG, 'e', 'n'}, 0, 3)); + // 0x05 is not a defined ID3v2 text encoding + byte[] badFlag = {5, 'e', 'n', 'g', 'D', 'e', 's', 'c', 0, 'T'}; + assertNull(ID3v2Frame.getComment(badFlag, 0, badFlag.length)); + } + + /** + * A double byte comment whose frame ends on a lone NUL has no room for a terminator, + * and must not read past the end of the frame looking for one. + */ + @Test + public void testCommentTruncatedOnTerminatorDoesNotOverrun() { + byte[] data = {UTF_16_BOM_FLAG, 'e', 'n', 'g', 0}; + ID3Comment comment = ID3v2Frame.getComment(data, 0, data.length); + assertEquals("eng", comment.getLanguage()); + } +} diff --git a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/Mp3ParserTest.java b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/Mp3ParserTest.java index 935b4be893..45af3e7679 100644 --- a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/Mp3ParserTest.java +++ b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/Mp3ParserTest.java @@ -19,6 +19,9 @@ package org.apache.tika.parser.mp3; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assumptions.assumeTrue; +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; + import org.junit.jupiter.api.Test; import org.apache.tika.TikaTest; @@ -26,6 +29,8 @@ import org.apache.tika.io.TikaInputStream; import org.apache.tika.metadata.Metadata; import org.apache.tika.metadata.TikaCoreProperties; import org.apache.tika.metadata.XMPDM; +import org.apache.tika.parser.ParseContext; +import org.apache.tika.sax.BodyContentHandler; /** * Test case for parsing mp3 files. @@ -362,4 +367,62 @@ public class Mp3ParserTest extends TikaTest { assertEquals("audio/mpeg", metadata.get(Metadata.CONTENT_TYPE)); assertEquals("", metadata.get(XMPDM.GENRE)); } + + /** + * A malformed COMM frame must not take the whole parse down with it. Each of these + * frame bodies used to abort the parse of otherwise readable audio, with an + * ArrayIndexOutOfBounds, a StringIndexOutOfBounds or a NullPointerException. + */ + @Test + public void testMalformedCommentFrameIsSkipped() throws Exception { + byte[][] bodies = { + new byte[0], // empty body + new byte[]{1}, // no language + new byte[]{0, 'e', 'n'}, // truncated language + // 0x05 is not a defined ID3v2 text encoding + new byte[]{5, 'e', 'n', 'g', 'D', 'e', 's', 'c', 0, 'T'}, + // double byte encoding, but the frame ends before the two byte terminator + new byte[]{1, 'e', 'n', 'g', 0} + }; + for (byte[] body : bodies) { + String name = "COMM body of length " + body.length; + Metadata metadata = new Metadata(); + try (TikaInputStream tis = TikaInputStream.get(mp3WithFrame("COMM", body))) { + new Mp3Parser().parse(tis, new BodyContentHandler(-1), metadata, new ParseContext()); + } + assertEquals("audio/mpeg", metadata.get(Metadata.CONTENT_TYPE), name); + // the audio behind the broken tag is still read + assertEquals("44100", metadata.get(XMPDM.AUDIO_SAMPLE_RATE), name); + } + } + + /** + * Wraps the audio of testMP3noid3.mp3 in an ID3v2.3 tag holding the single given frame. + */ + private byte[] mp3WithFrame(String frameId, byte[] body) throws Exception { + byte[] audio; + try (TikaInputStream tis = getResourceAsStream("/test-documents/testMP3noid3.mp3")) { + audio = tis.readAllBytes(); + } + + ByteArrayOutputStream frame = new ByteArrayOutputStream(); + frame.write(frameId.getBytes(StandardCharsets.US_ASCII)); + // ID3v2.3 frame sizes are plain 32 bit big endian + frame.write(new byte[]{(byte) (body.length >>> 24), (byte) (body.length >>> 16), + (byte) (body.length >>> 8), (byte) body.length}); + frame.write(new byte[]{0, 0}); + frame.write(body); + byte[] frames = frame.toByteArray(); + + ByteArrayOutputStream mp3 = new ByteArrayOutputStream(); + mp3.write("ID3".getBytes(StandardCharsets.US_ASCII)); + mp3.write(new byte[]{3, 0, 0}); + // the tag size is synchsafe: seven bits per byte + int size = frames.length; + mp3.write(new byte[]{(byte) ((size >>> 21) & 0x7f), (byte) ((size >>> 14) & 0x7f), + (byte) ((size >>> 7) & 0x7f), (byte) (size & 0x7f)}); + mp3.write(frames); + mp3.write(audio); + return mp3.toByteArray(); + } }
