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

Philipp Kunz philipp.kunz at paratix.ch
Tue Sep 26 05:37:15 UTC 2017


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 <philipp.kunz at paratix.ch> 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 <email>" you want to see in the Contributed-by field of the changeset.
>
> Thanks
> Max
>
>> Regards,
>> Philipp

-- 


Gruss Philipp



------------------------------------------------------------------------



Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
philipp.kunz at paratix.ch <mailto:philipp.kunz at paratix.ch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20170926/60de7d56/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/png
Size: 5134 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20170926/60de7d56/attachment-0001.png>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: JDK-6695402.patch
Type: text/x-patch
Size: 12681 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20170926/60de7d56/JDK-6695402-0001.patch>


More information about the security-dev mailing list