8335610: DiagnosticFramework: CmdLine::is_executable() correction

[kevinjwalls]

CmdLine::is_executable() looks wrong, surely an empty line is not executable.

With this change, in DCmd::parse_and_execute() we will avoid needlessly entering the code block to try and execute the command.

DCmd tests all good: make images test TEST="test/hotspot/jtreg/serviceability/dcmd test/jdk/sun/tools/jcmd"

---

Progress

• [ ] 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-8335610: DiagnosticFramework: CmdLine::is_executable() correction (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20006

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back kevinw! 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[bot]]

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

8335610: DiagnosticFramework: CmdLine::is_executable() correction

Reviewed-by: dholmes, jsjolen

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

• 9124a94e383f5bc6a3376eecc97ee8bd22f9e420: 8337165: Test jdk/jfr/event/gc/stacktrace/TestG1YoungAllocationPendingStackTrace.java failed: IndexOutOfBoundsException: Index 64 out of bounds for length 64
• db168d9e10c4a302b743ee208e24ba7303fec582: 8336966: Alpine Linux x86_64 compilation error: sendfile64
• dab2a0b5978cdd3fad693e4c120a84bb64a3ccde: 8337222: gc/TestDisableExplicitGC.java fails due to unexpected CodeCache GC
• 657c0bddf90b537ac653817571532705a6e3643a: 8336999: Verification for resource area allocated data structures in C2
• 90641a601c9385b5e76e1abc5362ace3ab1fff3d: 8336095: Use-after-free in Superword leads to memory corruption
• 2fbdbacad7ad45055a482c764f84da4568383020: 8337245: Fix wrong comment of StringConcatHelper
• 034297a6bd9bfcea7fa48792f54c84a6e976b319: 8336240: Test com/sun/crypto/provider/Cipher/DES/PerformanceTest.java fails with java.lang.ArithmeticException
• abc4ca5a8c440f8f3f36a9b35036772c5b5ee7ea: 8330427: Obsolete -XX:+PreserveAllAnnotations
• 0c3720b42176c7bc92105be87df7449973fbcea0: 8335131: Test "javax/swing/JColorChooser/Test6977726.java" failed on ubuntu x64 because "Preview" title is missing for GTK L&F
• 4f194f10a1481cdc9df4e6338f6cabd07a34c84c: 8337241: Shenandoah: Normalize include guards
• ... and 260 more: https://git.openjdk.org/jdk/compare/6923a5114b2a9f02f0d6f0fefc21141ac3b9322a...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[bot]]

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

• hotspot-runtime
• serviceability

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.

[kevinjwalls]

Noticed this issue while reviewing https://github.com/openjdk/jdk/pull/19776 (but not directly related to that change).

[mlbridge[bot]]

Webrevs

• 00: Full (4dff524f)

[kevinjwalls]

Can a line ever be empty?

No it can't. Runing "jcmd PID" becomes "help", and giving empty quotes as the command like jcmd PID "" does pass an empty string to parse_and_execute(), but the DCmdIter in there will never find anything to iterate (nor with multiple "" or with " ").

bash-4.2$ jcmd 18851 "" 18851: Command executed successfully

The CmdLine is the thing that would be returned by DCmdIter, and we don't create empty ones (as things stand today). So this change should be harmless but correct.

[dholmes-ora]

But if it can't be empty can we not just assert that and get rid of the is_empty check from is_executable?

[kevinjwalls]

But if it can't be empty can we not just assert that and get rid of the is_empty check from is_executable?

Yes, we might be able to do that. I was going for just the obvious logic correction, because it's distracting to read through things that look wrong.
Looking at it again today I'm still not sure if we should restrict it so a CmdLine can never be empty in some future usage.

[dholmes-ora]

My concern is that the logic was wrong and so you fixed it, but this then screams out for a test that would have detected the error, but you can't write a test because the line can never be empty, so that becomes an invariant rather than a runtime check.

[sspitsyn]

