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

naotoj · 2022-06-16T22:21:23Z

This is a regression caused by the fix to JDK-8286287, which assumed the method String.decodeWithDecoder() was only invoked with cs.REPLACE mode based on the comment "should not happen". Possibly this refers to the String(byte[], int, int, Charset) constructor, which specifically mentions the REPLACE mode. However, the method is invoked with String.newStringNoRepl() and it should NOT replace the malformed input (duh!). The fix is to throw an Error for the former case as before the regression, and CharacterCodingException for the latter via an IllegalArgumentException.
In fact, Files.readString() stopped throwing a MalformedInputException since JDK17 with the fix to JDK-8259842, which started throwing an Error.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

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

Reviewers

Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to ea8618fa
Iris Clark (@irisclark - Reviewer) ⚠️ Review applies to ea8618fa
@kristylee88 (no known github.com user name / role) ⚠️ Review applies to e1afd207
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9193/head:pull/9193
$ git checkout pull/9193

Update a local copy of the PR:
$ git checkout pull/9193
$ git pull https://git.openjdk.org/jdk pull/9193/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9193

View PR using the GUI difftool:
$ git pr show -t 9193

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9193.diff

bridgekeeper · 2022-06-16T22:21:50Z

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-06-16T22:24:55Z

@naotoj The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-06-16T22:29:08Z

Webrevs

RogerRiggs

LGTM

openjdk · 2022-06-16T22:47:38Z

@naotoj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: rriggs, iris, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 27 new commits pushed to the master branch:

5b583e4: Merge
6458ebc: 8288988: ProblemList serviceability/jvmti/vthread/ContStackDepthTest/ContStackDepthTest.java in -Xcomp mode
6037ccd: 8288846: misc tests fail "assert(ms < 1000) failed: Un-interruptable sleep, short time use only"
8fa46c8: 8288840: StructureViolationException should not link to fork method
7cf71bc: 8287982: Concurrent implicit attach from native threads crashes VM
9e2d9ac: 8288529: broken link in java.xml
3b1ec3e: 8288994: Incorrect @SInCE tags for @value update in JDK-8286101
58b6937: 8288982: JFR: Log event streaming actions
82c77ca: 8288926: make runtime/logging/DeoptStats.java more reliable
d4de475: 8287868: Localized names update in COMPAT locale provider
... and 17 more: https://git.openjdk.org/jdk/compare/f0804304400f380661a8a06e4c1dad97068fae60...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

AlanBateman · 2022-06-17T10:43:31Z

Files.readString has been broken several times by the changes in to String in this area. Would it be possible to survey the tests that we have for this method, esp. the error cases, to see if we need more tests.

naotoj · 2022-06-17T16:31:16Z

Files.readString has been broken several times by the changes in to String in this area. Would it be possible to survey the tests that we have for this method, esp. the error cases, to see if we need more tests.

I looked for similar test cases but ended up finding nothing. Thus I created a new test case here. Problem is that they are issued through SharedSecrets, which are effectively APIs but treated as private methods which leads to insufficient testing. I now think that I would add not only b2c test, but also c2b test (for getBytesNoRepl() method) is needed. I will modify the test case to include it.

BTW, I found a spec bug in Files.writeString() w/o Charset argument, where the @throws clause reads: "IOException - if an I/O error occurs writing to or creating the file, or the text cannot be encoded using the specified charset", but there is no specified charset there.

AlanBateman · 2022-06-21T08:55:09Z

I looked for similar test cases but ended up finding nothing. Thus I created a new test case here. Problem is that they are issued through SharedSecrets, which are effectively APIs but treated as private methods which leads to insufficient testing. I now think that I would add not only b2c test, but also c2b test (for getBytesNoRepl() method) is needed. I will modify the test case to include it.

My comment was mostly asking if we need to add more tests for Files.writeString. I would have expected a test for that method to fail with this bug. Maybe we need to create a new issue to expand the tests for this method.

BTW, I found a spec bug in Files.writeString() w/o Charset argument, where the @throws clause reads: "IOException - if an I/O error occurs writing to or creating the file, or the text cannot be encoded using the specified charset", but there is no specified charset there.

It looks like description for IOException was copied from the 4-arg writeString to the 3-arg writeString. I've created JDK-8288836 to track this.

AlanBateman · 2022-06-21T08:56:13Z

One other thing, this is a regression in 19 so I assume the PR should be against openjdk/jdk19 rather than the main line.

naotoj · 2022-06-21T18:23:35Z

My comment was mostly asking if we need to add more tests for Files.writeString. I would have expected a test for that method to fail with this bug. Maybe we need to create a new issue to expand the tests for this method.

Added a test case in ReadWriteString.java, which is the unit test for Files.read/writeString() methods.

It looks like description for IOException was copied from the 4-arg writeString to the 3-arg writeString. I've created JDK-8288836 to track this.

Thanks for filing the issue.

One other thing, this is a regression in 19 so I assume the PR should be against openjdk/jdk19 rather than the main line.

Since this PR already got a few approvals, I will backport the changeset to the jdk19 line after this PR gets integrated.

turbanoff · 2022-06-21T20:59:40Z

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

+    private final static String MALFORMED_WINDOWS_1252 = "\u0080\u041e";
+    private final static Charset WINDOWS_1252 = Charset.forName("windows-1252");
+
+    @Test


Looks confusing that annotation goes before javadoc

AlanBateman · 2022-06-22T10:48:47Z

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

