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