Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

Hi Philipp

The change is now at http://hg.openjdk.java.net/jdk10/master/rev/f60a42d4b8cd.

I only moved the test to its new directory. Everything else unchanged.

Thanks again for your contribution, looking forward for more!

--Max

> On Sep 27, 2017, at 1:49 PM, Philipp Kunz  wrote:
> 
> Hi Max
> 
> Thank you for your update and its associated effort.
> 
> I suggest that at least a comment should be added about why and how 
> non-existing files can be added and the test still serves it's purpose. In 
> fact I was quite a bit surprised when I found out that JarUtils.createJar 
> adds the file name as contents if the file cannot be found. Ideally, we would 
> also add some note about why this was relevant, about the test not compiling 
> on certain oses with jtreg together with the acute but it doesn't apply any 
> longer now. Altogether the test has become even simpler now.
> 
> One more small thing that I just noticed now is that I would prefer 
> ManifestFileName not to start with a capital letter like the other nearby 
> variables. I thought testName was too ambiguous a name and changed it to 
> testClassName. I also reviewed and slightly changed a few comments again. Of 
> course it will never become perfect but now it should do. See attached patch.
> 
> The contributed by is fine. I'd be glad to share credits with you and please 
> accept more flattering for your collaboration.
> 
> Regards,
> Philipp
> 

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Philipp Kunz]

Hi Max

Thank you for your update and its associated effort.

I suggest that at least a comment should be added about why and how 
non-existing files can be added and the test still serves it's purpose. 
In fact I was quite a bit surprised when I found out that 
JarUtils.createJar adds the file name as contents if the file cannot be 
found. Ideally, we would also add some note about why this was relevant, 
about the test not compiling on certain oses with jtreg together with 
the acute but it doesn't apply any longer now. Altogether the test has 
become even simpler now.


One more small thing that I just noticed now is that I would prefer 
ManifestFileName not to start with a capital letter like the other 
nearby variables. I thought testName was too ambiguous a name and 
changed it to testClassName. I also reviewed and slightly changed a few 
comments again. Of course it will never become perfect but now it should 
do. See attached patch.


The contributed by is fine. I'd be glad to share credits with you and 
please accept more flattering for your collaboration.


Regards,
Philipp



On 27.09.2017 03:20, Weijun Wang wrote:

Hi Philipp

The problem is that when launching by jtreg javac has difficulties writing the 
class file with the é char into the file system on several OSes.

I've updated the test a little. Now they are not written to files. Fortunately 
JarUtils can add non-existing entries. The test now passes on all our testing 
platforms.

http://cr.openjdk.java.net/~weijun/6695402/webrev.01

If you are OK with the new version I'll push them.

Thanks
Max


On Sep 26, 2017, at 10:54 PM, Weijun Wang  wrote:

It might be a jtreg issue, but I'll have to get it resolved before pushing your 
changeset.

--Max


On Sep 26, 2017, at 7:30 PM, Weijun Wang  wrote:

Oops, the new test fails on Linux and Solaris.

/scratch/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java:54:
 error: error while writing A1234567890B1234567890C123456789D1?xyz: bad 
filename 
RelativeFile[LineBrokenMultiByteCharacter$A1234567890B1234567890C123456789D1?xyz.class]
   static class A1234567890B1234567890C123456789D1\u00E9xyz { }
  ^
1 error

I'll ask the compiler team.

--Max


On Sep 26, 2017, at 3:51 PM, Weijun Wang  wrote:



On Sep 26, 2017, at 1:37 PM, Philipp Kunz  wrote:

Hi Max

This time I got it with readAllBytes. Thank you for the hint.

Apparently, UTF characters are allowed in source code, particularly in 
identifiers here, which also has caused the bug. Even if only for sending 
patches around I changed it and was surprised to see escaping working not only 
in strings but also in identifiers.

See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2

I've submitted your change to our testing server. Once it's OK, I'll push the 
changeset.

I assume "Contributed-by: Philipp Kunz " is good.

BTW, there are several TAB chars and trailing spaces in your patch. I've 
removed them.

Thanks for your contribution.

--Max


When I had another look at the test I came to the conclusion that it does not 
need what has been named refClassFileName before. The purpose of the test is 
only to check a signature of a class with a two byte character in its name 
and not at the same time to verify that if that test failed it is specifically 
because of the name. If it fails there is a problem no matter why. In the 
beginning it was handy to see the difference but I don't think it should be 
kept and maintained so I removed it. For the update signature case a second 
file to sign is still required though.

I considered multi-byte a one word before but now I also prefer it with a 
capital b. Anyway, this name might not be the best choice and I changed it to 
LineBrokenMultiByteCharacter.

See attached patch.

Regards,
Philipp



--


