8355371: NegativeArraySizeException in print methods in IO or System.console() in JShell #24897

tats-u · 2025-04-26T15:54:40Z

This PR will fix not only JDK-8355371 but also JDK-8354910. I chose the more serious one.

Note: according to JEP-512 (JDK-8344699), java.io.IO is going to stop using System.console().
I confirmed it by checking out and building #24438. (in that PR, the bug does not occur in IO.print(ln))

Before (Temurin 24):

jshell> IO.println("a".repeat(128))
Exception in thread "output reader" java.lang.NegativeArraySizeException: -128
|  State engine terminated.     at jdk.jshell/jdk.jshell.execution.impl.ConsoleImpl$ConsoleOutputStream.readCharsOrNull(ConsoleImpl.java:486)

        at jdk.jshell/jdk.jshell.execution.impl.ConsoleImpl$ConsoleOutputStream.write(ConsoleImpl.java:402)
        at java.base/java.io.OutputStream.write(OutputStream.java:167)
|  Restore definitions with: /reload -restore
        at java.base/java.io.OutputStream.write(OutputStream.java:124)
        at jdk.jshell/jdk.jshell.execution.DemultiplexInput.run(DemultiplexInput.java:74)

jshell> IO.println("ドラえもん")
￉￩えﾂﾓ

jshell> System.console().writer().println("a".repeat(128))
Exception in thread "output reader" java.lang.NegativeArraySizeException: -128
        at jdk.jshell/jdk.jshell.execution.impl.ConsoleImpl$ConsoleOutputStream.readCharsOrNull(ConsoleImpl.java:486)
|  State engine terminated.
        at jdk.jshell/jdk.jshell.execution.impl.ConsoleImpl$ConsoleOutputStream.write(ConsoleImpl.java:402)
        at java.base/java.io.OutputStream.write(OutputStream.java:167)
|  Restore definitions with: /reload -restore   at java.base/java.io.OutputStream.write(OutputStream.java:124)

        at jdk.jshell/jdk.jshell.execution.DemultiplexInput.run(DemultiplexInput.java:74)

jshell> System.console().writer().println("ドラえもん")
￉￩えﾂﾓ

