RFR: 8349254: Disable "best-fit" mapping on Windows environment variables [v2]

[Justin Lu]

On Fri, 7 Feb 2025 18:32:26 GMT, Naoto Sato <naoto at openjdk.org> wrote:

>> This is a follow-on fix to [JDK-8337506](https://bugs.openjdk.org/browse/JDK-8337506). The Java launcher can take arguments from "JDK_JAVA_OPTIONS" environment variable, so the same processing is needed. Also, obsolete code for Windows 9x has been removed.
>
> Naoto Sato has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reflects review

Logic to get the UTF-16 string env value, and map back to a code page string looks good to me. All conversions are done with the `WC_NO_BEST_FIT_CHARS` flag, so indeed there should be no best-fit mappings done. All the failure checks on the return values of `MultiByteToWideChar` and `WideCharToMultiByte` look appropriate as well.

I left a few comments.

src/java.base/share/native/libjli/args.c line 603:

> 601: /*
> 602:  * getenv() without best-fit mapping
> 603:  */

I think a quick comment that says something along the lines of 


env variable name: code page -> UTF-16 
variable value : UTF-16 -> code page


would be helpful to overview what is happening here.

src/java.base/share/native/libjli/args.c line 611:

> 609:         LPWSTR wcVarName = JLI_MemAlloc(wcCount * sizeof(wchar_t));
> 610:         if (MultiByteToWideChar(CP_ACP, 0, var_name, -1, wcVarName, wcCount) != 0) {
> 611:             LPWSTR wcEnvVar = _wgetenv(wcVarName);

Since we are in Windows specific code, would it make sense to call `GetEnvironmentVariableW` over `_wgetenv`? It won't be as concise since we will have to follow the 2-call style of determining the size, then filling the buffer; but I presume it avoids some overhead, since it's directly apart of the Win32 API? I am also guessing it's the convention for Windows native code, although I'm not fully sure.

test/jdk/tools/launcher/DisableBestFitMappingTest.java line 32:

> 30:  * @requires (os.family == "windows")
> 31:  * @library /test/lib
> 32:  * @run junit DisableBestFitMappingTest

I think it might be best to re-write this test as a non Junit test. Through IntelliJ it's hard to run this test, I presume because of the combination of it being a Junit test and having a `main` method? If I run the 'launcher' suite of tests, this test does not seem to be included.

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

PR Review: https://git.openjdk.org/jdk/pull/23498#pullrequestreview-2602530119
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947013548
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1946999296
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947024510