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

8268349: Provide more detail in JEP 411 warning messages #4400

Closed
wants to merge 1 commit into from

Conversation

wangweij
Copy link
Contributor

@wangweij wangweij commented Jun 7, 2021

More loudly and precise warning messages when a security manager is either enabled at startup or installed at runtime.


Progress

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

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issue

  • JDK-8268349: Provide more detail in JEP 411 warning messages

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/4400
$ git pull https://git.openjdk.java.net/jdk pull/4400/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4400

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4400.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 7, 2021

👋 Welcome back weijun! 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 Pull request is ready for review label Jun 7, 2021
@openjdk
Copy link

openjdk bot commented Jun 7, 2021

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

  • core-libs
  • net

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.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org labels Jun 7, 2021
@wangweij
Copy link
Contributor Author

wangweij commented Jun 7, 2021

/label add security

@wangweij
Copy link
Contributor Author

wangweij commented Jun 7, 2021

/csr needed

@openjdk openjdk bot added the security security-dev@openjdk.org label Jun 7, 2021
@openjdk
Copy link

openjdk bot commented Jun 7, 2021

@wangweij
The security label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Jun 7, 2021

Webrevs

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Jun 7, 2021
@openjdk
Copy link

openjdk bot commented Jun 7, 2021

@wangweij has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@wangweij please create a CSR request for issue JDK-8268349. This pull request cannot be integrated until the CSR request is approved.

WARNING: A Security Manager implementation has been enabled on the command line
WARNING: java.lang.SecurityManager is deprecated and will be removed in a future release
WARNING: -Djava.security.manager=%s will have no effect when java.lang.SecurityManager is removed
""", smProp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raw strings may be useful here but means the lines length are inconsistent and makes it too hard to look at side by side diffs now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you mean when I switch to Split View. While I can extract the lines to a method, I somehow think it's not worth doing because for each type of warning the method is only called once.

@@ -324,6 +327,22 @@ private static void checkIO() {
private static native void setOut0(PrintStream out);
private static native void setErr0(PrintStream err);

// Remember original System.err. setSecurityManager() warning goes here
private static PrintStream oldErrStream = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this should needs to be volatile and @stable. I think we need a better name for it too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add the modifiers. How about "originalErr"?

private static class CallerHolder {
// Remember callers of setSecurityManager() here so that warning
// is only printed once for each different caller
final static Map<String, Boolean> callersOfSSM = new WeakHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use a WeakHashMap without synchronization but a big question here is whether a single caller frame is sufficient. If I were doing this then I think I would capture the hash of a number of stack frames to create a better filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but not sure of performance impact. Is the worst problem that more than one warnings will be printed for a single caller? It's not really harmless.

As for the frame, if the warning message only contain the caller class name and its code source, why is it worth using a key of multiple frames? The message will look the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that but not sure of performance impact. Is the worst problem that more than one warnings will be printed for a single caller? It's not really harmless.

As for the frame, if the warning message only contain the caller class name and its code source, why is it worth using a key of multiple frames? The message will look the same.

WeakHashMap access needs synchronization. Whether we need to cache to avoid excessive warnings isn't clear. If the SM is enabled once and never disabled/re-enabled then caching isn't interesting. On the other hand if there are programs that are enabling/disabling to execute subsets of code then maybe it is. Maybe we should just drop this and see if there is any feedback on the repeated warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you meant by "WeakHashMap access synchronization", it's just a noun without any other parts. Do you think synchronization is necessary?

For the cache, I'm OK to drop it at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be simpler to start out without the caller cache. Sorry the sentence got garbled, I was trying to repeat what I said above that WeakHashMap is not synchronized so you would need to add synchronization to use it.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a number of hotspot tests that will trigger this warning, so please ensure they work correctly with the extra output.

You might want to make your "WARNING" consistent with the VM's "Warning" so that OutputAnalyzer's logic to ignore warnings will automatically ignore these too.

Thanks,
David

@AlanBateman
Copy link
Contributor

You might want to make your "WARNING" consistent with the VM's "Warning" so that OutputAnalyzer's logic to ignore warnings will automatically ignore these too.

The uppercase "WARNING" is intentional here, it was the same with illegal reflective access warnings. I'm sure Max has or will run all tests to see if there are any issues.

@dfuch
Copy link
Member

dfuch commented Jun 8, 2021

Changes to LoggerFinderLoaderTest look reasonable to me.

@wangweij
Copy link
Contributor Author

wangweij commented Jun 8, 2021

You might want to make your "WARNING" consistent with the VM's "Warning" so that OutputAnalyzer's logic to ignore warnings will automatically ignore these too.

The uppercase "WARNING" is intentional here, it was the same with illegal reflective access warnings. I'm sure Max has or will run all tests to see if there are any issues.

Will definitely run all from tier1-tier9. I ran them multiple times while implementing JEP 411.

I've seen warnings with "VM" word in the prefix and test methods that filter them out, but feel the warnings here are not related to VM. The new warnings do have impacts on some tests and I'll be very carefully not break them.

@mlbridge
Copy link

mlbridge bot commented Jun 9, 2021

Mailing list message from Peter Firmstone on security-dev:

I can re-license some code that decorates Concurrent collections with
references, so you can do this without blocking.

https://pfirmstone.github.io/JGDMS/jgdms-collections/apidocs/index.html

On 9/06/2021 4:31 am, Alan Bateman wrote:

--
Regards,

Peter Firmstone
0498 286 363
Zeus Project Services Pty Ltd.

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Jun 9, 2021

Mailing list message from Peter Firmstone on security-dev:

I can re-license some code that decorates Concurrent collections with
references, so you can do this without blocking.

https://pfirmstone.github.io/JGDMS/jgdms-collections/apidocs/index.html

On 9/06/2021 4:31 am, Alan Bateman wrote:

--
Regards,

Peter Firmstone
0498 286 363
Zeus Project Services Pty Ltd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration net net-dev@openjdk.org rfr Pull request is ready for review security security-dev@openjdk.org
4 participants