My concern is that the logic was wrong and so you fixed it, but this then screams out for a test that would have detected the error, but you can't write a test because the line can never be empty, so that becomes an invariant rather than a runtime check.

Do I understand this right that the suggestion is to add an explicit runtime check for non-emptiness and report an error if an empty line has been discovered? If so, then the is_empty()check can be removed from the CmdLine::is_executable().

[dholmes-ora]

Do I understand this right that the suggestion is to add an explicit runtime check for non-emptiness and report an error if an empty line has been discovered? If so, then the is_empty()check can be removed from the CmdLine::is_executable().

Basically yes. If the line can never be empty then assert that is the case.

[kevinjwalls]

Thanks - I was trying to make the simple "obvious" correction.

The jump is from the fact that nobody currently creates an empty CmdLine, to a ruling that nobody ever can in the future.

There aren't many places that create these CmdLine objects, and we don't expect much new code to use it, but there was an example of a new usage recently.

I don't object to making the additional change, haven't got to it yet.

[dcubed-ojdk]

Is it useful to allow (but ignore) an "empty" command to perhaps allow perf analysis of the dcmd mechanism?

[dholmes-ora]

The jump is from the fact that nobody currently creates an empty CmdLine, to a ruling that nobody ever can in the future.

No my point is either it is impossible to create an empty cmdline or else we should have a test that introduces one. If you cannot write such a test then it must be impossible. Hence we can assert that it cannot be empty. If worried about some future use then add a comment/check that would catch it.

[jdksjolen]

At the point where this is checked, the commandline string is still not parsed. I do not want any asserts regarding the structure of untrusted input. Anyway, PoC of getting "empty" lines such that is_empty() is true.

I read the code:

// diagnosticFramework.cpp:387
DCmdIter iter(cmdline, '\n'); // <--- Delimiter right there

Hypothesis: Multiple newlines in a message will lead to empty lines.

First attempt: I used jcmd directly, this failed, as jcmd does some clean up on its own. So I had to go with connecting to the PID using Python.

My Python 'attacker':

import socket
import os

# My Java PID, connect with JCMD ordinarily to your Java process first to create the PID file, then 
# replace the numbers with your Java PID
socket_path = "/proc/2603121/root/tmp/.java_pid2603121"
client = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)

print("Connect")
print(client.connect(socket_path))

#  // The request is:
#  //   <ver>0<cmd>0<arg>0<arg>0<arg>0
#  // where <ver> is the protocol version (1), <cmd> is the command
#  // name ("load", "datadump", ...), and <arg> is an argument
connect_payload = "1\x00jcmd\x00\n\n\n\x00\n\n\n\x00\n\n\n\x00"
print("Sending payload")
print(client.sendall(connect_payload.encode()))
print("Sent")
# Receive a response from the server
response = client.recv(4096)
print(f'Received response: {response.decode()}')

Later on, in a gdb session for my Java program and where I've connected to the Java process using my Python program:

(gdb) p line.is_empty()
$6 = true
(gdb) p cmdline
$7 = 0x7fff68240fc9 "\n\n\n"
(gdb) p line
$8 = {<StackObj> = {<No data fields>}, _cmd = 0x7fff68240fc9 "\n\n\n", _cmd_len = 0, 
  _args = 0x7fff68240fc9 "\n\n\n", _args_len = 0}
(gdb) p bt
No symbol "bt" in current context.
(gdb) bt
#0  DCmd::parse_and_execute (source=DCmd_Source_AttachAPI, out=0x7fffaa62ecc0, cmdline=0x7fff68240fc9 "\n\n\n", 
    delim=32 ' ', __the_thread__=0x7fff8c001010)
    at /home/johan/jdk/open/src/hotspot/share/services/diagnosticFramework.cpp:399
#1  0x00007ffff58f8748 in jcmd (op=0x7fff68240fb0, out=0x7fffaa62ecc0)
    at /home/johan/jdk/open/src/hotspot/share/services/attachListener.cpp:209
