Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8261785: Calling "main" method in anonymous nested class crashes the JVM #2999

Closed
wants to merge 3 commits into from

Conversation

@slowhog
Copy link
Contributor

@slowhog slowhog commented Mar 14, 2021

This patch ensure launcher won't crash JVM for the new static Methods from local/anonymous class on MacOS.

As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when the launcher trying to grab class name to be displayed as the Application name on the menu.

The fix is to not setting name, test shows that GUI java application shows 'bin' as the application name. It's possible for us to set the name to something more friendly, for example, "Java", but I am not sure that should be launcher's responsibility to choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible to pick such name in case the environment variable is not set.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8261785: Calling "main" method in anonymous nested class crashes the JVM

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2999/head:pull/2999
$ git checkout pull/2999

To update a local copy of the PR:
$ git checkout pull/2999
$ git pull https://git.openjdk.java.net/jdk pull/2999/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 14, 2021

👋 Welcome back henryjen! 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 openjdk bot added the rfr label Mar 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 14, 2021

@slowhog The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-runtime

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 14, 2021

Webrevs

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 15, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Henry,

On 15/03/2021 9:40 am, Henry Jen wrote:

This patch ensure launcher won't crash JVM for the new static Methods from local/anonymous class on MacOS.

As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when the launcher trying to grab class name to be displayed as the Application name on the menu.

The fix is to not setting name, test shows that GUI java application shows 'bin' as the application name. It's possible for us to set the name to something more friendly, for example, "Java", but I am not sure that should be launcher's responsibility to choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible to pick such name in case the environment variable is not set.

Maybe the AWT folk should decide what name should be displayed in this
case. The canonical name was chosen when all main classes had to have a
canonical name. So perhaps a simple name will suffice in the case where
there is no canonical name?

Cheers,
David

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 15, 2021

/cc awt

@openjdk openjdk bot added the awt label Mar 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@mrserb
The awt label was successfully added.

Copy link
Member

@mrserb mrserb left a comment

This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine.

@@ -0,0 +1,43 @@
import java.io.IOException;

This comment has been minimized.

@mrserb

mrserb Mar 16, 2021
Member

Copyright?

This comment has been minimized.

@slowhog

slowhog Mar 16, 2021
Author Contributor

This file is mostly based on the bug report as I just adjust static keyword to make sure we cover different cases, thus I am not sure whether to add copyright or not.

This comment has been minimized.

@mrserb

mrserb Mar 16, 2021
Member

you need to clarify this, if we cannot add copyright here, we should not use this file.

* Copyright (c) 2021, 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

This comment has been minimized.

@mrserb

mrserb Mar 16, 2021
Member

Looks like formatting much wider than usual

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 16, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 16/03/2021 11:58 am, Sergey Bylokhov wrote:

On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen <henryjen at openjdk.org> wrote:

This patch ensure launcher won't crash JVM for the new static Methods from local/anonymous class on MacOS.

As @dholmes-ora pointed out in the analysis, this is a MacOS specific bug when the launcher trying to grab class name to be displayed as the Application name on the menu.

The fix is to not setting name, test shows that GUI java application shows 'bin' as the application name. It's possible for us to set the name to something more friendly, for example, "Java", but I am not sure that should be launcher's responsibility to choose such a default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d environment variable should be responsible to pick such name in case the environment variable is not set.

This bug is similar to https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine.

Both issues involve a problem trying to use the canonical name, but I'd
consider both fixes deficient when an alternative name could be used.
But this isn't my code so ...

David

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 16, 2021

Maybe the AWT folk should decide what name should be displayed in this
case. The canonical name was chosen when all main classes had to have a
canonical name. So perhaps a simple name will suffice in the case where
there is no canonical name?

This is not the last attempt to set the name, the JAVA_MAIN_CLASS_ variable is used in the middle of the name selection, there are some others. And the "bin" is selected by some of the next step, I agree it is not a friendly name that could be improved.

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Mar 16, 2021

Using an anonymous class for the main class looks strange and hard to believe anyone is relying on this. I wonder if we should do more checking LauncherHelper.validateMainClass to reject cases like this.

@slowhog
Copy link
Contributor Author

@slowhog slowhog commented Mar 16, 2021

Using an anonymous class for the main class looks strange and hard to believe anyone is relying on this. I wonder if we should do more checking LauncherHelper.validateMainClass to reject cases like this.

I raised that same question, and people tends to agree launcher could reject anonymous/local classes. But pointed out that should require a CSR review. Therefore I chose to fix crash first, and we can file another ticket to address main class requirements.

This is not the last attempt to set the name, the JAVA_MAIN_CLASS_ variable is used in the middle of the name selection, there are some others. And the "bin" is selected by some of the next step, I agree it is not a friendly name that could be improved.

I tried to do a quick search on JAVA_MAIN_CLASS_%pid variable, didn't find other code to set this. I had a version that would set the variable to "Java", I can extend that to cover exception case as well.

slowhog added 2 commits Mar 16, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 17, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 16/03/2021 2:59 pm, David Holmes wrote:

On 16/03/2021 11:58 am, Sergey Bylokhov wrote:

On Sun, 14 Mar 2021 23:34:55 GMT, Henry Jen <henryjen at openjdk.org> wrote:

This patch ensure launcher won't crash JVM for the new static Methods
from local/anonymous class on MacOS.

As @dholmes-ora pointed out in the analysis, this is a MacOS specific
bug when the launcher trying to grab class name to be displayed as
the Application name on the menu.

