JDK-8278967 rmiregistry fails to start because SecurityManager is disabled

[stuart-marks]

Enable the security manager in rmiregistry's launcher arguments.

---

Progress

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

Issue

• JDK-8278967: rmiregistry fails to start because SecurityManager is disabled

Reviewers

• Alan Bateman (@AlanBateman - Reviewer)
• Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 45

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

Using diff file

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

[bridgekeeper]

👋 Welcome back smarks! 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]

@stuart-marks The following label will be automatically applied to this pull request:

• build

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]

Webrevs

• 01: Full - Incremental (f713e731)
• 00: Full (e899bd2d)

[dholmes-ora]

Hi Stuart,

I think specifying "allow" would be the behaviour preserving change here. That avoids any risk that enabling the SM earlier changes some behaviour during VM initialization.

Cheers,
David

[stuart-marks]

I think specifying "allow" would be the behaviour preserving change here.

This is strictly true. I did think about doing this. However, I don't think it makes much of a practical difference. It's always been fully supported to enable the security manager using properties, and the rmiregistry main hardly does anything before enabling the security manager. As long as we're going to the trouble of setting a property to allow the security manager, might as well enable it at the same time, I think.

In addition, the warning messages emitted by enabling the security manager with the API are worse than those from the command line. Using the API gives this:

WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by sun.rmi.registry.RegistryImpl
WARNING: Please consider reporting this to the maintainers of sun.rmi.registry.RegistryImpl
WARNING: System::setSecurityManager will be removed in a future release

and using the property gives this:

WARNING: A command line option has enabled the Security Manager
WARNING: The Security Manager is deprecated and will be removed in a future release

