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

[Serguei Spitsyn]

On Wed, 19 Jun 2024 01:50:30 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

> 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

The fix looks good. Posted some nits and questions.

src/jdk.attach/windows/native/libattach/VirtualMachineImpl.c line 57:

> 55: doPrivilegedOpenProcess(DWORD dwDesiredAccess, BOOL bInheritHandle, DWORD dwProcessId);
> 56: 
> 57: /* Convert jstring to C string, returns JNI_FALSE if the string has been truncated. */

Nit: It is better to have it either:
 "Convert jstring to C string, return JNI_FALSE ..."
or
 "Converts jstring to C string, returns JNI_FALSE"
 
 The same applies to the comment at line 621.

test/hotspot/jtreg/serviceability/attach/LongArgTest.java line 94:

> 92:             HotSpotVirtualMachine vm = (HotSpotVirtualMachine)VirtualMachine.attach(String.valueOf(app.getPid()));
> 93: 
> 94:             if (setFlag(vm)) {

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

test/hotspot/jtreg/serviceability/attach/LongArgTest.java line 101:

> 99:                                 + (actualValue == null
> 100:                                     ? "null"
> 101:                                     : ("size is " + actualValue.length() + " instead of " + flagValue.length()));

Q: What if value is different but size is the same?
I understand the probability is very low but this is still confusing.

test/hotspot/jtreg/serviceability/attach/LongArgTest.java line 126:

> 124:                 ex.printStackTrace(System.out);
> 125:                 return false;
> 126:             }

Q: Is it always the case there is no need to close the `BufferedReader` in this context? Just want to understand.

-------------

PR Review: https://git.openjdk.org/jdk/pull/19780#pullrequestreview-2173973061
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1675379386
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676279668
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676373808
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676377248