8339289: Parameter size mismatch between client and VM sides of the Attach API - Windows

[alexmenkov]

The fix improves Attch API protocol and implements updated protocol on windows; shared code is ready to implement updated protocol support on other platforms. More detailed explanations on the 1st comment.

Testing: tier1,tier2,tier3,tier4,hs-tier5-svc manually tested backward compatibility (old tools can attach to current VMs, current tools can attach to older VMs) on Windows with jdk21u and jdk8u.

---

Progress

• [x] 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-8339289: Parameter size mismatch between client and VM sides of the Attach API - Windows (Sub-task - P3)

Reviewers

• Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20782

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

Using diff file

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

Webrev

Link to Webrev Comment

[alexmenkov]

Currently Attach API has the following restrictions:

1. attach operation must have exactly 3 arguments; The value cannot be changed due backward compatibility - tools from previous release should be able to attach to current java processes, current tool should be able to work with older java processes;
2. each argument is resticted by 1024 symbols; this may be not enough as some arguments contain file paths.

DCmd framework supports any number of command arguments, but encodes all arguments as a single attach operation arguement, so total length of all arguments are restricted by 1024 symbols.

Attach API changes:

• version of the protocol is bumped to 2; attach operation request v2 contains length of the request, so request can contain any number of argument of arbitrary length (for security reason request length is restricted by 256K);
• for backward compatibility both client and server sides support both v1 and v2; if v2 is not supported by tool or target JVM, v1 is used for communication; new attach operation is implemented: "getVersion"; old VMs report "Operation not recognized", new VMs report supported version;
• for testing purposes "jdk.attach.compat" system property is introduced to disable v2 protocol;

Windows implementation: Windows implementation is different from other platforms (other platforms use unix sockets for communications). On Windows client enqueues operation by copying request parameters and executing remote thread in the target VM process which calls special exported function. Operation results are returned by using pipe (1-directional), created by the client;

• to enqueue operation v2 new exported function has been added; only pipe name is passed to the target VM for v2 requests;
• all operation parameters are passed through pipe, operation results are returned through the same pipe (it becomes 2-directional).

[bridgekeeper[bot]]

:wave: Welcome back amenkov! 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]]

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

8339289: Enhance Attach API to support arbitrary length arguments - Windows

Reviewed-by: kevinw, sspitsyn

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

• d1540e2a49c7a41eb771fc9896c367187d070dec: 8342090: Infer::IncorporationBinaryOp::equals can produce side-effects
• 7af46a6b424cadfe298958d774da0f21db58ecd3: 8340554: Improve MessageFormat readObject checks
• 7d5eefa50673d6f7c5bd916f63271cf7898d6dee: 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang
• d8c3b0f834c603fe115ef4ca442727948b7a834e: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
• 3c14c2babbdfb46a77636ed80e083ef2f8be2b45: 8341566: Add Reader.of(CharSequence)
• b0ac633b2d0076d64b463b2a6ce19abf6b12c50f: 8342075: HttpClient: improve HTTP/2 flow control checks
• 85774b713edf8782f162ac25b61ce99a77e116f4: 8342882: RISC-V: Unify handling of jumps to runtime
• 2c31c8eeb42188ad6fd15eca50db4342cd791fb2: 8339730: Windows regression after removing ObjectMonitor Responsible
• f0b130e54f33d3190640ce33c991e35f27e9f812: 8339296: Record deconstruction pattern in switch fails to compile
• e96b4cf0a81914c6a615bb4f62ea3f139a4737f3: 8342387: C2 SuperWord: refactor and improve compiler/loopopts/superword/TestDependencyOffsets.java
• ... and 758 more: https://git.openjdk.org/jdk/compare/b711c41d442fc369a84745c0203db638e0b7e671...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]]

⚠️ @alexmenkov This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

[openjdk[bot]]

@alexmenkov 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.

[mlbridge[bot]]

Webrevs

• 04: Full - Incremental (9e6a34e0)
• 03: Full - Incremental (86bd1cd2)
• 02: Full - Incremental (1fed4ea6)
• 01: Full - Incremental (2e904c58)
• 00: Full (fd52c130)

[dcubed-ojdk]

Thanks for putting the details in a separate comment. That will make the emails that get sent for this PR much shorter.

You use the word symbols, e.g., each argument is resticted by 1024 symbols. Do you really mean characters or bytes?

[dcubed-ojdk]

Also thanks for including all the testing info!

[alexmenkov]

You use the word symbols, e.g., each argument is resticted by 1024 symbols. Do you really mean characters or bytes?

Bytes. The code used char buffers with fixed size.

[alexmenkov]

Looking for reviewers for the fix

[kevinjwalls]

Thanks for updating, looks good. I think it's clearer now that this is not just a Windows-specific fix, but will be an enhancement for all platforms in the long term. Likely argument length is more of a limitation than number of arguments.

Looking back at JDK-8215622: Add dump to file support for jmap –histo ..and that was extending the "inspectheap" attach command, but it should have been using a DiagnosticCommand invoked by jcmd. We may not say it clearly, but the attach api commands are a few basic fundamentals, and most of what we want to implement should be implemented in a DiagnosticCommand.

[alexmenkov]

Thank you for review Serguei and Kevin I'm going to rebase the change and run sanity tests before integration

[alexmenkov]

/integrate

[openjdk[bot]]

Going to push as commit 36d71735e3554264e8d17f7e0e72999ac639e398. Since your change was applied there have been 782 commits pushed to the master branch:

• ff165f9f0cf519144d7361b766bcce53d04c518e: 8342934: TYPE_USE annotations printed with error causing "," in toString output
• 0853aee3b377cf9f17340a85f600651db42e6999: 8338426: Test java/nio/channels/Selector/WakeupNow.java failed
• c202a2f7b231152136bd8960c55e43bc96cf1eb9: 8295269: G1: Improve slow startup due to predictor initialization
• 5cbd578fbe9df4f68ab21bf764208ad4f67443f6: 8342930: New tests from JDK-8335912 are failing
• 1e35da8d3341ed1af266e5b59aa90bfcfae6576a: 8343063: RISC-V: remove redundant reg copy in generate_resolve_blob
• 4f8f395e2bb692148e2b891198f28a579749dd6d: 8343060: RISC-V: enable TestFloat16VectorConvChain for riscv
• a9eb50a2d8341b454c55c2f56446775c497ddde9: 8342953: RISC-V: Fix definition of RISCV_HWPROBE_EXT_ZVFHMIN
• 94317dbcf26a54428c649ad0286e127bd6dab570: 8342884: RISC-V: verify float <--> float16 conversion
• 3c5db12bbe4d1155ab874c2862005621c6b8541d: 8342857: SA: Heap iterator makes incorrect assumptions about TLAB layout
• 4635351b1570fcea07fac1ece5f76f528d68c2a7: 8342939: Building ZGC without compiler2 fails
• ... and 772 more: https://git.openjdk.org/jdk/compare/b711c41d442fc369a84745c0203db638e0b7e671...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@alexmenkov Pushed as commit 36d71735e3554264e8d17f7e0e72999ac639e398.

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