#2  0x00007ffff58f91ae in AttachListenerThread::thread_entry (thread=0x7fff8c001010, __the_thread__=0x7fff8c001010)
    at /home/johan/jdk/open/src/hotspot/share/services/attachListener.cpp:418
#3  0x00007ffff6036294 in JavaThread::thread_main_inner (this=0x7fff8c001010)
    at /home/johan/jdk/open/src/hotspot/share/runtime/javaThread.cpp:757
#4  0x00007ffff6036129 in JavaThread::run (this=0x7fff8c001010)
    at /home/johan/jdk/open/src/hotspot/share/runtime/javaThread.cpp:742
#5  0x00007ffff67c5fb7 in Thread::call_run (this=0x7fff8c001010)
    at /home/johan/jdk/open/src/hotspot/share/runtime/thread.cpp:225
#6  0x00007ffff6549e28 in thread_native_entry (thread=0x7fff8c001010)
    at /home/johan/jdk/open/src/hotspot/os/linux/os_linux.cpp:858
#7  0x00007ffff7c94ac3 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:442
#8  0x00007ffff7d26850 in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
(gdb) 

[jdksjolen]

Oh, and also, the output of my Python running:

Connect
None
Sending payload
None
Sent
Received response: -1
java.lang.IllegalArgumentException: Unknown diagnostic command

[dholmes-ora]

@jdksjolen - thank you! IIUC from your report, our own code may never have an empty CmdLine but you can directly inject one using the communication protocol. In that case we the fix is good as-is. A test case would be nice but not if we need to use python.

[kevinjwalls]

Thanks Johan @jdksjolen and thanks David!

Yes good reminder that there can be multiple lines to process, and we have occasionally seen tools that emulate the attach protocol and send text.

[jdksjolen]

FWIW, the Python code maps closely to the standard C/POSIX socket API, it was just a bit quicker to write.

[kevinjwalls]

Thanks for the reviews, integrating.

[kevinjwalls]

/integrate

[openjdk[bot]]

Going to push as commit 0325ab8d2353f29ac40ff4b028cbc29bff40c59b. Since your change was applied there have been 282 commits pushed to the master branch:

• 7ac531181c25815577ba2f6f426e1da270e4f589: 8331126: [s390x] secondary_super_cache does not scale well
• 156f0b4332bf076165898417cf6678d2fc32df5c: 8337213: Shenandoah: Add verification for class mirrors
• 9e6e0a8f341389215f0db6b2260f2b16351f02be: 8336343: Add more known sysroot library locations for ALSA
• bc7c255b156bf3bb3fd8c3f622b8127ab27e7c7a: 8337416: Fix -Wzero-as-null-pointer-constant warnings in misc. runtime code
• 7e21d4c1916d6690b717911179314c26a0da5ed9: 8337268: Redundant Math.ceil in StyleSheet.ListPainter#drawShape
• ab27090aa085283233851410827767785b3b7b1f: 8337225: Demote maxStack and maxLocals from CodeModel to CodeAttribute
• bd36b6ae5d0d760670a0bd722878614a6cd553d6: 8337285: Examine java.text.DecimalFormat API for api/implXxx tag usage
• a86244f83cc4e234bdf8dc2c8d87640b6aab8463: 6381729: Javadoc for generic constructor doesn't document type parameter
• c4e6255ac3ec5520c4cb6d0d4ad9013da177ba88: 8332738: Debug agent can deadlock on callbackLock when using StackFrame.PopFrames
• c23d37e10a429c0e7248593b07ef1ccdcd34bd1c: 8337300: java/lang/Process/WaitForDuration.java leaves child process behind
• ... and 272 more: https://git.openjdk.org/jdk/compare/6923a5114b2a9f02f0d6f0fefc21141ac3b9322a...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@kevinjwalls Pushed as commit 0325ab8d2353f29ac40ff4b028cbc29bff40c59b.

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