8288589: Files.readString ignores encoding errors for UTF-16

Reviewed-by: rriggs, iris, alanb

src/java.base/share/classes/java/lang/String.java

@@ -658,14 +658,22 @@ public String(byte[] bytes, int offset, int length, Charset charset) {
 
             // decode using CharsetDecoder
             int en = scale(length, cd.maxCharsPerByte());
+            cd.onMalformedInput(CodingErrorAction.REPLACE)
+                    .onUnmappableCharacter(CodingErrorAction.REPLACE);
             char[] ca = new char[en];
             if (charset.getClass().getClassLoader0() != null &&
                     System.getSecurityManager() != null) {
                 bytes = Arrays.copyOfRange(bytes, offset, offset + length);
                 offset = 0;
             }
 
-            int caLen = decodeWithDecoder(cd, ca, bytes, offset, length);
+            int caLen;
+            try {
+                caLen = decodeWithDecoder(cd, ca, bytes, offset, length);
+            } catch (CharacterCodingException x) {
+                // Substitution is enabled, so this shouldn't happen
+                throw new Error(x);
+            }
             if (COMPACT_STRINGS) {
                 byte[] bs = StringUTF16.compress(ca, 0, caLen);
                 if (bs != null) {
@@ -791,7 +799,13 @@ private static String newStringNoRepl1(byte[] src, Charset cs) {
                 System.getSecurityManager() != null) {
             src = Arrays.copyOf(src, len);
         }
-        int caLen = decodeWithDecoder(cd, ca, src, 0, src.length);
+        int caLen;
+        try {
+            caLen = decodeWithDecoder(cd, ca, src, 0, src.length);
+        } catch (CharacterCodingException x) {
+            // throw via IAE
+            throw new IllegalArgumentException(x);
+        }
         if (COMPACT_STRINGS) {
             byte[] bs = StringUTF16.compress(ca, 0, caLen);
             if (bs != null) {
@@ -1199,23 +1213,16 @@ private static int decodeUTF8_UTF16(byte[] src, int sp, int sl, byte[] dst, int
         return dp;
     }
 
-    private static int decodeWithDecoder(CharsetDecoder cd, char[] dst, byte[] src, int offset, int length) {
+    private static int decodeWithDecoder(CharsetDecoder cd, char[] dst, byte[] src, int offset, int length)
+                                            throws CharacterCodingException {
         ByteBuffer bb = ByteBuffer.wrap(src, offset, length);
         CharBuffer cb = CharBuffer.wrap(dst, 0, dst.length);
-        cd.onMalformedInput(CodingErrorAction.REPLACE)
-            .onUnmappableCharacter(CodingErrorAction.REPLACE);
-        try {
-            CoderResult cr = cd.decode(bb, cb, true);
-            if (!cr.isUnderflow())
-                cr.throwException();
-            cr = cd.flush(cb);
-            if (!cr.isUnderflow())
-                cr.throwException();
-        } catch (CharacterCodingException x) {
-            // Substitution is always enabled,
-            // so this shouldn't happen
-            throw new Error(x);
-        }
+        CoderResult cr = cd.decode(bb, cb, true);
+        if (!cr.isUnderflow())
+            cr.throwException();
+        cr = cd.flush(cb);
+        if (!cr.isUnderflow())
+            cr.throwException();
         return cb.position();
     }
 

test/jdk/java/lang/String/NoReplTest.java

@@ -0,0 +1,85 @@
+/*
+ * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8286287 8288589
+ * @summary Tests for *NoRepl() shared secret methods.
+ * @run testng NoReplTest
+ * @modules jdk.charsets
+ */
+
+import java.io.IOException;
+import java.nio.charset.CharacterCodingException;
+import java.nio.charset.Charset;
+import java.nio.file.Files;
+import java.util.HexFormat;
+import static java.nio.charset.StandardCharsets.UTF_16;
+
+import org.testng.annotations.Test;
+
+@Test
+public class NoReplTest {
+    private final static byte[] MALFORMED_UTF16 = {(byte)0x00, (byte)0x20, (byte)0x00};
+    private final static String MALFORMED_WINDOWS_1252 = "\u0080\u041e";
+    private final static Charset WINDOWS_1252 = Charset.forName("windows-1252");
+
+    /**
+     * Verifies newStringNoRepl() throws a CharacterCodingException.
+     * The method is invoked by `Files.readString()` method.
+     */
+    @Test
+    public void newStringNoReplTest() throws IOException {
+        var f = Files.createTempFile(null, null);
+        try (var fos = Files.newOutputStream(f)) {
+            fos.write(MALFORMED_UTF16);
+            var read = Files.readString(f, UTF_16);
+            throw new RuntimeException("Exception should be thrown for a malformed input. Bytes read: " +
+                    HexFormat.of()
+                            .withPrefix("x")
+                            .withUpperCase()
+                            .formatHex(read.getBytes(UTF_16)));
+        } catch (CharacterCodingException cce) {
+            // success
+        } finally {
+            Files.delete(f);
+        }
+    }
+
+    /**
+     * Verifies getBytesNoRepl() throws a CharacterCodingException.
+     * The method is invoked by `Files.writeString()` method.
+     */
+    @Test
+    public void getBytesNoReplTest() throws IOException {
+        var f = Files.createTempFile(null, null);
+        try {
+            Files.writeString(f, MALFORMED_WINDOWS_1252, WINDOWS_1252);
+            throw new RuntimeException("Exception should be thrown");
+        } catch (CharacterCodingException cce) {
+            // success
+        } finally {
+            Files.delete(f);
+        }
+    }
+}

test/jdk/java/nio/file/Files/ReadWriteString.java

@@ -24,10 +24,12 @@
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.nio.charset.Charset;
+import java.nio.charset.CharacterCodingException;
 import java.nio.charset.MalformedInputException;
 import java.nio.charset.UnmappableCharacterException;
-import static java.nio.charset.StandardCharsets.US_ASCII;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
+import static java.nio.charset.StandardCharsets.US_ASCII;
+import static java.nio.charset.StandardCharsets.UTF_16;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import java.nio.file.Files;
 import java.nio.file.OpenOption;
@@ -46,7 +48,7 @@
 import org.testng.annotations.Test;
 
 /* @test
- * @bug 8201276 8205058 8209576 8287541
+ * @bug 8201276 8205058 8209576 8287541 8288589
  * @build ReadWriteString PassThroughFileSystem
  * @run testng ReadWriteString
  * @summary Unit test for methods for Files readString and write methods.
@@ -60,6 +62,8 @@ public class ReadWriteString {
     final String TEXT_UNICODE = "\u201CHello\u201D";
     final String TEXT_ASCII = "ABCDEFGHIJKLMNOPQRSTUVWXYZ\n abcdefghijklmnopqrstuvwxyz\n 1234567890\n";
     private static final String JA_STRING = "\u65e5\u672c\u8a9e\u6587\u5b57\u5217";
+    private static final Charset WINDOWS_1252 = Charset.forName("windows-1252");
+    private static final Charset WINDOWS_31J = Charset.forName("windows-31j");
 
     static byte[] data = getData();
 
@@ -88,14 +92,14 @@ static byte[] getData() {
      */
     @DataProvider(name = "malformedWrite")
     public Object[][] getMalformedWrite() throws IOException {
-        Path path = Files.createTempFile("malformedWrite", null);
+        Path path = Files.createFile(Path.of("malformedWrite"));
         return new Object[][]{
             {path, "\ud800", null},  //the default Charset is UTF_8
             {path, "\u00A0\u00A1", US_ASCII},
             {path, "\ud800", UTF_8},
             {path, JA_STRING, ISO_8859_1},
-            {path, "\u041e", Charset.forName("windows-1252")}, // cyrillic capital letter O
-            {path, "\u091c", Charset.forName("windows-31j")}, // devanagari letter ja
+            {path, "\u041e", WINDOWS_1252}, // cyrillic capital letter O
+            {path, "\u091c", WINDOWS_31J}, // devanagari letter ja
         };
     }
 
@@ -105,13 +109,26 @@ public Object[][] getMalformedWrite() throws IOException {
      */
     @DataProvider(name = "illegalInput")
     public Object[][] getIllegalInput() throws IOException {
-        Path path = Files.createTempFile("illegalInput", null);
+        Path path = Files.createFile(Path.of("illegalInput"));
         return new Object[][]{
             {path, data, ISO_8859_1, null},
             {path, data, ISO_8859_1, UTF_8}
         };
     }
 
+    /*
+     * DataProvider for illegal input bytes test
+     */
+    @DataProvider(name = "illegalInputBytes")
+    public Object[][] getIllegalInputBytes() throws IOException {
+        return new Object[][]{
+            {new byte[] {(byte)0x00, (byte)0x20, (byte)0x00}, UTF_16, MalformedInputException.class},
+            {new byte[] {-50}, UTF_16, MalformedInputException.class},
+            {new byte[] {(byte)0x81}, WINDOWS_1252, UnmappableCharacterException.class}, // unused in Cp1252
+            {new byte[] {(byte)0x81, (byte)0xff}, WINDOWS_31J, UnmappableCharacterException.class}, // invalid trailing byte
+        };
+    }
+
     /*
      * DataProvider for writeString test
      * Writes the data using both the existing and new method and compares the results.
@@ -143,16 +160,9 @@ public Object[][] getReadString() {
 
     @BeforeClass
     void setup() throws IOException {
-        testFiles[0] = Files.createTempFile("readWriteString", null);
-        testFiles[1] = Files.createTempFile("writeString_file1", null);
-        testFiles[2] = Files.createTempFile("writeString_file2", null);
-    }
-
-    @AfterClass
-    void cleanup() throws IOException {
-        for (Path path : testFiles) {
-            Files.deleteIfExists(path);
-        }
+        testFiles[0] = Files.createFile(Path.of("readWriteString"));
+        testFiles[1] = Files.createFile(Path.of("writeString_file1"));
+        testFiles[2] = Files.createFile(Path.of("writeString_file2"));
     }
 
     /**
@@ -241,11 +251,10 @@ public void testReadString(Path path, String text, Charset cs, Charset cs2) thro
      */
     @Test(dataProvider = "malformedWrite", expectedExceptions = UnmappableCharacterException.class)
     public void testMalformedWrite(Path path, String s, Charset cs) throws IOException {
-        path.toFile().deleteOnExit();
         if (cs == null) {
-            Files.writeString(path, s, CREATE);
+            Files.writeString(path, s);
         } else {
-            Files.writeString(path, s, cs, CREATE);
+            Files.writeString(path, s, cs);
         }
     }
 
@@ -261,16 +270,40 @@ public void testMalformedWrite(Path path, String s, Charset cs) throws IOExcepti
      */
     @Test(dataProvider = "illegalInput", expectedExceptions = MalformedInputException.class)
     public void testMalformedRead(Path path, byte[] data, Charset csWrite, Charset csRead) throws IOException {
-        path.toFile().deleteOnExit();
         String temp = new String(data, csWrite);
-        Files.writeString(path, temp, csWrite, CREATE);
+        Files.writeString(path, temp, csWrite);
         if (csRead == null) {
             Files.readString(path);
         } else {
             Files.readString(path, csRead);
         }
     }
 
+    /**
+     * Verifies that IOException is thrown when reading a file containing
+     * illegal bytes
+     *
+     * @param data the data used for the test
+     * @param csRead the Charset to use for reading the file
+     * @param expected exception class
+     * @throws IOException when the Charset used for reading the file is incorrect
+     */
+    @Test(dataProvider = "illegalInputBytes")
+    public void testMalformedReadBytes(byte[] data, Charset csRead, Class<CharacterCodingException> expected)
+            throws IOException {
+        Path path = Path.of("illegalInputBytes");
+        Files.write(path, data);
+        try {
+            Files.readString(path, csRead);
+        } catch (MalformedInputException | UnmappableCharacterException e) {
+            if (expected.isInstance(e)) {
+                // success
+                return;
+            }
+        }
+        throw new RuntimeException("An instance of " + expected + " should be thrown");
+    }
+
     private void checkNullPointerException(Callable<?> c) {
         try {
             c.call();