+    public void testMalformedReadBytes(byte[] data, Charset csRead, Class<CharacterCodingException> expected)
+            throws IOException {
+        Path path = Files.createTempFile("illegalInputBytes", null);
+        path.toFile().deleteOnExit();


Although this is consistent with the existing tests in this source file, I think it would be better if we change it to create the temporary files in the current directory and not delete it. Leaving the file behind is useful for diagnosing issues where there re test failures. We can do that in a separate issue if you'd like.

Done.

Thanks, that will be helpful if the test fails.

AlanBateman · 2022-06-22T10:50:56Z

One other thing, this is a regression in 19 so I assume the PR should be against openjdk/jdk19 rather than the main line.

Since this PR already got a few approvals, I will backport the changeset to the jdk19 line after this PR gets integrated.

Okay, although doing it this way means there will need to be a PR for openjdk/jdk19 too.

naotoj · 2022-06-22T17:43:27Z

One other thing, this is a regression in 19 so I assume the PR should be against openjdk/jdk19 rather than the main line.

Since this PR already got a few approvals, I will backport the changeset to the jdk19 line after this PR gets integrated.

Okay, although doing it this way means there will need to be a PR for openjdk/jdk19 too.

Yes. It's much easier to backport it than to create a jdk19 PR from scratch, as /backport skara command does the job for me 😀

naotoj · 2022-06-23T15:48:20Z

/integrate

openjdk · 2022-06-23T15:50:02Z

Going to push as commit 2728770.
Since your change was applied there have been 34 commits pushed to the master branch:

ef17ee4: 8288515: (ch) Unnecessary use of Math.addExact() in java.nio.channels.FileLock.overlaps()
72f286a: 8287580: (se) CancelledKeyException during channel registration
b8db0c3: 6980847: (fs) Files.copy needs to be "tuned"
d579916: 8288740: Change incorrect documentation for sjavac flag
26c03c1: 8288719: [arm32] SafeFetch32 thumb interleaving causes random crashes
a802b98: 8287760: --do-not-resolve-by-default gets overwritten if --warn-if-resolved flags is used
bf0623b: 8286314: Trampoline not created for far runtime targets outside small CodeCache
5b583e4: Merge
6458ebc: 8288988: ProblemList serviceability/jvmti/vthread/ContStackDepthTest/ContStackDepthTest.java in -Xcomp mode
6037ccd: 8288846: misc tests fail "assert(ms < 1000) failed: Un-interruptable sleep, short time use only"
... and 24 more: https://git.openjdk.org/jdk/compare/f0804304400f380661a8a06e4c1dad97068fae60...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-06-23T15:50:27Z

@naotoj Pushed as commit 2728770.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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

ea8618f

openjdk bot added the rfr Pull request is ready for review label Jun 16, 2022

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 16, 2022

RogerRiggs approved these changes Jun 16, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 16, 2022

irisclark approved these changes Jun 17, 2022

View reviewed changes

Added c2b test

e1afd20

Adding a test case in ReadWriteString.java

f652050

turbanoff reviewed Jun 21, 2022

View reviewed changes

naotoj added 2 commits June 21, 2022 14:28

Refined the test, adding more variations.

84d127b

Moved @Test annotations

864806e

AlanBateman reviewed Jun 22, 2022

View reviewed changes

AlanBateman approved these changes Jun 22, 2022

View reviewed changes

naotoj added 2 commits June 22, 2022 09:33

Merge branch 'master' into JDK-8288589

6b758d1

Leave the log files in the current directory

09cdf5e

AlanBateman approved these changes Jun 23, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 23, 2022

openjdk bot closed this Jun 23, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

naotoj commented Jun 16, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 16, 2022

openjdk bot commented Jun 16, 2022

mlbridge bot commented Jun 16, 2022 •

edited

Loading

RogerRiggs left a comment

openjdk bot commented Jun 16, 2022 •

edited

Loading

AlanBateman commented Jun 17, 2022

naotoj commented Jun 17, 2022

AlanBateman commented Jun 21, 2022

AlanBateman commented Jun 21, 2022

naotoj commented Jun 21, 2022

turbanoff Jun 21, 2022

naotoj Jun 21, 2022

AlanBateman Jun 22, 2022

naotoj Jun 22, 2022

AlanBateman Jun 23, 2022

AlanBateman commented Jun 22, 2022

naotoj commented Jun 22, 2022

naotoj commented Jun 23, 2022

openjdk bot commented Jun 23, 2022

openjdk bot commented Jun 23, 2022

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

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

Conversation

naotoj commented Jun 16, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 16, 2022

openjdk bot commented Jun 16, 2022

mlbridge bot commented Jun 16, 2022 • edited Loading

Webrevs

RogerRiggs left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 16, 2022 • edited Loading

AlanBateman commented Jun 17, 2022

naotoj commented Jun 17, 2022

AlanBateman commented Jun 21, 2022

AlanBateman commented Jun 21, 2022

naotoj commented Jun 21, 2022

turbanoff Jun 21, 2022

Choose a reason for hiding this comment

naotoj Jun 21, 2022

Choose a reason for hiding this comment

AlanBateman Jun 22, 2022

Choose a reason for hiding this comment

naotoj Jun 22, 2022

Choose a reason for hiding this comment

AlanBateman Jun 23, 2022

Choose a reason for hiding this comment

AlanBateman commented Jun 22, 2022

naotoj commented Jun 22, 2022

naotoj commented Jun 23, 2022

openjdk bot commented Jun 23, 2022

openjdk bot commented Jun 23, 2022

naotoj commented Jun 16, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 16, 2022 •

edited

Loading

openjdk bot commented Jun 16, 2022 •

edited

Loading