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

[Naoto Sato]

On Fri, 7 Feb 2025 13:41:03 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Naoto Sato has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Reflects review
>
> src/java.base/share/native/libjli/args.c line 472:
> 
>> 470: JLI_AddArgsFromEnvVar(JLI_List args, const char *var_name) {
>> 471: #ifdef _WIN32
>> 472:     char *env = winGetEnv(var_name);
> 
> I see that we are limiting this call to use `_wgetenv()` (the wide-character version of `getenv()`) only to `_WIN32`. Is there any harm in using `_wgetenv()` even for `_WIN64`? Or is the `getenv()` implementation on `_WIN64` not susceptible to the issues noted in the JBS?

The macro name is confusing, but `_WIN32` means Win32 API set and does not specify the target platform. This link (https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170) reads


_WIN32 Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined.


So I believen `_WIN32` is correct here.

> src/java.base/share/native/libjli/args.c line 479:
> 
>> 477:     jboolean ret = JNI_FALSE;
>> 478: 
>> 479:     if (firstAppArgIndex != 0 && // If not 'java', return
> 
> Hello Naoto, this is a pre-existing thing, but since we are changing this part of the code, I think the checks for `firstAppArgIndex` and `relaunch` can be moved before the call to `getenv()` (and now before `winGetEnv()` too) since the syscall to `getenv()` wasn't anyway changing the state of `firstAppArgIndex` or `relaunch`. I think doing those checks before the call to fetch the env value will simplify the conditional check here a bit, in terms of readability.

Thanks Jai. That is a good point. Moved those checks before winGetEnv().

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1946998326
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1946998307