jdk 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook

Making sure restoreEcho correctly reflects the state in the shutdown thread, which differs from the application's thread that issues the readPassword() method.

Progress

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

Issue

JDK-8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook (Bug - P4)

Reviewers

Pavel Rappo (@pavelrappo - Reviewer) ⚠️ Review applies to 3e7d2e0c
Joe Wang (@JoeWang-Java - Reviewer) ⚠️ Review applies to 3e7d2e0c
Stuart Marks (@stuart-marks - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19184

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

Using diff file

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

Webrev

Link to Webrev Comment

May 10 '24 21:05 naotoj

:wave: 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.

May 10 '24 21:05 bridgekeeper[bot]

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

8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook

Reviewed-by: prappo, joehw, smarks

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

cd3e4c03661f770ebeefcd3637d56589243ac0a9: 8326734: text-decoration applied to lost when mixed with or
c2cca2ab443ff87f689810b747985adfdbfbe54a: 8330647: Two CDS tests fail with -UseCompressedOops and UseSerialGC/UseParallelGC
6d2aeb82bc6f8b6894bf3777162be0efb2826397: 8332745: Method::is_vanilla_constructor is never used
cfdc64fcb43e3b261dddc6cc6947235a9e76154e: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. 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.

May 10 '24 21:05 openjdk[bot]

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

May 10 '24 21:05 openjdk[bot]

Webrevs

May 10 '24 22:05 mlbridge[bot]

Thanks, Pavel.

While it's hard to test this change (hence the noreg-hard label), the restoreEcho functionality does not seem to be tested at all. Should we add a straightforward test for it, perhaps separately?

Filed: https://bugs.openjdk.org/browse/JDK-8332161

May 13 '24 17:05 naotoj

Hm, I don't think we want to add any synchronized blocks within a shutdown hook. If a thread is blocked reading from the console, it will hold readLock; if the JVM is then shut down using a signal, it will run shutdown hooks, and this hook will block awaiting readLock. This can deadlock the shutdown completely.

To ensure proper memory visibility, maybe consider making restoreEcho be volatile.

May 13 '24 21:05 stuart-marks

Right, making the field volatile is a much safer solution.

May 13 '24 21:05 naotoj

Hm, I don't think we want to add any synchronized blocks within a shutdown hook. If a thread is blocked reading from the console, it will hold readLock; if the JVM is then shut down using a signal, it will run shutdown hooks, and this hook will block awaiting readLock. This can deadlock the shutdown completely.

To ensure proper memory visibility, maybe consider making restoreEcho be volatile.

It turns out it's not that hard to imagine. Thanks for helping me imagine it, Stuart.

I think that we should add the most straightforward test to demonstrate that the restoreEcho logic (or the lack of it) has an observable effect. If we fail to do that, it might mean that the restoreEcho logic is not needed and that the rest of this message is probably irrelevant.

The test should be added not in a separate PR for 8332161 -- a bug which Naoto has just filed -- but in this PR, and then we should /issue add 8332161.

Since we now have established that concurrency is possible, we should find an adequate concurrent solution. I can imagine that the proposed volatile-based solution may fall short in the following scenario.

Thread 1 reaches the line marked by *. Let's assume that immediately before executing that line restoreEcho is false and echo is on:

public char[] readPassword(String fmt, Object ... args) {
    char[] passwd = null;
    synchronized (writeLock) {
        synchronized(readLock) {
            installShutdownHook();
            try {
                restoreEcho = echo(false); // *
            } catch (IOException x) {
                throw new IOError(x);
            }
            IOError ioe = null;
            try {
                if (!fmt.isEmpty())
                    pw.format(fmt, args);
                passwd = readline(true);
            } catch (IOException x) {
                ioe = new IOError(x);
            } finally {
                try {
                    if (restoreEcho) // **
                        restoreEcho = echo(true);
                } catch (IOException x) {
            ...

Thread 1 executes echo(false), which returns true. true is not yet written into restoreEcho.

Meanwhile, thread 2 (the hook thread) is about to execute the line marked by ***:

public void run() {
    try {
        if (restoreEcho) { // ***
            echo(true);
        }
    } catch (IOException x) { }
}

Thread 2 sees restoreEcho is false and skips echo(true). If thread 1 then ceases to exist (remember, we're shutting down) before it reaches the line marked by **in the finally block, the console echo will remain off.

May 14 '24 09:05 pavelrappo

I thought of the same scenario that is certainly possible. Now I am tempted to avoid this race condition altogether by removing checking restoreEcho and always issue echo(true). What do you guys think?

May 14 '24 13:05 naotoj

I thought of the same scenario that is certainly possible. Now I am tempted to avoid this race condition altogether by removing checking restoreEcho and always issue echo(true). What do you guys think?

It should be possible to provide something adequate based on atomic primitives. However, it would complicate code. So, let's first clarify the problem that restoreEcho is trying to solve: why is it important to restore the echo state upon JVM exit? What would happen if we fail to do that? Can it be demonstrated?

May 14 '24 14:05 pavelrappo

OK, I realized removing checking restoreEcho does not work, here is why.

It should be possible to provide something adequate based on atomic primitives. However, it would complicate code. So, let's first clarify the problem that restoreEcho is trying to solve: why is it important to restore the echo state upon JVM exit? What would happen if we fail to do that? Can it be demonstrated?

restoreEcho, as I understand it, does what it stands, ie., restore the platform console's echo status. Since java.base's Console implementation relies on the platform's echo function, JVM needs to restore the original echo state after JVM quits. So in an unlike situation where the echo is off when the JVM starts, my proposal won't work. We'll still have restoreEcho check in the shutdown thread.

May 14 '24 16:05 naotoj

I think we're assuming here that it's good practice for the Console to snapshot the initial tty state and to restore that state on exit. That's probably right.

If we're really supporting that, though, it means that the Console needs to be prepared to deal with whatever state the tty is in. In particular, if echo is off when the JVM starts, then we'll also need to make sure that ordinary reads turn echo on before actually reading.

And JLine probably also needs to do things like disable canonical input processing (stty -icanon, or "raw" mode, in Unix parlance). Will these be restored on exit? Hm... I just went and looked a bit through JLine, and it installs its own shutdown hooks! I didn't dig further to see exactly what they do though. Probably they're solving a similar problem though.

Unfortunately, shutdown hooks are unordered, so it's possible to that the shutdown hook added at this level will run either before or after JLine's and maybe they'll conflict. So maybe the SPI needs to have tty-save-state and tty-restore-state functions added to it, or something like that, so the responsibility for handling tty stated is pushed down to the provider. But I don't know if JLine could support that. The java.base provider might be able to do something reasonable though.

May 14 '24 17:05 stuart-marks

Thanks, Stuart. The shutdown hook in question is in the java.base's JdkConsole implementation, so the save/restore responsibility is already pushed down to each provider. Thinking that java.base's implementation is now non-default, and the possibility of the race condition is quite low, I now think making restoreEcho as volatile (the current PR) is the most practical solution, unless we could find a reasonably simple way to resolve the contention. BTW, the java.base's shutdown hook has the special slot 0, which is guaranteed to be called first: https://github.com/openjdk/jdk/blob/e91492ab4333c61f39b50eb428fa932131a5b908/src/java.base/share/classes/java/lang/Shutdown.java#L47

May 14 '24 18:05 naotoj

Ah, ok, so each provider has its own shutdown hook, so it's never the case that both hooks are installed at the same time and can conflict with each other. The java.base console provider tracking and restoring echo status on its own is probably fine.

May 14 '24 18:05 stuart-marks

OK, I went over all this again, and it looks pretty good, though there's a potential risk.

In the old code, the restoreEcho variable was usually false, and it was only true while readPassword was running, which then set it back to false. The shutdown hook would therefore almost always see a value of false and decide to do nothing.

In the new code, the restoreEcho variable stores the initial state of the echo status, which is usually true. We don't keep track of whether readPassword() has set the echo status, since that leads to mutable state and race conditions etc. as discussed previously. The only time the shutdown hook doesn't set the echo status is if the initial echo status of the terminal is false, which probably almost never occurs. Thus, the shutdown hook almost always sets the echo status to true regardless of whether it's necessary to do so.

I'm not concerned about the amount of work the shutdown hook is doing. I'm wondering if there's a possibility that setting the terminal modes almost every time on exit is the right thing. It might block, potentially hanging the VM shutdown. ... Reading further, the case I was concerned about was if somebody runs a Java program from a shell and then puts it into the background. Then the JVM shuts down and the shutdown hook tries to enable echo. Does this work, or does it cause the process to be stopped with SIGTTIN or something? And if it works, would it restore echo immediately, even if say a foreground process were reading a password?

May 21 '24 20:05 stuart-marks

Based on an internal discussion regarding Stuart's above comment, I changed the restoreEcho field back to the original. Instead of making it volatile, synchronizing it with a dedicated lock would ensure visibility and atomic status updates (no races).

May 21 '24 22:05 naotoj

I looked at the most recent commit, https://github.com/openjdk/jdk/pull/19184/commits/f69f747a8669647d6f924369fd98b945f9d0ae63.

You are right, the race that we hypothesised previously when restoreEcho was volatile is no longer present. However, there's another race. If it's of any consolation, that new race isn't new at all: it was there before. (I know it's frustrating to be discussing the same problem over and over again, but we're making progress.)

What we want to do is restore the echo state immediately before JVM exits. We achieve this by installing a shutdown hook which restores the state. However, there's no coordination between the shutdown hook and any thread that also modifies the state.

If I read this correctly, due to the mechanics of JVM exit, we simply don't know which thread finishes first: a thread that calls readPassword or the shutdown hook. If the shutdown hook finishes first, then a readPassword thread can corrupt the state: unlike the shutdown hook, which JVM normally has to wait to complete, the readPassword thread can be terminated at any moment. It might as well be terminated before finally but after echo(false), in which case we end up with echo turned off.

What I'm saying is that we should ensure that the shutdown hook has the final say: once completed, no one should modify echo status. I understand that this means more involved communication between threads.

Does it make sense?

May 22 '24 15:05 pavelrappo

Hi Pavel,

If I read this correctly, due to the mechanics of JVM exit, we simply don't know which thread finishes first: a thread that calls readPassword or the shutdown hook.

IIUC, the thread that waits on readPassword() is not a shutdown hook, right? Then I think it is guaranteed that it is still waiting when all the shutdown hooks are executed.

If the shutdown hook finishes first, then a readPassword thread can corrupt the state: unlike the shutdown hook, which JVM normally has to wait to complete, the readPassword thread can be terminated at any moment. It might as well be terminated before finally but after echo(false), in which case we end up with echo turned off.

After the shutdown hooks finish, then the VM is terminated (https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#termination). In there:

finally clauses are not executed;

So I think the shutdown hook's restoreEcho has the final say, sans the situation if some app installs a shutdown hook with readPassword but I don't think we can guarantee that case, and I believe it is OK.

May 22 '24 17:05 naotoj

Hi Pavel,

If I read this correctly, due to the mechanics of JVM exit, we simply don't know which thread finishes first: a thread that calls readPassword or the shutdown hook.

IIUC, the thread that waits on readPassword() is not a shutdown hook, right? Then I think it is guaranteed that it is still waiting when all the shutdown hooks are executed.

If the shutdown hook finishes first, then a readPassword thread can corrupt the state: unlike the shutdown hook, which JVM normally has to wait to complete, the readPassword thread can be terminated at any moment. It might as well be terminated before finally but after echo(false), in which case we end up with echo turned off.

After the shutdown hooks finish, then the VM is terminated (https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/Runtime.html#termination). In there:
finally clauses are not executed;
So I think the shutdown hook's restoreEcho has the final say, sans the situation if some app installs a shutdown hook with readPassword but I don't think we can guarantee that case, and I believe it is OK.

I might be confused, but what if the shutdown hook completes and then some application thread enters readPassword. If that thread manages to turn off echo before all other shutdown hooks complete, it might never execute finally, hence a race.

May 22 '24 18:05 pavelrappo

I might be confused, but what if the shutdown hook completes and then some application thread enters readPassword. If that thread manages to turn off echo before all other shutdown hooks complete, it might never execute finally, hence a race.

Yes, that is possible. However I would say that it is equally unlikely as an app installs a shutdown hook with readPassword(), so I think this is OK too. BTW, if an app issues Runtime.halt() while readPassword() is waiting, then those shutdown hooks aren't even executed, thus ends up in not restoring the echo without a race, which I think is OK as well.

May 22 '24 19:05 naotoj

I might be confused, but what if the shutdown hook completes and then some application thread enters readPassword. If that thread manages to turn off echo before all other shutdown hooks complete, it might never execute finally, hence a race.

Yes, that is possible. However I would say that it is equally unlikely as an app installs a shutdown hook with readPassword(), so I think this is OK too. BTW, if an app issues Runtime.halt() while readPassword() is waiting, then those shutdown hooks aren't even executed, thus ends up in not restoring the echo without a race, which I think is OK as well.

Okay, but can we call it a best-effort attempt to restore the echo state? I guess, it is a judgement call.

May 22 '24 21:05 pavelrappo

Okay, but can we call it a best-effort attempt to restore the echo state? I guess, it is a judgement call.

That would be fair, and exactly what I am aiming for, considering we can do nothing for the halt() case.

May 22 '24 21:05 naotoj

/integrate

May 24 '24 17:05 naotoj

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

b3b33667ad3bdb7be868fb165a1ea53054947cd0: 8332631: Update nsk.share.jpda.BindServer to don't use finalization
f66a58661459bf64212ec332540c12d5d691270f: 8332641: Update nsk.share.jpda.Jdb to don't use finalization
cd3e4c03661f770ebeefcd3637d56589243ac0a9: 8326734: text-decoration applied to lost when mixed with or
c2cca2ab443ff87f689810b747985adfdbfbe54a: 8330647: Two CDS tests fail with -UseCompressedOops and UseSerialGC/UseParallelGC
6d2aeb82bc6f8b6894bf3777162be0efb2826397: 8332745: Method::is_vanilla_constructor is never used
cfdc64fcb43e3b261dddc6cc6947235a9e76154e: 8331291: java.lang.classfile.Attributes class performs a lot of static initializations

Your commit was automatically rebased without conflicts.

May 24 '24 17:05 openjdk[bot]

@naotoj Pushed as commit 236432dbdb9bab4aece54c2fea08f055e5dbf97e.

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

May 24 '24 17:05 openjdk[bot]

jdk jdk copied to clipboard

8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook

Progress

Issue

Reviewers

Reviewing

Webrev

Webrevs

jdk
jdk copied to clipboard