8334169: Long arguments of attach operation are silently truncated on Windows #19780

alexmenkov · 2024-06-19T01:50:30Z

The change fixes a bug in Attach API implementation on Windows when argument(s) are longer than 1023 bytes

testing: test/hotspot/jtreg/serviceability/attach, test/jdk/com/sun/tools/attach on Oracle supported platforms

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8334169: Long arguments of attach operation are silently truncated on Windows (Bug - P3)

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer)
Chris Plummer (@plummercj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19780

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-19T01:51:08Z

👋 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 · 2024-06-19T01:51:21Z

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

8334169: Long arguments of attach operation are silently truncated on Windows

Reviewed-by: sspitsyn, cjplummer

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

bc7cd42: 8314498: [macos] Transferring File objects to Finder fails
c8a95a7: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent
388fcf0: 8336349: Fix more simple -Wzero-as-null-pointer-constant warnings in C2 code
ab27aca: 8336297: C2: Fix -Wzero-as-null-pointer-constant warnings in derived Node ctors
9dfcd75: 8334121: Anonymous class capturing two enclosing instances fails to compile
000de30: 8335269: [Graal] occasional timeout in java/lang/StringBuffer/TestSynchronization.java with loom
4635531: 8335159: Move method reference to lambda desugaring before Lower
a253e0f: 8335642: Hide Transform implementation for Class-File API
2b0adfc: 8335817: javac AssertionError addLocalVar checkNull
a96de6d: 8336256: memcpy short value to int local is incorrect in VtableStubs::unsafe_hash
... and 310 more: https://git.openjdk.org/jdk/compare/ba5a4670b8ad86fefb41a939752754bf36aac9dc...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 · 2024-06-19T01:51:51Z

@alexmenkov 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 · 2024-06-19T01:54:40Z

Webrevs

sspitsyn

The fix looks good. Posted some nits and questions.

src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c

sspitsyn · 2024-07-12T17:51:27Z

test/hotspot/jtreg/serviceability/attach/LongArgTest.java

+
+            HotSpotVirtualMachine vm = (HotSpotVirtualMachine)VirtualMachine.attach(String.valueOf(app.getPid()));
+
+            if (setFlag(vm)) {


Q: Should the test fail if there an IOException was caught in the setFlag() call?

Currently Attach operations have restriction for argument size, so setFlag() is expected to fail for long values.
This fix adds AttachOperationFailedException for the case on windows, linux/bsd/aix implementations throw generic IOException.
(There is JDK-8334168 to throw AttachOperationFailedException instead of IOException if an argument is too long on other platforms)

Not sure, you've answered my question. Let me ask it differently.
Here I do not care about other exceptions on other platforms.
The setFlag() can catch an IOException at the line 126 and return false in that case. The run() method at the line 94 is swallowing the false value as there is no else statement. So, we just ignore the IOException and do not test anything.
My question is: Why don't we throw a RuntimeException in the case the setFlag() returned false?

Because vm.setFlag may (and should) throw exception if bad argument is specified.
This is what the issue about - on Windows vm.setFlag with long argument did not throw exception, but truncated the argument, and successfully executed the command with truncated argument value.

test/hotspot/jtreg/serviceability/attach/LongArgTest.java

plummercj · 2024-07-13T00:39:02Z

test/hotspot/jtreg/serviceability/attach/LongArgTest.java

+
+        // Value length exceeds 1K.
+        Test withLongValue() {
+            flagValue = generateValue(1024 + 1);


Shouldn't we also be testing exactly 1024 and expect it to work?

good question!
tried to make it 1024 and it does not work!
Windows VirtualMachineImpl has one more issue - it uses 1024 char buffers, but it should be (1024 + 1) as AttachListener does (1 extra char to 0-terminator), so maximum argument size on windows is 1023.
Will fix it.

Fixed the issue, also renamed jstring_to_cstring argument to make it clear that buffer size is expected and updated callers to use sizeof.

sspitsyn

Looks good.

alexmenkov · 2024-07-16T18:08:26Z

/integrate

openjdk · 2024-07-16T18:10:50Z

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

59bf3d7: 8336080: Fix -Wzero-as-null-pointer-constant warnings in ClassLoaderStats ctor
88eff4c: 8336421: ciMethod() constructor should use ConditionalMutexLocker(Compile_lock)
c99be35: 8336474: Problemlist compiler/interpreter/Test6833129 on x86_32
419cc46: 8335533: OutOfMemoryError: Metaspace observed again on AIX in test RedefineLeakThrowable.java after JDK-8294960
8feabc8: 8334057: JLinkReproducibleTest.java support receive test.tool.vm.opts
bc7cd42: 8314498: [macos] Transferring File objects to Finder fails
c8a95a7: 8072701: resume001 failed due to ERROR: timeout for waiting for a BreakpintEvent
388fcf0: 8336349: Fix more simple -Wzero-as-null-pointer-constant warnings in C2 code
ab27aca: 8336297: C2: Fix -Wzero-as-null-pointer-constant warnings in derived Node ctors
9dfcd75: 8334121: Anonymous class capturing two enclosing instances fails to compile
... and 315 more: https://git.openjdk.org/jdk/compare/ba5a4670b8ad86fefb41a939752754bf36aac9dc...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-16T18:10:57Z

@alexmenkov Pushed as commit a60608e.

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

JDK-8334168

0ffedb1

openjdk bot added the rfr Pull request is ready for review label Jun 19, 2024

openjdk bot added the serviceability serviceability-dev@openjdk.org label Jun 19, 2024

sspitsyn reviewed Jul 12, 2024

View reviewed changes

feedback

be66602

plummercj reviewed Jul 13, 2024

View reviewed changes

Chris feedback

f075a00

plummercj approved these changes Jul 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 15, 2024

sspitsyn approved these changes Jul 16, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 16, 2024

openjdk bot closed this Jul 16, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 16, 2024

alexmenkov deleted the attach_win_long_arg branch July 16, 2024 18:11


		HotSpotVirtualMachine vm = (HotSpotVirtualMachine)VirtualMachine.attach(String.valueOf(app.getPid()));

		if (setFlag(vm)) {

8334169: Long arguments of attach operation are silently truncated on Windows #19780

8334169: Long arguments of attach operation are silently truncated on Windows #19780

Uh oh!

Conversation

alexmenkov commented Jun 19, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 19, 2024

Uh oh!

openjdk bot commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 19, 2024

Uh oh!

mlbridge bot commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sspitsyn Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

alexmenkov Jul 12, 2024

Choose a reason for hiding this comment

Uh oh!

sspitsyn Jul 13, 2024

Choose a reason for hiding this comment

Uh oh!

alexmenkov Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plummercj Jul 13, 2024

Choose a reason for hiding this comment

Uh oh!

alexmenkov Jul 13, 2024

Choose a reason for hiding this comment

Uh oh!

alexmenkov Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

alexmenkov commented Jul 16, 2024

Uh oh!

openjdk bot commented Jul 16, 2024

Uh oh!

openjdk bot commented Jul 16, 2024

Uh oh!

Uh oh!

alexmenkov commented Jun 19, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 19, 2024 •

edited

Loading

mlbridge bot commented Jun 19, 2024 •

edited

Loading