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

@@ -72,7 +72,7 @@ public ID3v22Handler(ID3v2Frame frame) throws IOException, SAXException, TikaExc
                     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 @@ 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.
+     */
+    private void addComment(ID3Comment comment) {
+        if (comment != null) {
+            comments.add(comment);
+        }
+    }
+
     public boolean getTagsPresent() {
         return true;
     }

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 ID3v23Handler(ID3v2Frame frame) throws IOException, SAXException, TikaExc
                     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 @@ 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.
+     */
+    private void addComment(ID3Comment comment) {
+        if (comment != null) {
+            comments.add(comment);
+        }
+    }
+
     public boolean getTagsPresent() {
         return true;
     }

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 ID3v24Handler(ID3v2Frame frame) throws IOException, SAXException, TikaExc
                     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 @@ 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.
+     */
+    private void addComment(ID3Comment comment) {
+        if (comment != null) {
+            comments.add(comment);
+        }
+    }
+
     public boolean getTagsPresent() {
         return true;
     }

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 @@ 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);
         }
     }
 
+    /**
+     * 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,36 +319,38 @@ 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;
         String text = null;
 
         // 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