8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was set #3000

YaSuenag · 2021-03-15T02:01:12Z

NPE was thrown when I set server name prefix for debugd as following:

$ jhsdb -J-Dsun.jvm.hotspot.rmi.serverNamePrefix=test debugd --pid 781
Attaching to process ID 781 and starting RMI services, please wait...
Error attaching to process or starting server: sun.jvm.hotspot.debugger.DebuggerException: java.lang.NullPointerException: Cannot invoke "String.length()" because "this.input" is null
        at jdk.hotspot.agent/sun.jvm.hotspot.RMIHelper.rebind(RMIHelper.java:78)
        at jdk.hotspot.agent/sun.jvm.hotspot.HotSpotAgent.setupDebugger(HotSpotAgent.java:379)
        at jdk.hotspot.agent/sun.jvm.hotspot.HotSpotAgent.go(HotSpotAgent.java:329)
        at jdk.hotspot.agent/sun.jvm.hotspot.HotSpotAgent.startServer(HotSpotAgent.java:215)
        at jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runDEBUGD(SALauncher.java:431)
        at jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:493)
Caused by: java.lang.NullPointerException: Cannot invoke "String.length()" because "this.input" is null
        at java.base/java.net.URI$Parser.parse(URI.java:3166)
        at java.base/java.net.URI.<init>(URI.java:623)
        at java.rmi/java.rmi.Naming.intParseURL(Naming.java:273)
        at java.rmi/java.rmi.Naming.parseURL(Naming.java:237)
        at java.rmi/java.rmi.Naming.rebind(Naming.java:171)
        at jdk.hotspot.agent/sun.jvm.hotspot.RMIHelper.rebind(RMIHelper.java:64)
        ... 5 more

serverNamePrefix would not affect due to trivial bug.

Progress

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

Issue

JDK-8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was set

Reviewers

