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

[Yasumasa Suenaga]

Thanks Sato-san!

Yasumasa


On 2020/03/11 12:27, naoto.sato at oracle.com wrote:
> 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