RFR: 8240197: Cannot start JVM when $JAVA_HOME includes CJK characters

[Yasumasa Suenaga]

Hi Ralf, Ioi,

I uploaded new webrev:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8240197/webrev.02/

My comments are in below.

On 2020/03/03 3:41, Ioi Lam wrote:
> 
> 
> On 3/2/20 5:24 AM, Schmelter, Ralf wrote:
>> Hi Yasumasa,
>>
>> thanks for your patch. I have some remarks:
>>
>> 1. wcstombs_s creates UTF-16, as does MultiByteToWideChar. That wasn't the problem. The problem was that wcstombs_s uses the current locale, which is "C" initially. One is normally expected to set the locale to the system locale via setlocale(LC_ALL, ""), as it is done in ParseLocale() on Unix.
>>
>> One could fix the original problem by setting the locale (probably not a good idea, since this changes the locale of the whole process). Or one could specify the correct locale to wcstombs like:
>>
>> static _locale_t loc = _create_locale(LC_ALL, "");
>> err = ::_mbstowcs_s_l(&converted_chars, path_start, buf_len + 1, buf, buf_len, loc);
>>
>> Both fix the problem. But I agree, using the MultiByteToWideChar method is better, since you don't have to create the locale.
>>
>> There are some other places in the code where mbstowcs is used on Windows (canonicalize_md.c for example). They should probably fixed in the same way.

I agree with you, We need to fix similar codes in entire of JDK.
I will pick up and will file them to JBS in future.


>> 2.I don't think that using strlen(buf) as the number of wchars to allocate for the converted string is really wrong. There are no code pages supported by Java which would map a single byte to a Unicode character > U+ffff. But since one can get the always correct value via MultiByteToWideChar, this is futureproof and obviously right.
>>
>> 3. I would not use the CP_THREAD_ACP code page in MultiByteToWideChar and instead use CP_ACP, because the latter always uses the system locale. And this is the locale used to convert the arguments to bytes in the first place.

We should use CP_THREAD_ACP because the user might change locale via `chcp` on their console.


>> 4. You replaced the malloc/memcpy for the string which will be used to call native_path() with a strdup. This leads to a memory overwrite, when a path like "C:" is used, since native_path() will convert it to "C:.". And the debug version of the VM will detect this, since the tail guard will be partially overwritten. You can check this by running:
>>
>> make run-test TEST=gtest:os_windows

Ok, I use malloc() instead of strdup() in new webrev.


>> 5. Using WCHAR and LPWSTR seems odd to me. Nobody uses LPCSTR or CHAR for char* and char. But if you think WCHAR is better, I would adjust the return value of the method and the functions which use it too.

This change is based on the comment from Thomas, and I agree with him because Windows API uses their types.
I changed return type of wide_abs_unc_path() to LPWSTR, but I didn't change the caller of it because I want to focus this change to wide_abs_unc_path() and it would not give big effect to caller.


>> 6. When you handle a relative path, you now only allocate enough space to hold the relative path, not the generated absolute path. Since a relative path is likely to be smaller than the absolute path, the _wfullpath call later will likely fail. That was the reason we used JVM_MAXPATHLEN for relative paths. You could fix this like in this patch http://cr.openjdk.java.net/~rschmelter/webrevs/tmp/unicode_convert.patch
> Regarding #6, we have test cases under test/hotspot/jtreg/runtime/cds/appcds that use relative paths. Please run these tests when validating the patch.

I use GetFullPathNameW() to get the length of result and to convert to full path.
It resolves your concern.

This webrev has passed all tests on submit repo (mach5-one-ysuenaga-JDK-8240197-2-20200303-0158-9122754) and also has passed test/hotspot/jtreg/runtime/cds/appcds jtreg tests on my laptop.


Thanks,

Yasumasa


> Thanks
> - Ioi
> 
>>
>> Best regards,
>> Ralf
>