The fix is to not setting name, test shows that GUI java application
shows 'bin' as the application name. It's possible for us to set the
name to something more friendly, for example, "Java", but I am not
sure that should be launcher's responsibility to choose such a
default name. It seems to me the consumer of the JAVA_MAIN_CLASS_%d
environment variable should be responsible to pick such name in case
the environment variable is not set.

This bug is similar to
https://bugs.openjdk.java.net/browse/JDK-8076264, and the fix looks fine.

Both issues involve a problem trying to use the canonical name, but I'd
consider both fixes deficient when an alternative name could be used.

Except I overlooked that this is an anonymous class so no simple name
either. I agree with Henry's later proposal - fix the crash simply then
outlaw the usecase later.

Cheers,
David

But this isn't my code so ...

David

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Mar 17, 2021

Using an anonymous class for the main class looks strange and hard to believe anyone is relying on this. I wonder if we should do more checking LauncherHelper.validateMainClass to reject cases like this.

I raised that same question, and people tends to agree launcher could reject anonymous/local classes. But pointed out that should require a CSR review. Therefore I chose to fix crash first, and we can file another ticket to address main class requirements.

The requirement for a CSR and release note should not be a concern here. I don't object to the fix but I think it would be very useful to document the main class and whether local or anonymous classes can be used, its accessibility, and the accessibility of the main method. We had to do something similar recently with the premain method (java.lang.instrument).

@slowhog
Copy link
Contributor Author

@slowhog slowhog commented Mar 17, 2021

Using an anonymous class for the main class looks strange and hard to believe anyone is relying on this. I wonder if we should do more checking LauncherHelper.validateMainClass to reject cases like this.

I raised that same question, and people tends to agree launcher could reject anonymous/local classes. But pointed out that should require a CSR review. Therefore I chose to fix crash first, and we can file another ticket to address main class requirements.

The requirement for a CSR and release note should not be a concern here. I don't object to the fix but I think it would be very useful to document the main class and whether local or anonymous classes can be used, its accessibility, and the accessibility of the main method. We had to do something similar recently with the premain method (java.lang.instrument).

Absolutely we need to clarify that, however, the discussion of what should or should not be allowed may take some time. For example, if we limit such usage in launcher, should it be possible for custom launcher to start such a main method? Thus the recommendation to have a separate ticket for that.

The current java document mostly only say to load the specify the class name, however there is a sentence By default, the first argument that isn't an option of the java command is the fully qualified name of the class to be called. If -jar is specified, then its argument is the name of the JAR file containing class and resource files for the application. The startup class must be indicated by the Main-Class manifest header in its manifest file.. Not that it says fully qualified name of the class.

Filed JDK-8263735 for the follow up, in the mean time, we should get this crash resolved.

@mrserb
mrserb approved these changes Mar 18, 2021
Copy link
Member

@mrserb mrserb left a comment

Looks fine from the client point of view.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

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

8261785: Calling "main" method in anonymous nested class crashes the JVM

Reviewed-by: serb

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:

  • 6aa28b3: 8263827: Suspend "missing" javadoc doclint checks for smartcardio
  • ed1e25d: 8263833: Stop disabling warnings for sunFont.c with gcc
  • 788e30c: 8263320: [test] Add Object Stream Formatter to work with test utility HexPrinter
  • fa0f161: 8263742: (bf) MappedByteBuffer.force() should use the capacity as its upper bound
  • c82a673: 8262001: java/lang/management/ThreadMXBean/ResetPeakThreadCount.java failed with "RuntimeException: Current Peak = 14 Expected to be == previous peak = 7 + 8"
  • 01ddf3d: 8263622: The java.awt.color.ICC_Profile#setData invert the order of bytes for the "head" tag
  • e34f766: 8252723: Run stack016.java also with C2-only
  • 2173fed: 8263439: getSupportedAttributeValues() throws NPE for Finishings attribute
  • e543a50: 8261352: Create implementation for component peer for all the components who should be ignored in a11y interactions
  • 21db0f6: 8263659: Reflow GTestResultParser for better readability
  • ... and 81 more: https://git.openjdk.java.net/jdk/compare/da9ead5e7fec1facdb4e44fc0a5b872edb704b9a...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.

@openjdk openjdk bot added the ready label Mar 18, 2021
@slowhog
Copy link
Contributor Author

@slowhog slowhog commented Mar 22, 2021

/integrate

@openjdk openjdk bot closed this Mar 22, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 22, 2021

@slowhog Since your change was applied there have been 128 commits pushed to the master branch:

  • 840ab7b: 8263894: Convert defaultPrinter and printers fields to local variables
  • ba504fc: 8187450: JNI local refs exceeds capacity warning in NetworkInterface::getAll
  • 0abbfb2: 8263729: [test] divert spurious output away from stream under test in ProcessBuilder Basic test
  • 6c2220e: 8263861: Shenandoah: Remove unused member in ShenandoahGCStateResetter
  • 5262d95: 8263855: Use the blessed modifier order in java.management/naming
  • 6f1bcb0: 8263593: Fix multiple typos in hsdis README
  • a9d2267: 8260589: Crash in JfrTraceIdLoadBarrier::load(_jclass*)
  • 42104e5: 8263488: Verify CWarningWindow works with metal rendering pipeline
  • 5a7f22a: 8263579: ZGC: Concurrent mark hangs with debug loglevel
  • 35cd945: 8263908: Build fails due to initialize_static_field_for_dump defined but not used after JDK-8263771
  • ... and 118 more: https://git.openjdk.java.net/jdk/compare/da9ead5e7fec1facdb4e44fc0a5b872edb704b9a...master

Your commit was automatically rebased without conflicts.

Pushed as commit b2df513.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants