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 5f0102cd030..25bd4afc34e 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 @@ -71,9 +71,13 @@ public ID3v22Handler(ID3v2Frame frame) throws IOException, SAXException, TikaExc case "TCR": copyright = getTagString(tag.data, 0, tag.data.length); break; - case "COM": - comments.add(getComment(tag.data, 0, tag.data.length)); + case "COM": { + ID3Comment comment = getComment(tag.data, 0, tag.data.length); + if (comment != null) { + comments.add(comment); + } break; + } case "TRK": trackNumber = getTagString(tag.data, 0, tag.data.length); break; 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 9d10a5ac293..be3fd4fb118 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 @@ -72,9 +72,13 @@ public ID3v23Handler(ID3v2Frame frame) throws IOException, SAXException, TikaExc case "TCOP": copyright = getTagString(tag.data, 0, tag.data.length); break; - case "COMM": - comments.add(getComment(tag.data, 0, tag.data.length)); + case "COMM": { + ID3Comment comment = getComment(tag.data, 0, tag.data.length); + if (comment != null) { + comments.add(comment); + } break; + } case "TRCK": trackNumber = getTagString(tag.data, 0, tag.data.length); break; 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 f9d7b106a88..cb7fc381657 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 @@ -78,9 +78,13 @@ public ID3v24Handler(ID3v2Frame frame) throws IOException, SAXException, TikaExc case "TCOP": copyright = getTagString(tag.data, 0, tag.data.length); break; - case "COMM": - comments.add(getComment(tag.data, 0, tag.data.length)); + case "COMM": { + ID3Comment comment = getComment(tag.data, 0, tag.data.length); + if (comment != null) { + comments.add(comment); + } break; + } case "TRCK": trackNumber = getTagString(tag.data, 0, tag.data.length); break; 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 1dddd1410b3..3b40a5344b9 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,14 +230,13 @@ protected static String getTagString(byte[] data, int offset, int length) { // (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); @@ -245,10 +244,59 @@ protected static String getTagString(byte[] data, int offset, int length) { } /** - * Builds up the ID3 comment, by parsing and extracting - * the comment string parts from the given data. + * Decodes text in the frame's declared encoding. $01 is UTF-16 with a BOM; if a tagger omits + * it, Java assumes big-endian and turns little-endian text into mojibake ('T' 0x54 0x00 -> + * U+5400), so recover the byte order from the bytes. + */ + 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); + } + + 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); + } + + /** + * Recovers the byte order of BOM-less UTF-16, only reached when a $01 frame omits its + * mandatory BOM (a malformed tagger). Chars below U+0100, which dominate ID3 tags, carry one + * NUL per code unit whose column reveals the order. Chars above (eg CJK) carry no NUL and no + * signal; pure-CJK text then falls back to big-endian - Java's own BOM-less default, so this + * is never worse than the prior unconditional decode. + */ + 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"; + } + + /** + * Parses the comment parts from the given data, or null if the frame is too short or + * malformed to hold a comment. */ protected static ID3Comment getComment(byte[] data, int offset, int length) { + // encoding flag + 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 +312,7 @@ protected static ID3Comment getComment(byte[] data, int offset, int length) { 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 +320,30 @@ protected static ID3Comment getComment(byte[] data, int offset, int length) { // 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 both bytes 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 00000000000..8fee5d672c5 --- /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,171 @@ +/* + * 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 has a NUL per char in UTF-16 (a byte-order signal); CJK 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)))); + } + + // $01 promises a BOM; omitting it used to decode LE as BE mojibake ('T' 0x54 0x00 -> 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)))); + } + + // no NUL bytes (all chars above U+00FF) means no signal, so keep the big-endian default + @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 decodes description and text separately, so each recovers byte order on its own + @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()); + } + + // too short for flag + 3 byte language, or an unknown flag, decodes to null (no overrun) + @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 ending on a lone NUL must not overrun looking for a terminator + @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 935b4be893e..bbe9936e2e5 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 @@ 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.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,54 @@ public void testNakedUTF16BOM() throws Exception { assertEquals("audio/mpeg", metadata.get(Metadata.CONTENT_TYPE)); assertEquals("", metadata.get(XMPDM.GENRE)); } + + // each body used to abort the parse of readable audio (AIOOBE / SIOOBE / NPE) + @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 + new byte[]{5, 'e', 'n', 'g', 'D', 'e', 's', 'c', 0, 'T'}, // 0x05 = unknown encoding + new byte[]{1, 'e', 'n', 'g', 0} // ends before the double byte terminator + }; + 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); + // 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}); + // tag size is synchsafe (7 bits/byte), unlike the plain frame size above + 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(); + } }