TIKA-4784: fix unicode in id3

Author: tballison

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v22Handler.java

@@ -113,10 +113,7 @@ private ID3Comment getComment(byte[] data, int offset, int length) {
         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.
-     */
+    /** Skips null comments (malformed frames) that would trip up consumers. */
     private void addComment(ID3Comment comment) {
         if (comment != null) {
             comments.add(comment);

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v23Handler.java

@@ -99,10 +99,7 @@ private ID3Comment getComment(byte[] data, int offset, int length) {
         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.
-     */
+    /** Skips null comments (malformed frames) that would trip up consumers. */
     private void addComment(ID3Comment comment) {
         if (comment != null) {
             comments.add(comment);

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v24Handler.java

@@ -105,10 +105,7 @@ private ID3Comment getComment(byte[] data, int offset, int length) {
         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.
-     */
+    /** Skips null comments (malformed frames) that would trip up consumers. */
     private void addComment(ID3Comment comment) {
         if (comment != null) {
             comments.add(comment);

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/parser/mp3/ID3v2Frame.java

@@ -244,13 +244,9 @@ protected static String getTagString(byte[] data, int offset, int length) {
     }
 
     /**
-     * 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.
+     * Decodes text in the frame's declared encoding. Encoding {@code $01} is UTF-16 with a BOM;
+     * when a tagger omits it Java assumes big-endian and turns little-endian text into mojibake
+     * ({@code 'T' 0x54 0x00} decodes to U+5400), so recover the byte order from the bytes.
      */
     private static String decodeText(byte[] data, int offset, int length, TextEncoding encoding)
             throws UnsupportedEncodingException {
@@ -261,9 +257,7 @@ private static String decodeText(byte[] data, int offset, int length, TextEncodi
         return new String(data, offset, length, charset);
     }
 
-    /**
-     * Does the text at the given offset start with a UTF-16 byte order mark?
-     */
+    /** Does the text at the offset start with a UTF-16 BOM? */
     private static boolean hasBOM(byte[] data, int offset, int length) {
         if (length < 2) {
             return false;
@@ -274,10 +268,10 @@ private static boolean hasBOM(byte[] data, int offset, int length) {
     }
 
     /**
-     * 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.
+     * Byte order of BOM-less UTF-16, from which column holds each code unit's NUL: chars below
+     * U+0100 dominate ID3 tags and encode as {@code 0x00 lo} (BE) or {@code lo 0x00} (LE). No
+     * NULs (eg CJK) means no signal, so keep the big-endian default. Deterministic single-field
+     * form of {@code Utf16ColumnFeatureExtractor} features 0/1.
      */
     private static String guessUTF16ByteOrder(byte[] data, int offset, int length) {
         int bigEndian = 0;
@@ -295,11 +289,11 @@ private static String guessUTF16ByteOrder(byte[] data, int offset, int length) {
 
     /**
      * 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.
+     * the comment string 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) {
-        // A comment is at minimum an encoding flag and a 3 byte language
+        // need at least an encoding flag + 3 byte language
         if (length < 4) {
             return null;
         }
@@ -328,7 +322,7 @@ protected static ID3Comment getComment(byte[] data, int offset, int length) {
         // Find where the description ends
         try {
             for (int i = descStart; i < end; i++) {
-                // A double byte terminator needs two bytes to be present
+                // 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 < end && data[i + 2] == 0) {

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/ID3v2FrameTest.java

@@ -44,10 +44,7 @@ public class ID3v2FrameTest {
     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.
-     */
+    // 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 = "日本語";
 
@@ -88,21 +85,14 @@ public void testUTF16BEWithoutBOMFlag() {
         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).
-     */
+    // $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))));
     }
 
-    /**
-     * 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.
-     */
+    // 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))));
@@ -118,19 +108,14 @@ public void testNullTerminationIsTrimmed() {
         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.
-     */
+    // 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.
-     */
+    // COMM decodes description and text separately, so each recovers byte order on its own
     @Test
     public void testCommentWithoutBOMRecoversByteOrder() throws IOException {
         ByteArrayOutputStream out = new ByteArrayOutputStream();
@@ -165,11 +150,7 @@ public void testCommentWithBOM() throws IOException {
         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.
-     */
+    // 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));
@@ -180,10 +161,7 @@ public void testMalformedCommentsReturnNull() {
         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.
-     */
+    // 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};

tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/test/java/org/apache/tika/parser/mp3/Mp3ParserTest.java

@@ -368,21 +368,15 @@ public void testNakedUTF16BOM() throws Exception {
         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.
-     */
+    // 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
-                // 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}
+                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;
@@ -391,14 +385,12 @@ public void testMalformedCommentFrameIsSkipped() throws Exception {
                 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
+            // 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.
-     */
+    // 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")) {
@@ -417,7 +409,7 @@ private byte[] mp3WithFrame(String frameId, byte[] body) throws Exception {
         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
+        // 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)});