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

[Alex Menkov]

On Fri, 12 Jul 2024 06:27:46 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Alex Menkov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   feedback
>
> 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.

Fixed

> 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?

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](https://bugs.openjdk.org/browse/JDK-8334168) to throw AttachOperationFailedException instead of IOException if an argument is too long on other platforms)

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

I updated the code to check if the value was truncated

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

We can get exception here only if vm.setFlag() throws it. In the case BufferedReader is not yet constructed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676454684
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676437197
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676454322
PR Review Comment: https://git.openjdk.org/jdk/pull/19780#discussion_r1676454488