After (master & without #24438):

jshell> IO.println("a".repeat(128))
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

jshell> IO.println("ドラえもん")
ドラえもん

jshell> System.console().writer().println("a".repeat(128))
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

jshell> System.console().writer().println("ドラえもん")
ドラえもん

I don't know how to prepare the template other PRs use or whether outsiders like me may create a PR.

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

Issues

JDK-8355371: NegativeArraySizeException in print methods in IO or System.console() in JShell (Bug - P4)
JDK-8354910: Output by java.io.IO or System.console() corrupted for some non-ASCII characters (Bug - P4)

Reviewers

Chen Liang (@liach - Reviewer)
Jan Lahoda (@lahodaj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24897

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-26T15:55:39Z

Hi @tats-u, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user tats-u" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-04-26T15:56:21Z

@tats-u 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:

8355371: NegativeArraySizeException in print methods in IO or System.console() in JShell
8354910: Output by java.io.IO or System.console() corrupted for some non-ASCII characters

Reviewed-by: liach, jlahoda

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 91 new commits pushed to the master branch:

811f117: 8355980: RISC-V: remove vmclr_m before vmsXX and vmfXX
d29700c: 8344706: Implement JEP 512: Compact Source Files and Instance Main Methods
bd7c778: 8350549: MethodHandleProxies.WRAPPER_TYPES is not thread-safe
... and 88 more: https://git.openjdk.org/jdk/compare/04bb5dcf5759509e0239a6049db9ae2b97880aa9...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@liach, @lahodaj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-04-26T15:56:49Z

@tats-u The following label will be automatically applied to this pull request:

kulla

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.

liach · 2025-04-26T18:30:04Z

src/jdk.jshell/share/classes/jdk/jshell/execution/impl/ConsoleImpl.java

-            buffer[bp++] = b;
+            // Can be negative because directly casted from byte.
+            // java.io.OutputStream.write(int b) stipulates "The 24 high-order bits of b are ignored."
+            buffer[bp++] = b & 0xff;


I think the cause is that OutputStream.write(byte[], int, int) provides negative bytes. I recommend you to update the "can be negative" comment above to be like:

// Can be negative because widening from byte in write(byte[], int, int).

Also note, there is another usage of b below in case READ_CHARS -> but I think it is also a bug; it should be readInt(1).

I think the cause is that OutputStream.write(byte[], int, int) provides negative bytes.

I seem to recall that perhaps so. Your suggestion is better.

it should be readInt(1).

Do you mean int len = readInt(b);? I will update it but unfortunately I have no idea which is correct. Do you have any idea to test that the fix is correct?
I could not tell the difference in jshell by playing with some input.

Yep, that readInt is probably fine because the buffer is large enough, plus the result is unused, so there was no observable consequence.

Also in principle, we will not approve patches unless oca is cleared; meanwhile please update the 2024 last updated year in the license header of 2 files to 2025. (I thought this rule was mentioned in the guide, but apparently it wasnt!)

~~Which notation is the best?~~

2023-2025 (uses the ASCII hypen)

2023–2025 (uses U+2013 EN dash)

2023, 2024, 2025

Looks like "Copyright (c) A, B" means the the closed interval [A, B] (i.e. A–B).

Is "2023, 2025" correct?

Thank you. Does the current change look good to you?

Yes. But I will get a more professional jshell reviewer to double check once the oca is resolved.

liach · 2025-04-26T18:42:50Z

Newcomer notes:

Please check this part of the guide https://openjdk.org/guide/#sign-the-oca or the bot comment for signing the contributor agreement. The robot will automatically update the oca status once you have signed and typed /signed command in a github comment.
Please check our guide https://openjdk.org/guide for more details (though they are too long and many don't apply to newcomers); for example, these files need license header updates. See https://openjdk.org/guide/#making-a-change.
Our bot has a lot of commands. See https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands for details. For example, we have /issue to add other issues fixed by the same PR; you can use that to include the other issue you want.

tats-u · 2025-04-27T04:45:21Z

/signed

bridgekeeper · 2025-04-27T04:46:11Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

tats-u · 2025-04-27T04:50:33Z

/issue JDK-8354910

openjdk · 2025-04-27T04:51:18Z

@tats-u
Adding additional issue to issue list: 8354910: Output by java.io.IO or System.console() corrupted for some non-ASCII characters.

Co-authored-by: Chen Liang <liach@openjdk.org>

mlbridge · 2025-04-29T20:56:15Z

Webrevs

01: Full - Incremental (7225a190)
00: Full (bf0c454a)

liach

Thank you for the wait and the fix!
Since @lahodaj works more closely with JShell, please wait for a review from Lahoda before typing integrate command.

lahodaj

Looks good to me, thanks!

My only slight worry is with the UTF-8 character - please see the inline comment.

test/langtools/jdk/jshell/ConsoleTest.java

tats-u · 2025-05-01T08:44:57Z

@liach Could you review the latest commit again? Sorry for bothering you.

liach

The char update looks good.

tats-u · 2025-05-02T03:32:39Z

@liach Thank you for your approval. Can I run the integrate command or should I wait until @lahodaj 's 2nd review?

liach · 2025-05-02T03:52:19Z

Let's wait for Jan - I think it is holiday for Jan so you might need to wait for a few days.

lahodaj

Looks good to me, thanks!

tats-u · 2025-05-02T08:57:55Z

@lahodaj Thank you for your approval.

/integrate

openjdk · 2025-05-02T08:58:59Z

@tats-u
Your change (at version 7225a19) is now ready to be sponsored by a Committer.

tats-u · 2025-05-02T09:00:53Z

@liach @lahodaj Thank you in advance for /sponsor.

lahodaj · 2025-05-02T15:03:50Z

FWIW, I have ran tests just to be sure, turned out OK. It is a bit late for me right before the weekend, so I am a bit reluctant to sponsor right now. Unless @liach sponsors before that, I'll sponsor on Monday morning.

tats-u · 2025-05-03T06:15:50Z

I got it. Have a nice weekend.

lahodaj · 2025-05-05T05:02:16Z

/sponsor

openjdk · 2025-05-05T05:03:17Z

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

5b3ae92: 8350182: [s390x] Relativize locals in interpreter frames
03f0ec4: 8183348: Better cleanup for jdk/test/sun/security/pkcs12/P12SecretKey.java
4d2d129: 8355962: RISCV64 cross build fails after 8354996
... and 103 more: https://git.openjdk.org/jdk/compare/04bb5dcf5759509e0239a6049db9ae2b97880aa9...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-05T05:03:25Z

@lahodaj @tats-u Pushed as commit c8ce61c.

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

8355371: Ignore 24 high-order bits in ConsoleOutputStream.write

6238309

bridgekeeper bot added the oca Needs verification of OCA signatory status label Apr 26, 2025

openjdk bot added the kulla kulla-dev@openjdk.org label Apr 26, 2025

tats-u changed the title ~~8355371: Ignore 24 high-order bits in ConsoleOutputStream.write~~ 8355371: NegativeArraySizeException in print methods in IO or System.console() in JShell Apr 26, 2025

liach reviewed Apr 26, 2025

View reviewed changes

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Apr 27, 2025

tats-u and others added 3 commits April 27, 2025 14:10

Fix comment and potentially incorrectreadInt argument

693b5a0

Co-authored-by: Chen Liang <liach@openjdk.org>

Update copyright year

5f1900d

Co-authored-by: Chen Liang <liach@openjdk.org>

Merge branch 'master' into fix-io-jshell

bf0c454

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Apr 29, 2025

openjdk bot added the rfr Pull request is ready for review label Apr 29, 2025

liach approved these changes Apr 29, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 29, 2025

lahodaj reviewed Apr 30, 2025

View reviewed changes

test/langtools/jdk/jshell/ConsoleTest.java Outdated Show resolved Hide resolved

Escape katakana

7225a19

openjdk bot removed the ready Pull request is ready to be integrated label Apr 30, 2025

liach approved these changes May 1, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 1, 2025

lahodaj approved these changes May 2, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label May 2, 2025

openjdk bot added the integrated Pull request has been integrated label May 5, 2025

openjdk bot closed this May 5, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 5, 2025

tats-u deleted the fix-io-jshell branch May 5, 2025 05:12

8355371: NegativeArraySizeException in print methods in IO or System.console() in JShell #24897

8355371: NegativeArraySizeException in print methods in IO or System.console() in JShell #24897

Uh oh!

Conversation

tats-u commented Apr 26, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 26, 2025

Uh oh!

openjdk bot commented Apr 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 26, 2025

Uh oh!

liach Apr 26, 2025

Choose a reason for hiding this comment

Uh oh!

tats-u Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

liach Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

tats-u Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liach Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

tats-u Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

liach Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

liach commented Apr 26, 2025

Uh oh!

tats-u commented Apr 27, 2025

Uh oh!

bridgekeeper bot commented Apr 27, 2025

Uh oh!

tats-u commented Apr 27, 2025

Uh oh!

openjdk bot commented Apr 27, 2025

Uh oh!

mlbridge bot commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tats-u commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

tats-u commented May 2, 2025

Uh oh!

liach commented May 2, 2025

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

tats-u commented May 2, 2025

Uh oh!

openjdk bot commented May 2, 2025

Uh oh!

tats-u commented May 2, 2025

Uh oh!

tats-u commented Apr 26, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 26, 2025 •

edited

Loading

tats-u Apr 27, 2025 •

edited

Loading

mlbridge bot commented Apr 29, 2025 •

edited

Loading

tats-u commented May 1, 2025 •

edited

Loading

tats-u commented May 3, 2025 •

edited

Loading