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

[Yasumasa Suenaga]

Thanks Sato-san!

Yasumasa


On 2020/03/11 12:27, naoto.s...@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.s...@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.s...@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

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

[naoto . sato]

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

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

[Yasumasa Suenaga]

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

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

[naoto . sato]

Ah, thanks Martin. Never mind about the comment wrt free(NULL) then.

Naoto

On 3/10/20 11:30 AM, Martin Buchholz wrote:



On Tue, Mar 10, 2020 at 10:08 AM > wrote:



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.


FYI  free(NULL) is a no-op
https://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html 

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

[Martin Buchholz]

On Tue, Mar 10, 2020 at 10:08 AM  wrote:

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

FYI  free(NULL) is a no-op
https://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html

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

[naoto . sato]

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


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


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

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

[Yasumasa Suenaga]

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

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

[Yasumasa Suenaga]

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

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

[naoto . sato]

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 

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

[Yasumasa Suenaga]

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