TIKA-4784 -- improve utf16 handling in id3

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,13 @@ private ID3Comment getComment(byte[] data, int offset, int length) {
         return ID3v2Frame.getComment(data, offset, length);
     }
 
+    /** Skips null comments (malformed frames) that would trip up 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,13 @@ private ID3Comment getComment(byte[] data, int offset, int length) {
         return ID3v2Frame.getComment(data, offset, length);
     }
 
+    /** Skips null comments (malformed frames) that would trip up 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,13 @@ private ID3Comment getComment(byte[] data, int offset, int length) {
         return ID3v2Frame.getComment(data, offset, length);
     }
 
+    /** Skips null comments (malformed frames) that would trip up 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,74 @@ 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. 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 {
+        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 offset start with a UTF-16 BOM? */
+    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);
+    }
+
+    /**
+     * 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;
+        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.
+     * 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) {
+        // need at least an 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 +313,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());
+    }
+}