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

[Ioi Lam]

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

Thanks
- Ioi

>
> Best regards,
> Ralf