Gruss Philipp







Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
philipp.k...@paratix.ch 
diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
--- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java	Thu Aug 31 22:21:20 2017 -0700
+++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java	Wed Sep 27 07:40:53 2017 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2011, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,6 +28,7 @@
 import java.security.*;
 import java.util.HashMap;
 import java.io.ByteArrayOutputStream;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 /**
  * This class is used to compute 

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

Hi Philipp

The problem is that when launching by jtreg javac has difficulties writing the 
class file with the é char into the file system on several OSes.

I've updated the test a little. Now they are not written to files. Fortunately 
JarUtils can add non-existing entries. The test now passes on all our testing 
platforms.

   http://cr.openjdk.java.net/~weijun/6695402/webrev.01

If you are OK with the new version I'll push them.

Thanks
Max

> On Sep 26, 2017, at 10:54 PM, Weijun Wang  wrote:
> 
> It might be a jtreg issue, but I'll have to get it resolved before pushing 
> your changeset.
> 
> --Max
> 
>> On Sep 26, 2017, at 7:30 PM, Weijun Wang  wrote:
>> 
>> Oops, the new test fails on Linux and Solaris.
>> 
>> /scratch/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java:54:
>>  error: error while writing A1234567890B1234567890C123456789D1?xyz: bad 
>> filename 
>> RelativeFile[LineBrokenMultiByteCharacter$A1234567890B1234567890C123456789D1?xyz.class]
>>   static class A1234567890B1234567890C123456789D1\u00E9xyz { }
>>  ^
>> 1 error
>> 
>> I'll ask the compiler team.
>> 
>> --Max
>> 
>>> On Sep 26, 2017, at 3:51 PM, Weijun Wang  wrote:
>>> 
>>> 
 On Sep 26, 2017, at 1:37 PM, Philipp Kunz  wrote:
 
 Hi Max
 
 This time I got it with readAllBytes. Thank you for the hint.
 
 Apparently, UTF characters are allowed in source code, particularly in 
 identifiers here, which also has caused the bug. Even if only for sending 
 patches around I changed it and was surprised to see escaping working not 
 only in strings but also in identifiers.
>>> 
>>> See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2
>>> 
>>> I've submitted your change to our testing server. Once it's OK, I'll push 
>>> the changeset.
>>> 
>>> I assume "Contributed-by: Philipp Kunz " is good.
>>> 
>>> BTW, there are several TAB chars and trailing spaces in your patch. I've 
>>> removed them.
>>> 
>>> Thanks for your contribution.
>>> 
>>> --Max
>>> 
 
 When I had another look at the test I came to the conclusion that it does 
 not need what has been named refClassFileName before. The purpose of the 
 test is only to check a signature of a class with a two byte character 
 in its name and not at the same time to verify that if that test failed it 
 is specifically because of the name. If it fails there is a problem no 
 matter why. In the beginning it was handy to see the difference but I 
 don't think it should be kept and maintained so I removed it. For the 
 update signature case a second file to sign is still required though.
 
 I considered multi-byte a one word before but now I also prefer it with a 
 capital b. Anyway, this name might not be the best choice and I changed it 
 to LineBrokenMultiByteCharacter.
 
 See attached patch.
 
 Regards,
 Philipp
 
>>> 
>> 
> 

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

It might be a jtreg issue, but I'll have to get it resolved before pushing your 
changeset.

--Max

> On Sep 26, 2017, at 7:30 PM, Weijun Wang  wrote:
> 
> Oops, the new test fails on Linux and Solaris.
> 
> /scratch/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java:54:
>  error: error while writing A1234567890B1234567890C123456789D1?xyz: bad 
> filename 
> RelativeFile[LineBrokenMultiByteCharacter$A1234567890B1234567890C123456789D1?xyz.class]
>static class A1234567890B1234567890C123456789D1\u00E9xyz { }
>   ^
> 1 error
> 
> I'll ask the compiler team.
> 
> --Max
> 
>> On Sep 26, 2017, at 3:51 PM, Weijun Wang  wrote:
>> 
>> 
>>> On Sep 26, 2017, at 1:37 PM, Philipp Kunz  wrote:
>>> 
>>> Hi Max
>>> 
>>> This time I got it with readAllBytes. Thank you for the hint.
>>> 
>>> Apparently, UTF characters are allowed in source code, particularly in 
>>> identifiers here, which also has caused the bug. Even if only for sending 
>>> patches around I changed it and was surprised to see escaping working not 
>>> only in strings but also in identifiers.
>> 
>> See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2
>> 
>> I've submitted your change to our testing server. Once it's OK, I'll push 
>> the changeset.
>> 
>> I assume "Contributed-by: Philipp Kunz " is good.
>> 
>> BTW, there are several TAB chars and trailing spaces in your patch. I've 
>> removed them.
>> 
>> Thanks for your contribution.
>> 
>> --Max
>> 
>>> 
>>> When I had another look at the test I came to the conclusion that it does 
>>> not need what has been named refClassFileName before. The purpose of the 
>>> test is only to check a signature of a class with a two byte character 
>>> in its name and not at the same time to verify that if that test failed it 
>>> is specifically because of the name. If it fails there is a problem no 
>>> matter why. In the beginning it was handy to see the difference but I don't 
>>> think it should be kept and maintained so I removed it. For the update 
>>> signature case a second file to sign is still required though.
>>> 
>>> I considered multi-byte a one word before but now I also prefer it with a 
>>> capital b. Anyway, this name might not be the best choice and I changed it 
>>> to LineBrokenMultiByteCharacter.
>>> 
>>> See attached patch.
>>> 
>>> Regards,
>>> Philipp
>>> 
>> 
> 

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

Oops, the new test fails on Linux and Solaris.

/scratch/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java:54:
 error: error while writing A1234567890B1234567890C123456789D1?xyz: bad 
filename 
RelativeFile[LineBrokenMultiByteCharacter$A1234567890B1234567890C123456789D1?xyz.class]
static class A1234567890B1234567890C123456789D1\u00E9xyz { }
   ^
1 error

I'll ask the compiler team.

--Max

> On Sep 26, 2017, at 3:51 PM, Weijun Wang  wrote:
> 
> 
>> On Sep 26, 2017, at 1:37 PM, Philipp Kunz  wrote:
>> 
>> Hi Max
>> 
>> This time I got it with readAllBytes. Thank you for the hint.
>> 
>> Apparently, UTF characters are allowed in source code, particularly in 
>> identifiers here, which also has caused the bug. Even if only for sending 
>> patches around I changed it and was surprised to see escaping working not 
>> only in strings but also in identifiers.
> 
> See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2
> 
> I've submitted your change to our testing server. Once it's OK, I'll push the 
> changeset.
> 
> I assume "Contributed-by: Philipp Kunz " is good.
> 
> BTW, there are several TAB chars and trailing spaces in your patch. I've 
> removed them.
> 
> Thanks for your contribution.
> 
> --Max
> 
>> 
>> When I had another look at the test I came to the conclusion that it does 
>> not need what has been named refClassFileName before. The purpose of the 
>> test is only to check a signature of a class with a two byte character 
>> in its name and not at the same time to verify that if that test failed it 
>> is specifically because of the name. If it fails there is a problem no 
>> matter why. In the beginning it was handy to see the difference but I don't 
>> think it should be kept and maintained so I removed it. For the update 
>> signature case a second file to sign is still required though.
>> 
>> I considered multi-byte a one word before but now I also prefer it with a 
>> capital b. Anyway, this name might not be the best choice and I changed it 
>> to LineBrokenMultiByteCharacter.
>> 
>> See attached patch.
>> 
>> Regards,
>> Philipp
>> 
> 

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

> On Sep 26, 2017, at 1:37 PM, Philipp Kunz  wrote:
> 
> Hi Max
> 
> This time I got it with readAllBytes. Thank you for the hint.
> 
> Apparently, UTF characters are allowed in source code, particularly in 
> identifiers here, which also has caused the bug. Even if only for sending 
> patches around I changed it and was surprised to see escaping working not 
> only in strings but also in identifiers.

See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2

I've submitted your change to our testing server. Once it's OK, I'll push the 
changeset.

I assume "Contributed-by: Philipp Kunz " is good.

BTW, there are several TAB chars and trailing spaces in your patch. I've 
removed them.

Thanks for your contribution.

--Max

> 
> When I had another look at the test I came to the conclusion that it does not 
> need what has been named refClassFileName before. The purpose of the test is 
> only to check a signature of a class with a two byte character in its 
> name and not at the same time to verify that if that test failed it is 
> specifically because of the name. If it fails there is a problem no matter 
> why. In the beginning it was handy to see the difference but I don't think it 
> should be kept and maintained so I removed it. For the update signature case 
> a second file to sign is still required though.
> 
> I considered multi-byte a one word before but now I also prefer it with a 
> capital b. Anyway, this name might not be the best choice and I changed it to 
> LineBrokenMultiByteCharacter.
> 
> See attached patch.
> 
> Regards,
> Philipp
> 

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Philipp Kunz]

Hi Max

This time I got it with readAllBytes. Thank you for the hint.

Apparently, UTF characters are allowed in source code, particularly in 
identifiers here, which also has caused the bug. Even if only for 
sending patches around I changed it and was surprised to see escaping 
working not only in strings but also in identifiers.


When I had another look at the test I came to the conclusion that it 
does not need what has been named refClassFileName before. The purpose 
of the test is only to check a signature of a class with a two byte 
character in its name and not at the same time to verify that if that 
test failed it is specifically because of the name. If it fails there is 
a problem no matter why. In the beginning it was handy to see the 
difference but I don't think it should be kept and maintained so I 
removed it. For the update signature case a second file to sign is still 
required though.


I considered multi-byte a one word before but now I also prefer it with 
a capital b. Anyway, this name might not be the best choice and I 
changed it to LineBrokenMultiByteCharacter.


See attached patch.

Regards,
Philipp


On 26.09.2017 06:19, Weijun Wang wrote:

On Sep 26, 2017, at 1:11 AM, Philipp Kunz  wrote:

Hi Max

Thank you for the detailed assistance and I really hope it doesn't annoy you 
too much with so many beginner's mistakes. Every little point of yours seems to 
be absolutely justified in my point of view.

I'm flattered.


I did not understand where I could apply the readAllBytes. In case it still 
applies, would you give me another hint? I'd rather expect some potential for 
butifying verifyClassNameLineBroken but then I haven't found any really.

I meant you can change "while (inputStream.read() > -1);" to 
"inputStream.readAllBytes()". Yours is fine. Some people do not like the return value wasted 
and the auto-increment mechanism in the method is unnecessary here.


About where to place the declaration of nameBuf I was a little confused 
probably/obviously when there was a ByteArrayOutputStream before which, 
however, actually was not used into assuming that reusing the same buffer for 
all sections would result in better performance or so but that's certainly not 
a valid assumption now as I consider it again.

About the import static you convinced me but for statically importing 
java.nio.charset.StandardCharsets.UTF_8 which I prefer this way still. After 
you wrote I should decide myself, I removed other static imports. If there was 
kind of an unwritten rule not to use static imports generally that would be a 
compelling reason but I didn't bother to search for import static through the 
jdk sources.

I don't know if there is a rule. Yours is OK. I said it was just my habit.


The @modules was a left-over from previous versions.

I'm not sure where you meant I should put the test to exactly. In 
http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/test/jdk I cant 
find the parent folder sun you mentioned. In case there is some reorganization 
in progress not having arrived yet at the tip, could the new test be moved 
together with it? Or did you mean I should create the sun folder but then the 
existing tests would not be next to the new one? Did I look in the wrong place?

Oh sorry, we've just recently change it to 
http://hg.openjdk.java.net/jdk10/master/. You don't really need to rework this 
time, but if you want to continue hacking on OpenJDK, this is the new repo.


When updating the copyright years I was not sure if I should let the existing one, 2011, 
there in ManifestDigester or replace it with the current year which I did. When looking 
into other class files I saw none with more than two years but "," looks a 
little odd to me for indicating a range.

It is a little uncommon, but that does mean a range.


I assumed that I remove the JDK- prefix from the @bug tag and not the @test tag 
unless @bug is considered part of the @test.

Correct. @test must be there otherwise jtreg will not treat it as a test. @bug 
might be used by some reporting tools.


Is it correct, that a maximum line width of 80 characters is the convention? In 
case I added some more breaks for that. I just wonder what other style guides I 
should have respected.

Yes that is the convention. Sometimes a line can be longer if wrapping it is 
very ugly. In general, don't make a Sdiffs page in a webrev non-readable. A 
Sdiffs page example is here

   
http://cr.openjdk.java.net/~bpb/8186766/webrev.00/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c.sdiff.html


Just in case you think it would be more efficient I'd not object that a 
reviewer or some else would just change these little things to get over with 
it. On the other hand this way I learn it. If there is a suitable documentation 
that much detailed, I'd be glad if you could point me at it. That might save a 
few cycles.

I don't know if there is a more suitable doc.

I have some small suggestions:

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

> On Sep 26, 2017, at 1:11 AM, Philipp Kunz  wrote:
> 
> Hi Max
> 
> Thank you for the detailed assistance and I really hope it doesn't annoy you 
> too much with so many beginner's mistakes. Every little point of yours seems 
> to be absolutely justified in my point of view.

I'm flattered.

> 
> I did not understand where I could apply the readAllBytes. In case it still 
> applies, would you give me another hint? I'd rather expect some potential for 
> butifying verifyClassNameLineBroken but then I haven't found any really.

I meant you can change "while (inputStream.read() > -1);" to 
"inputStream.readAllBytes()". Yours is fine. Some people do not like the return 
value wasted and the auto-increment mechanism in the method is unnecessary 
here. 

> 
> About where to place the declaration of nameBuf I was a little confused 
> probably/obviously when there was a ByteArrayOutputStream before which, 
> however, actually was not used into assuming that reusing the same buffer for 
> all sections would result in better performance or so but that's certainly 
> not a valid assumption now as I consider it again.
> 
> About the import static you convinced me but for statically importing 
> java.nio.charset.StandardCharsets.UTF_8 which I prefer this way still. After 
> you wrote I should decide myself, I removed other static imports. If there 
> was kind of an unwritten rule not to use static imports generally that would 
> be a compelling reason but I didn't bother to search for import static 
> through the jdk sources.

I don't know if there is a rule. Yours is OK. I said it was just my habit.

> 
> The @modules was a left-over from previous versions.
> 
> I'm not sure where you meant I should put the test to exactly. In 
> http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/test/jdk I cant 
> find the parent folder sun you mentioned. In case there is some 
> reorganization in progress not having arrived yet at the tip, could the new 
> test be moved together with it? Or did you mean I should create the sun 
> folder but then the existing tests would not be next to the new one? Did I 
> look in the wrong place?

Oh sorry, we've just recently change it to 
http://hg.openjdk.java.net/jdk10/master/. You don't really need to rework this 
time, but if you want to continue hacking on OpenJDK, this is the new repo.

> 
> When updating the copyright years I was not sure if I should let the existing 
> one, 2011, there in ManifestDigester or replace it with the current year 
> which I did. When looking into other class files I saw none with more than 
> two years but "," looks a little odd to me for indicating a range.

It is a little uncommon, but that does mean a range.

> 
> I assumed that I remove the JDK- prefix from the @bug tag and not the @test 
> tag unless @bug is considered part of the @test.

Correct. @test must be there otherwise jtreg will not treat it as a test. @bug 
might be used by some reporting tools.

> 
> Is it correct, that a maximum line width of 80 characters is the convention? 
> In case I added some more breaks for that. I just wonder what other style 
> guides I should have respected.

Yes that is the convention. Sometimes a line can be longer if wrapping it is 
very ugly. In general, don't make a Sdiffs page in a webrev non-readable. A 
Sdiffs page example is here

  
http://cr.openjdk.java.net/~bpb/8186766/webrev.00/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c.sdiff.html

> 
> Just in case you think it would be more efficient I'd not object that a 
> reviewer or some else would just change these little things to get over with 
> it. On the other hand this way I learn it. If there is a suitable 
> documentation that much detailed, I'd be glad if you could point me at it. 
> That might save a few cycles.

I don't know if there is a more suitable doc.

I have some small suggestions:

1. In fact I don't know exactly if a non-ASCII is allowed in source code now. 
For safety, please change the é char to \u1234 style.

2. Maybe it's better to capitalize "b" in the test name?

3. The @see tag has its format, you can just use "See...". The method name in 
@link has a wrong signature.

   * @see also eAcute in
   * {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}

4. Several "throws Exception" should be indented by 8 chars.

5. There are several trailing spaces in your patch. Maybe because of 
copy-and-paste.

Please send me the new patch as an attachment. That will be more precise and 
more formal. Please send me what the exact "name " you want to see in 
the Contributed-by field of the changeset.

Thanks
Max

> 
> Regards,
> Philipp

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Wang Weijun]

Some more suggestions on the test:

1. You can use readAllBytes() to ... err ... read all bytes. 

2. I normally don’t remove intermediate files. Jtreg will do an automatic 
cleanup, and it can be configured to retain those files when the test fails. 
They will be very helpful in debugging. 

Thanks
Max 

> 在 2017年9月24日，23:12，Weijun Wang  写道：
> 
> Great.
> 
> Some suggestions, just my own habit, you are free to decide yourself:
> 
> 1. In ManifestDigester.java, I'd rather define nameBuf inside the if 
> (isNameAttr(bytes, start)) block.
> 
> 2. I normally don't use "import static" unless I can save a lot of keystrokes 
> and still do not confuse anyone. Both in the test and in 
> ManifestDigester.java.
> 
> 3. In the test, there is no need to write "@run main MultibyteUnicodeName" if 
> it is the only action (i.e. no other build/compile) and there is no modifier 
> on main (i.e. no othervm/timeout etc).
> 
> Several tiny problems:
> 
> 1. No need for @modules in the test. Maybe you used some internal classes in 
> your previous version?
> 
> 2. In the new consolidated repo, the test should be inside test/jdk/sun/..., 
> please note the "jdk" after "test".
> 
> 3. Please update the copyright years.
> 
> 4. In the test, there should be no "JDK-" in the @test tag.
> 
> Thanks
> Max
> 
>> On Sep 24, 2017, at 7:51 PM, Philipp Kunz  wrote:
>> 
>> Hi Max and Vincent
>> 
>> Thank you for your suggestions. It sure looks better now. I hope this time I 
>> got the patch added in the right format.
>> 
>> diff -r ddc25f646c2e 
>> src/java.base/share/classes/sun/security/util/ManifestDigester.java
>> --- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java
>> Thu Aug 31 22:21:20 2017 -0700
>> +++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java
>> Sun Sep 24 10:34:00 2017 +0200
>> @@ -28,6 +28,7 @@
>> import java.security.*;
>> import java.util.HashMap;
>> import java.io.ByteArrayOutputStream;
>> +import static java.nio.charset.StandardCharsets.UTF_8;
>> 
>> /**
>> * This class is used to compute digests on sections of the Manifest.
>> @@ -112,7 +113,7 @@
>>rawBytes = bytes;
>>entries = new HashMap<>();
>> 
>> -ByteArrayOutputStream baos = new ByteArrayOutputStream();
>> +ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
>> 
>>Position pos = new Position();
>> 
>> @@ -131,50 +132,40 @@
>> 
>>if (len > 6) {
>>if (isNameAttr(bytes, start)) {
>> -StringBuilder nameBuf = new StringBuilder(sectionLen);
>> +nameBuf.reset();
>> +nameBuf.write(bytes, start+6, len-6);
>> 
>> -try {
>> -nameBuf.append(
>> -new String(bytes, start+6, len-6, "UTF8"));
>> +int i = start + len;
>> +if ((i-start) < sectionLen) {
>> +if (bytes[i] == '\r') {
>> +i += 2;
>> +} else {
>> +i += 1;
>> +}
>> +}
>> 
>> -int i = start + len;
>> -if ((i-start) < sectionLen) {
>> -if (bytes[i] == '\r') {
>> -i += 2;
>> -} else {
>> -i += 1;
>> -}
>> +while ((i-start) < sectionLen) {
>> +if (bytes[i++] == ' ') {
>> +// name is wrapped
>> +int wrapStart = i;
>> +while (((i-start) < sectionLen)
>> +&& (bytes[i++] != '\n'));
>> +if (bytes[i-1] != '\n')
>> +return; // XXX: exception?
>> +int wrapLen;
>> +if (bytes[i-2] == '\r')
>> +wrapLen = i-wrapStart-2;
>> +else
>> +wrapLen = i-wrapStart-1;
>> +
>> +nameBuf.write(bytes, wrapStart, wrapLen);
>> +} else {
>> +break;
>>}
>> +}
>> 
>> -while ((i-start) < sectionLen) {
>> -if (bytes[i++] == ' ') {
>> -// name is wrapped
>> -int wrapStart = i;
>> -while (((i-start) < sectionLen)
>> -&& (bytes[i++] != '\n'));
>> -if (bytes[i-1] != '\n')
>> -return; // XXX: exception?
>> -

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

Great.

Some suggestions, just my own habit, you are free to decide yourself:

1. In ManifestDigester.java, I'd rather define nameBuf inside the if 
(isNameAttr(bytes, start)) block.

2. I normally don't use "import static" unless I can save a lot of keystrokes 
and still do not confuse anyone. Both in the test and in ManifestDigester.java.

3. In the test, there is no need to write "@run main MultibyteUnicodeName" if 
it is the only action (i.e. no other build/compile) and there is no modifier on 
main (i.e. no othervm/timeout etc).

Several tiny problems:

1. No need for @modules in the test. Maybe you used some internal classes in 
your previous version?

2. In the new consolidated repo, the test should be inside test/jdk/sun/..., 
please note the "jdk" after "test".

3. Please update the copyright years.

4. In the test, there should be no "JDK-" in the @test tag.

Thanks
Max

> On Sep 24, 2017, at 7:51 PM, Philipp Kunz  wrote:
> 
> Hi Max and Vincent
> 
> Thank you for your suggestions. It sure looks better now. I hope this time I 
> got the patch added in the right format.
> 
> diff -r ddc25f646c2e 
> src/java.base/share/classes/sun/security/util/ManifestDigester.java
> --- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java
> Thu Aug 31 22:21:20 2017 -0700
> +++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java
> Sun Sep 24 10:34:00 2017 +0200
> @@ -28,6 +28,7 @@
>  import java.security.*;
>  import java.util.HashMap;
>  import java.io.ByteArrayOutputStream;
> +import static java.nio.charset.StandardCharsets.UTF_8;
>  
>  /**
>   * This class is used to compute digests on sections of the Manifest.
> @@ -112,7 +113,7 @@
>  rawBytes = bytes;
>  entries = new HashMap<>();
>  
> -ByteArrayOutputStream baos = new ByteArrayOutputStream();
> +ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
>  
>  Position pos = new Position();
>  
> @@ -131,50 +132,40 @@
>  
>  if (len > 6) {
>  if (isNameAttr(bytes, start)) {
> -StringBuilder nameBuf = new StringBuilder(sectionLen);
> +nameBuf.reset();
> +nameBuf.write(bytes, start+6, len-6);
>  
> -try {
> -nameBuf.append(
> -new String(bytes, start+6, len-6, "UTF8"));
> +int i = start + len;
> +if ((i-start) < sectionLen) {
> +if (bytes[i] == '\r') {
> +i += 2;
> +} else {
> +i += 1;
> +}
> +}
>  
> -int i = start + len;
> -if ((i-start) < sectionLen) {
> -if (bytes[i] == '\r') {
> -i += 2;
> -} else {
> -i += 1;
> -}
> +while ((i-start) < sectionLen) {
> +if (bytes[i++] == ' ') {
> +// name is wrapped
> +int wrapStart = i;
> +while (((i-start) < sectionLen)
> +&& (bytes[i++] != '\n'));
> +if (bytes[i-1] != '\n')
> +return; // XXX: exception?
> +int wrapLen;
> +if (bytes[i-2] == '\r')
> +wrapLen = i-wrapStart-2;
> +else
> +wrapLen = i-wrapStart-1;
> +
> +nameBuf.write(bytes, wrapStart, wrapLen);
> +} else {
> +break;
>  }
> +}
>  
> -while ((i-start) < sectionLen) {
> -if (bytes[i++] == ' ') {
> -// name is wrapped
> -int wrapStart = i;
> -while (((i-start) < sectionLen)
> -&& (bytes[i++] != '\n'));
> -if (bytes[i-1] != '\n')
> -return; // XXX: exception?
> -int wrapLen;
> -if (bytes[i-2] == '\r')
> -wrapLen = i-wrapStart-2;
> -else
> -wrapLen = i-wrapStart-1;
> -
> -nameBuf.append(new String(bytes, wrapStart,
> -  wrapLen, "UTF8"));
> -} else {
> -   

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Philipp Kunz]

Hi Max and Vincent

Thank you for your suggestions. It sure looks better now. I hope this 
time I got the patch added in the right format.


diff -r ddc25f646c2e 
src/java.base/share/classes/sun/security/util/ManifestDigester.java
--- 
a/src/java.base/share/classes/sun/security/util/ManifestDigester.java 
Thu Aug 31 22:21:20 2017 -0700
+++ 
b/src/java.base/share/classes/sun/security/util/ManifestDigester.java 
Sun Sep 24 10:34:00 2017 +0200

@@ -28,6 +28,7 @@
 import java.security.*;
 import java.util.HashMap;
 import java.io.ByteArrayOutputStream;
+import static java.nio.charset.StandardCharsets.UTF_8;

 /**
  * This class is used to compute digests on sections of the Manifest.
@@ -112,7 +113,7 @@
 rawBytes = bytes;
 entries = new HashMap<>();

-ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();

 Position pos = new Position();

@@ -131,50 +132,40 @@

 if (len > 6) {
 if (isNameAttr(bytes, start)) {
-StringBuilder nameBuf = new StringBuilder(sectionLen);
+nameBuf.reset();
+nameBuf.write(bytes, start+6, len-6);

-try {
-nameBuf.append(
-new String(bytes, start+6, len-6, "UTF8"));
+int i = start + len;
+if ((i-start) < sectionLen) {
+if (bytes[i] == '\r') {
+i += 2;
+} else {
+i += 1;
+}
+}

-int i = start + len;
-if ((i-start) < sectionLen) {
-if (bytes[i] == '\r') {
-i += 2;
-} else {
-i += 1;
-}
+while ((i-start) < sectionLen) {
+if (bytes[i++] == ' ') {
+// name is wrapped
+int wrapStart = i;
+while (((i-start) < sectionLen)
+&& (bytes[i++] != '\n'));
+if (bytes[i-1] != '\n')
+return; // XXX: exception?
+int wrapLen;
+if (bytes[i-2] == '\r')
+wrapLen = i-wrapStart-2;
+else
+wrapLen = i-wrapStart-1;
+
+nameBuf.write(bytes, wrapStart, wrapLen);
+} else {
+break;
 }
+}

-while ((i-start) < sectionLen) {
-if (bytes[i++] == ' ') {
-// name is wrapped
-int wrapStart = i;
-while (((i-start) < sectionLen)
-&& (bytes[i++] != '\n'));
-if (bytes[i-1] != '\n')
-return; // XXX: exception?
-int wrapLen;
-if (bytes[i-2] == '\r')
-wrapLen = i-wrapStart-2;
-else
-wrapLen = i-wrapStart-1;
-
-nameBuf.append(new String(bytes, wrapStart,
-  wrapLen, "UTF8"));
-} else {
-break;
-}
-}
-
-entries.put(nameBuf.toString(),
-new Entry(start, sectionLen, 
sectionLenWithBlank,

-rawBytes));
-
-} catch (java.io.UnsupportedEncodingException uee) {
-throw new IllegalStateException(
-"UTF8 not available on platform");
-}
+entries.put(new String(nameBuf.toByteArray(), UTF_8),
+new Entry(start, sectionLen, 
sectionLenWithBlank, rawBytes));

 }
 }
 start = pos.startOfNext;
diff -r ddc25f646c2e 
test/sun/security/tools/jarsigner/MultibyteUnicodeName.java

--- /dev/nullThu Jan 01 00:00:00 1970 +
+++ b/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java Sun 
Sep 24 10:34:00 2017 +0200

@@ -0,0 +1,209 @@
+/*
+ * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or 

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Weijun Wang]

Hi Philipp

The change mostly looks fine. You might want to put everything into a patch 
file so Vincent can recreate a webrev and post it to cr.openjdk.java.net.

One thing I would suggest for the test is that instead of using jarsigner 
-verify and check the text in output you can open it as a JarFile, consume the 
content of an entry, and look into its getCertificates().

Also, you might want to reuse test/lib/jdk/test/lib/SecurityTools.java, and 
there is also JarUtils.java you can use. Maybe Files.copy(InputStream,Path) can 
be used in copyClassToFile().

Thanks
Max

> On Sep 20, 2017, at 7:41 AM, Philipp Kunz  wrote:
> 
> Hello Vincent
> 
> Here may be the fix for JDK-6695402. First a test.
> 
> BEGIN /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
> /*
>  * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>  *
>  * This code is free software; you can redistribute it and/or modify it
>  * under the terms of the GNU General Public License version 2 only, as
>  * published by the Free Software Foundation.
>  *
>  * This code is distributed in the hope that it will be useful, but WITHOUT
>  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>  * version 2 for more details (a copy is included in the LICENSE file that
>  * accompanied this code).
>  *
>  * You should have received a copy of the GNU General Public License version
>  * 2 along with this work; if not, write to the Free Software Foundation,
>  * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>  *
>  * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>  * or visit www.oracle.com if you need additional information or have any
>  * questions.
>  */
> 
> /*
>  * @test
>  * @bug JDK-6695402
>  * @summary verify signatures of jars containing classes with names with 
> multi-
>  *  byte unicode characters broken across lines
>  * @library /test/lib
>  * @modules jdk.jartool/sun.tools.jar
>  *  jdk.jartool/sun.security.tools.jarsigner
>  * @build jdk.test.lib.JDKToolLauncher
>  *jdk.test.lib.process.*
>  * @run main MultibyteUnicodeName
>  */
> 
> import java.io.ByteArrayOutputStream;
> import java.io.IOException;
> import java.io.InputStream;
> import java.io.UnsupportedEncodingException;
> import java.nio.file.Files;
> import java.nio.file.Paths;
> import java.util.jar.JarFile;
> import java.util.jar.Manifest;
> import java.util.stream.Collectors;
> import java.util.Arrays;
> import java.util.Map;
> import java.util.jar.Attributes.Name;
> import java.util.jar.JarEntry;
> 
> import static java.nio.charset.StandardCharsets.UTF_8;
> 
> import sun.security.tools.jarsigner.Resources;
> 
> import jdk.test.lib.JDKToolLauncher;
> import jdk.test.lib.process.OutputAnalyzer;
> import jdk.test.lib.process.ProcessTools;
> 
> public class MultibyteUnicodeName {
> 
> public static void main(String[] args) throws Throwable {
> try {
> prepare();
> 
> testSignJar("test.jar");
> testSignJarNoManifest("test-no-manifest.jar");
> testSignJarUpdate("test-update.jar");
> testSignJarWithIndex("test-index.jar");
> testSignJarAddIndex("test-add-index.jar");
> 
> } finally {
> Files.deleteIfExists(Paths.get(keystoreFileName));
> Files.deleteIfExists(Paths.get(ManifestFileName));
> Files.deleteIfExists(Paths.get(refClassFilename));
> Files.deleteIfExists(Paths.get(testClassFilename));
> }
> }
> 
> static final String alias = "a";
> static final String keystoreFileName = "test.jks";
> static final String ManifestFileName = "MANIFEST.MF";
> 
> static class A1234567890B1234567890C1234567890D12345678exyz { }
> static class A1234567890B1234567890C1234567890D12345678éxyz { }
> 
> static final String refClassFilename = 
> MultibyteUnicodeName.class.getSimpleName() + 
> "$" + 
> A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + 
> ".class";
> static final String testClassFilename = 
> MultibyteUnicodeName.class.getSimpleName() + 
> "$" + 
> A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + 
> ".class";
> 
> static void prepare() throws Throwable {
> tool("keytool", "-keystore", keystoreFileName, "-genkeypair", 
> "-storepass", "changeit",
> "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, 
> "-dname", "CN=X",
> "-validity", "366")
> .shouldHaveExitValue(0);
> 
> Files.write(Paths.get(ManifestFileName),
> (Name.MANIFEST_VERSION.toString() + ": 
> 1.0\r\n").getBytes(UTF_8.name()));
> 

Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

[Philipp Kunz]

Hello Vincent

Here may be the fix for JDK-6695402. First a test.

BEGIN /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
/*
 * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License 
version

 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/*
 * @test
 * @bug JDK-6695402
 * @summary verify signatures of jars containing classes with names 
with multi-

 *  byte unicode characters broken across lines
 * @library /test/lib
 * @modules jdk.jartool/sun.tools.jar
 *  jdk.jartool/sun.security.tools.jarsigner
 * @build jdk.test.lib.JDKToolLauncher
 *jdk.test.lib.process.*
 * @run main MultibyteUnicodeName
 */

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.jar.JarFile;
import java.util.jar.Manifest;
import java.util.stream.Collectors;
import java.util.Arrays;
import java.util.Map;
import java.util.jar.Attributes.Name;
import java.util.jar.JarEntry;

import static java.nio.charset.StandardCharsets.UTF_8;

import sun.security.tools.jarsigner.Resources;

import jdk.test.lib.JDKToolLauncher;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.process.ProcessTools;

public class MultibyteUnicodeName {

public static void main(String[] args) throws Throwable {
try {
prepare();

testSignJar("test.jar");
testSignJarNoManifest("test-no-manifest.jar");
testSignJarUpdate("test-update.jar");
testSignJarWithIndex("test-index.jar");
testSignJarAddIndex("test-add-index.jar");

} finally {
Files.deleteIfExists(Paths.get(keystoreFileName));
Files.deleteIfExists(Paths.get(ManifestFileName));
Files.deleteIfExists(Paths.get(refClassFilename));
Files.deleteIfExists(Paths.get(testClassFilename));
}
}

static final String alias = "a";
static final String keystoreFileName = "test.jks";
static final String ManifestFileName = "MANIFEST.MF";

static class A1234567890B1234567890C1234567890D12345678exyz { }
static class A1234567890B1234567890C1234567890D12345678éxyz { }

static final String refClassFilename = 
MultibyteUnicodeName.class.getSimpleName() +
"$" + 
A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + 
".class";
static final String testClassFilename = 
MultibyteUnicodeName.class.getSimpleName() +
"$" + 
A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + 
".class";


static void prepare() throws Throwable {
tool("keytool", "-keystore", keystoreFileName, "-genkeypair", 
"-storepass", "changeit",
"-keypass", "changeit", "-storetype", "JKS", "-alias", 
alias, "-dname", "CN=X",

"-validity", "366")
.shouldHaveExitValue(0);

Files.write(Paths.get(ManifestFileName),
(Name.MANIFEST_VERSION.toString() + ": 
1.0\r\n").getBytes(UTF_8.name()));

copyClassToFile(refClassFilename);
copyClassToFile(testClassFilename);
}

static void copyClassToFile(String classFilename) throws IOException {
try (
InputStream asStream = 
MultibyteUnicodeName.class.getResourceAsStream(classFilename);

ByteArrayOutputStream buf = new ByteArrayOutputStream();
) {
int b;
while ((b = asStream.read()) != -1) buf.write(b);
Files.write(Paths.get(classFilename), buf.toByteArray());
}
}

static void testSignJar(String jarFileName) throws Throwable {
try {
tool("jar", "cvfm", jarFileName,
ManifestFileName, refClassFilename, testClassFilename)
.shouldHaveExitValue(0);
verifyJarSignature(jarFileName);

} finally {
Files.deleteIfExists(Paths.get(jarFileName));
}
}

static void