Using the property gives a shorter warning and one that is less misleading (!) than the one from use of the API. (I mean, it's kind of ridiculous for the rmiregistry command to emit a message suggesting that something be reported to the maintainers of RegistryImpl. In some sense it would be good if there were a way to disable the warning, but I rate the likelihood of that occurring as low.)

[dholmes-ora]

My concern is that having the SM installed during part of VM initialization could lead to different behaviour compared to installing the SM after that. This may be a small risk but it is a risk, and not something we are likely to observe in our own testing.

If there is no way to hide those warning messages (@SuppressWarnings in the code?) then we have a serious flaw in our warning system. As you say these warnings are for the developer not the end user!

[AlanBateman]

/label add security
/label add core-libs

[openjdk]

@AlanBateman
The security label was successfully added.

[openjdk]

@AlanBateman
The core-libs label was successfully added.

[AlanBateman]

As things stand, rmiregsitry -J-Djava.security.manager and rmiregistry -J-Djava.security.manager=allow are equivalent because rmiregistry sets the default SM. Some difference may be observed if someone is running rmiregistry with a custom system class loader set, or custom file system provider, or running it with a JVM TI agent that is looking at events between VMStart and VMInit but these seem somewhat obscure scenarios for a command line program If rmiregstry worked with ToolProvider then I think there would be more to discuss. If the final patch is to have the launcher set the default security manager then we may want to change RegistryImpl.createRegistry to fail if not set.

The warning that is emitted for both cases is expected. JEP 411 is very clear that it there is no mechanism to suppress it. We may need to add a release note to document that rmiregistry will emit a warning a startup, that will at least be something to point at in the event that anyone asks or reports an issue.

[stuart-marks]

I think there's little to worry about with custom configurations of the rmiregistry tool. What somebody might do is to enable a customized security manager using properties... of course they'd also have to ensure that the customized SM is somewhere in the classpath as well, by adding more command line options. That seems fairly unlikely to me. Anyone who wants to run an RMI registry service in some specialized configuration would probably just set things up themselves and then use the LocateRegistry.createRegistry API instead of trying to tinker with the rmiregistry command.

If rmiregistry enables the SM using properties, then yes we can probably change the code to assert that the SM is running instead of conditionally enabling it like it does now.

Next headache is that tools/launcher/VersionCheck.jtr fails because it sees the warning messages instead of the version string. (Argh!) Interestingly this test passes even though rmiregistry currently fails to operate normally, because it runs well enough to print its version string, but not well enough to start up a registry service. (Double argh!)

The warnings policy is a separate issue being discussed elsewhere.

[dholmes-ora]

In the same kind of PR (#53) for jstatd @AlanBateman writes:

This looks okay in that it is now worked exactly as it did in JDK 17.

And that PR uses allow as I have suggested here (to preserve existing behaviour). All the affected launchers should be fixed in the same consistent way.

[stuart-marks]

Enabling the security manager using a property, versus setting the property to allow and enabling it in code, is mostly a distinction without a difference. I don't think there is really a need to have the different tools be consistent. Local tool considerations probably swamp the need for any cross-tool consistency.

In this case some of the RMI registry tests set the property to allow on the command line and then rely on the code to enable the security manager using the API, so it's much easier to switch the rmiregistry launcher to use the allow technique instead. This is sort-of the tail (the tests) wagging the doc (the tool) but there doesn't seem to any benefit to be gained from fiddling around with the tests and the RMI test library.

I've also updated VersionCheck.java to filter out the security manager warning messages.

[AlanBateman]

This version looks okay, avoids having an attempt to set the SM in createRegistry always be skipped.

[openjdk]

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

8278967: rmiregistry fails to start because SecurityManager is disabled

Reviewed-by: alanb, erikj

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

• 713fbeb: 8278987: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in write_sample_info
• 7341439: 8279007: jstatd fails to start because SecurityManager is disabled
• 97c5cd7: 8278508: Enable X86 maskAll instruction pattern for 32 bit JVM.
• 9ee3ccf: 8279045: Intrinsics missing vzeroupper instruction
• 84d3333: 8279081: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on 2 platforms
• 1128674: 8278627: Shenandoah: TestHeapDump test failed
• 54517fa: 8279074: ProblemList compiler/codecache/jmx/PoolsIndependenceTest.java on macosx-aarch64
• ac7430c: 8278044: ObjectInputStream methods invoking the OIF.CFG.getSerialFilterFactory() silent about error cases.
• db3d6d7: 8278087: Deserialization filter and filter factory property error reporting under specified
• 467f654: 8279011: JFR: JfrChunkWriter incorrectly handles int64_t chunk size as size_t
• ... and 19 more: https://git.openjdk.java.net/jdk18/compare/918e3397858c425e9c3b82c9a918b7626603a59c...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.

[erikj79]

Build change looks good.

[stuart-marks]

/integrate

[openjdk]

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

• 2be3e7e: 8278239: vmTestbase/nsk/jvmti/RedefineClasses/StressRedefine failed with EXCEPTION_ACCESS_VIOLATION at 0x000000000000000d
• dfb15c3: 8274315: JFR: One closed state per file or stream
• e49d4a9: 8271447: java.nio.file.InvalidPathException: Malformed input or input contains unmappable characters
• 713fbeb: 8278987: RunThese24H.java failed with EXCEPTION_ACCESS_VIOLATION in write_sample_info
• 7341439: 8279007: jstatd fails to start because SecurityManager is disabled
• 97c5cd7: 8278508: Enable X86 maskAll instruction pattern for 32 bit JVM.
• 9ee3ccf: 8279045: Intrinsics missing vzeroupper instruction
• 84d3333: 8279081: ProblemList jdk/jfr/event/oldobject/TestLargeRootSet.java on 2 platforms
• 1128674: 8278627: Shenandoah: TestHeapDump test failed
• 54517fa: 8279074: ProblemList compiler/codecache/jmx/PoolsIndependenceTest.java on macosx-aarch64
• ... and 22 more: https://git.openjdk.java.net/jdk18/compare/918e3397858c425e9c3b82c9a918b7626603a59c...master

Your commit was automatically rebased without conflicts.

[openjdk]

@stuart-marks Pushed as commit 04ee921.

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