RFR: 8240725: Some functions might not work with CJK character

[naoto.sato at oracle.com]

Looks good to me. Thanks for the fix!

Naoto

On 3/10/20 5:39 PM, Yasumasa Suenaga wrote:
> Hi Sato-san,
> 
> Thanks for your comment.
> I uploaded new webrev:
> 
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.03/
> 
> On 2020/03/11 2:08, naoto.sato at oracle.com wrote:
>> Hi Suenaga-san,
>>
>> Thank you for the update. I think your changes to the return values of 
>> MultiByteToWideChar look good.
>>
>> Then I noticed (should have noticed in the first place, sorry) that 
>> the error handling in canonicalize_md.c is incorrect (unrelated to 
>> your change). All error cases "goto finish" which would end up 
>> "free(NULL)" in some cases, e.g., line 340 (in the new file) should 
>> not end up calling "free(wresult)" and "free(wpath)"
>>
>> java_md.c
>>
>> - "convert_to_unicode()" has different indentation than others (2 
>> spaces which should be 4).
> 
> I fixed it.
> 
> 
>> - Should "wpath" be set to NULL before returning EINVAL at line 524? I 
>> don't know the contract of "create_unc_path", but the caller would 
>> receive a garbage pointer as a return value.
> 
> create_unc_path() is static function, and it would be called by just 
> JLI_Open().
> If create_unc_path() would be failed (it means `err` is set excepting 
> ERROR_SUCCESS), wpath would call free() when it is not NULL.
> 
> Thus we should set related values as below:
> 
>    A: wpath = NULL, err != ERROR_SUCCESS
>    B: wpath != NULL (not free'ed), err != ERROR_SUCCESS
> 
> I implemented A in this webrev because it is simple.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
>> Nit: please add "noreg-hard" tag to the JIRA issue.
>>
>> Naoto
>>
>>
>> On 3/9/20 7:52 PM, Yasumasa Suenaga wrote:
>>> I forgot to free buffer if MultiByteToWideChar() failed in zip_util.c .
>>> So I updated webrev:
>>>
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.02/
>>>
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/03/10 9:12, Yasumasa Suenaga wrote:
>>>> Hi Sato-san,
>>>>
>>>> Thanks for your comment!
>>>>
>>>> I updated webrev. Could you review again?
>>>>
>>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.01/
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2020/03/10 2:24, naoto.sato at oracle.com wrote:
>>>>> Hi Suenaga-san,
>>>>>
>>>>> I think the return value from the second MultiByteToWideChar 
>>>>> invocation should be examined (non-zero), and if it was not 
>>>>> successful, appropriate action should be taken.
>>>>>
>>>>> Naoto
>>>>>
>>>>> On 3/9/20 3:50 AM, Yasumasa Suenaga wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this change:
>>>>>>
>>>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8240725
>>>>>>    webrev: 
>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8240725/webrev.00/
>>>>>>
>>>>>> We found the issue that HotSpot does not start when it is deployed 
>>>>>> on the path which contains CJK character(s), and it has been fixed 
>>>>>> in JDK-8240197.
>>>>>>
>>>>>> On the review of JDK-8240197 [1], we concern similar issue might 
>>>>>> occur in other place, and I found potentially problem in below:
>>>>>>
>>>>>> - ZFILE_Open() @ zip_util.c
>>>>>> - JDK_Canonicalize() @ canonicalize_md.c (for Windows)
>>>>>> - create_unc_path() @ java_md.c (for Windows)
>>>>>> - Platform::MultibyteStringToWideString() @ WindowsPlatform.cpp
>>>>>>
>>>>>> This change passed tests on submit repo 
>>>>>> (mach5-one-ysuenaga-JDK-8240725-20200309-0811-9304139).
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> [1] 
>>>>>> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-March/038397.html 
>>>>>>