TIKA-4784 -- improve utf16 handling in id3 (#2943)

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

@@ -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;

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;

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;

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,73 @@ 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);
         }
     }
 
     /**
-     * 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,36 +312,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 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

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());
+    }
+}