Chris Plummer (@plummercj - Reviewer)
Alex Menkov (@alexmenkov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3000

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

Using diff file

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

… set

bridgekeeper · 2021-03-15T02:02:00Z

👋 Welcome back ysuenaga! 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 · 2021-03-15T02:02:44Z

@YaSuenag The following label will be automatically applied to this pull request:

serviceability

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 · 2021-03-15T05:29:11Z

Webrevs

00: Full (1fadb31)

plummercj

The fix looks fine. It would be nice if this was documented somewhere (and even tested). Also, while looking into this I noticed the following comment:

    // debugServerID follows the pattern [unique_id@]host[:port]
    // we have to transform this as //host[:port]/<serverNamePrefix>['_'<unique_id>]

The [:port] part is also not document, not even in the command line syntax for sadebugd

openjdk · 2021-03-15T18:48:02Z

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

8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was set

Reviewed-by: cjplummer, amenkov

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

9201899: 8264729: Random check-in failing header checks.
d920f85: 8264540: WhiteBox.metaspaceReserveAlignment should return shared region alignment
104e925: 8264512: jdk/test/jdk/java/util/prefs/ExportNode.java relies on default platform encoding
a0ec2cb: 8248862: Implement Enhanced Pseudo-Random Number Generators
39719da: 8253266: JList and JTable constructors should clear OPAQUE_SET before calling updateUI
a8005ef: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
7f9ece2: 8264650: Cross-compilation to macos/aarch64
0039c18: 8264475: CopyArea ignores clip state in metal rendering pipeline
f084bd2: 8262355: Support for AVX-512 opmask register allocation.
0780666: 8254050: HotSpot Style Guide should permit using the "override" virtual specifier
... and 332 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.

YaSuenag · 2021-03-16T00:17:09Z

Thanks @plummercj for your review!

The fix looks fine. It would be nice if this was documented somewhere (and even tested). Also, while looking into this I noticed the following comment:
    // debugServerID follows the pattern [unique_id@]host[:port]
    // we have to transform this as //host[:port]/<serverNamePrefix>['_'<unique_id>]
The [:port] part is also not document, not even in the command line syntax for sadebugd

In addition sun.jvm.hotspot.rmi.startRegistry is also undocumented even though it works well.
Both serverNamePrefix and startRegistry are not exposed as command line options of jhsdb, I think documentation. implementing testcases, and exposing as command line options should be done simultaneously. We need to change both command line help and manpage in that work (CSR is needed of course.)

For example, I think we can expose them as following:

$ jhsdb debugd --help
    :
    --disableregistry             Do not start RMI registry (to use RMI registry that exists)
    --servernameprefix         Specify RMI server name prefix

$ jhsdb jinfo --help
    :
    --connect [<id>@]<host>[:registryport][/servernameprefix] To connect to a remote debug server (debugd).
    :
    Examples: jhsdb jinfo --pid 1234
          or  jhsdb jinfo --core ./core.1234 --exe ./myexe
          or  jhsdb jinfo --connect debugserver
          or  jhsdb jinfo --connect id@debugserver:1234/prefix

I will work for it if you are ok.

plummercj · 2021-03-16T00:41:25Z

That looks fine, although I'm a little unclear about the need for --disableregistry, and how that applies to existing code.

Also, I think a bit of explanation of [:registryport] is needed since it is not also specified by the jhsdb debugd command. That's just the port that it chooses to communicate over, and can be any available port, right? If one is not specified, what port is chosen, and what would be a reason that you would want to specify a port?

YaSuenag · 2021-03-16T00:57:48Z

That looks fine, although I'm a little unclear about the need for --disableregistry, and how that applies to existing code.

We can use it when we want to register SA endpoint to RMI registry that exists (e.g. rmid). I think it is simple to proxy to sun.jvm.hotspot.rmi.startRegistry like --registryport option in jhsdb.

https://github.com/openjdk/jdk/blob/master/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java#L392-L400

Also, I think a bit of explanation of [:registryport] is needed since it is not also specified by the jhsdb debugd command. That's just the port that it chooses to communicate over, and can be any available port, right? If one is not specified, what port is chosen, and what would be a reason that you would want to specify a port?

We can specify any available port as you said.
For example, if we run multiple debugd instances on same OS, we need to use different port. We can use --registryport and [:registryport] in that case.
However we will not have many opportunities to use this.

plummercj · 2021-03-16T01:19:26Z

I think the default rmi registry port is 1099. However, do we need to also support something like JDK-8196729 for the rmi communications port?

YaSuenag · 2021-03-16T01:25:26Z

I think the default rmi registry port is 1099. However, do we need to also support something like JDK-8196729 for the rmi communications port?

We can specify RMI communication port with --rmiport in jhsdb debugd. It has been introduced in JDK-8196751.

YaSuenag · 2021-03-16T01:28:49Z

For example, if we want to fix RMI registry and communication port, we can start debugd instance as following:

$ jhsdb debugd --pid 1234 --registryport 2000 --rmiport 3000

Then, we can connect to it as following:

$ jhsdb jinfo --connect localhost:2000

plummercj · 2021-03-16T01:59:29Z

Sorry. I was running an old jhsdb. So we already have --registryport 2000 --rmiport, and you can already specify [:] as part of the debugserver. So what needs to be added is just --servernameprefix, and better help for explaining all the parts of the debugserver that are specified with --connect. You also want to add --disableregistry, but I think that's kind of separate from the above and probably needs its own CR and CSR.

YaSuenag · 2021-03-16T02:39:02Z

So what needs to be added is just --servernameprefix, and better help for explaining all the parts of the debugserver that are specified with --connect. You also want to add --disableregistry, but I think that's kind of separate from the above and probably needs its own CR and CSR.

I filed them to JBS. I will work for them later.

--servernameprefix: https://bugs.openjdk.java.net/browse/JDK-8263635
--disableregistry: https://bugs.openjdk.java.net/browse/JDK-8263636

YaSuenag · 2021-03-24T00:54:49Z

Ping: could you review it? I need one more reviewer to push.

YaSuenag · 2021-04-06T00:08:03Z

/integrate

openjdk · 2021-04-06T00:09:34Z

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

c41cd15: 8264686: ClhsdbTestConnectArgument.java should use SATestUtils::validateSADebugDPrivileges
b7baca7: 8264288: Performance issue with MethodHandle.asCollector
9201899: 8264729: Random check-in failing header checks.
d920f85: 8264540: WhiteBox.metaspaceReserveAlignment should return shared region alignment
104e925: 8264512: jdk/test/jdk/java/util/prefs/ExportNode.java relies on default platform encoding
a0ec2cb: 8248862: Implement Enhanced Pseudo-Random Number Generators
39719da: 8253266: JList and JTable constructors should clear OPAQUE_SET before calling updateUI
a8005ef: 8166727: javac crashed: [jimage.dll+0x1942] ImageStrings::find+0x28
7f9ece2: 8264650: Cross-compilation to macos/aarch64
0039c18: 8264475: CopyArea ignores clip state in metal rendering pipeline
... and 334 more: https://git.openjdk.java.net/jdk/compare/da9ead5e7fec1facdb4e44fc0a5b872edb704b9a...master

Your commit was automatically rebased without conflicts.

Pushed as commit b1a225e.

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

8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was…

1fadb31

… set

openjdk bot added the serviceability serviceability-dev@openjdk.org label Mar 15, 2021

YaSuenag marked this pull request as ready for review March 15, 2021 05:25

openjdk bot added the rfr Pull request is ready for review label Mar 15, 2021

plummercj approved these changes Mar 15, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 15, 2021

alexmenkov approved these changes Apr 5, 2021

View reviewed changes

openjdk bot closed this Apr 6, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 6, 2021

YaSuenag deleted the JDK-8263565 branch April 6, 2021 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was set #3000

8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was set #3000

YaSuenag commented Mar 15, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 15, 2021

openjdk bot commented Mar 15, 2021

mlbridge bot commented Mar 15, 2021

plummercj left a comment

openjdk bot commented Mar 15, 2021 •

edited

Loading

YaSuenag commented Mar 16, 2021

plummercj commented Mar 16, 2021

YaSuenag commented Mar 16, 2021

plummercj commented Mar 16, 2021

YaSuenag commented Mar 16, 2021

YaSuenag commented Mar 16, 2021

plummercj commented Mar 16, 2021

YaSuenag commented Mar 16, 2021

YaSuenag commented Mar 24, 2021

YaSuenag commented Apr 6, 2021

openjdk bot commented Apr 6, 2021

8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was set #3000

8263565: NPE was thrown when sun.jvm.hotspot.rmi.serverNamePrefix was set #3000

Conversation

YaSuenag commented Mar 15, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 15, 2021

openjdk bot commented Mar 15, 2021

mlbridge bot commented Mar 15, 2021

Webrevs

plummercj left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 15, 2021 • edited Loading

YaSuenag commented Mar 16, 2021

plummercj commented Mar 16, 2021

YaSuenag commented Mar 16, 2021

plummercj commented Mar 16, 2021

YaSuenag commented Mar 16, 2021

YaSuenag commented Mar 16, 2021

plummercj commented Mar 16, 2021

YaSuenag commented Mar 16, 2021

YaSuenag commented Mar 24, 2021

YaSuenag commented Apr 6, 2021

openjdk bot commented Apr 6, 2021

YaSuenag commented Mar 15, 2021 •

edited by openjdk bot

Loading

openjdk bot commented Mar 15, 2021 •

edited

Loading