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.k...@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.k...@paratix.ch <mailto: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 Tue Sep 26 07:34:55 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 digests on sections of the Manifest.
@@ -112,8 +113,6 @@
rawBytes = bytes;
entries = new HashMap<>();
- ByteArrayOutputStream baos = new ByteArrayOutputStream();
-
Position pos = new Position();
if (!findSection(0, pos))
@@ -131,50 +130,42 @@
if (len > 6) {
if (isNameAttr(bytes, start)) {
- StringBuilder nameBuf = new StringBuilder(sectionLen);
+ ByteArrayOutputStream nameBuf =
+ new ByteArrayOutputStream();
+ 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/LineBrokenMultiByteCharacter.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java Tue Sep 26 07:34:55 2017 +0200
@@ -0,0 +1,189 @@
+/*
+ * Copyright (c) 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
+ * 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 6695402
+ * @summary verify signatures of jars containing classes with names
+ * with multi-byte unicode characters broken across lines
+ * @library /test/lib
+ */
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.jar.JarFile;
+import java.util.jar.Attributes.Name;
+import java.util.jar.JarEntry;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import jdk.test.lib.SecurityTools;
+import jdk.test.lib.util.JarUtils;
+
+public class LineBrokenMultiByteCharacter {
+
+ /**
+ * this class's name will break across lines in the jar manifest at the
+ * middle of a two-byte utf encoded character due to its e acute letter
+ * at its exact position.
+ *
+ * @see #verifyClassNameLineBroken(JarFile, String)
+ */
+ static class A1234567890B1234567890C123456789D1\u00E9xyz { }
+
+ static final String testClassFilename =
+ A1234567890B1234567890C123456789D1\u00E9xyz.class
+ .getTypeName() + ".class";
+ static final String anotherClassFilename =
+ LineBrokenMultiByteCharacter.class.getTypeName() + ".class";
+
+ static final String alias = "a";
+ static final String keystoreFileName = "test.jks";
+ static final String ManifestFileName = "MANIFEST.MF";
+
+ public static void main(String[] args) throws Exception {
+ prepare();
+
+ testSignJar("test.jar");
+ testSignJarNoManifest("test-no-manifest.jar");
+ testSignJarUpdate("test-update.jar", "test-updated.jar");
+ }
+
+ static void prepare() throws Exception {
+ SecurityTools.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));
+ Files.copy(LineBrokenMultiByteCharacter.class.getResourceAsStream(
+ testClassFilename), Paths.get(testClassFilename));
+ Files.copy(LineBrokenMultiByteCharacter.class.getResourceAsStream(
+ anotherClassFilename), Paths.get(anotherClassFilename));
+ }
+
+ static void testSignJar(String jarFileName) throws Exception {
+ JarUtils.createJar(jarFileName, ManifestFileName, testClassFilename);
+ verifyJarSignature(jarFileName);
+ }
+
+ static void testSignJarNoManifest(String jarFileName) throws Exception {
+ JarUtils.createJar(jarFileName, testClassFilename);
+ verifyJarSignature(jarFileName);
+ }
+
+ static void testSignJarUpdate(
+ String initialFileName, String updatedFileName
+ ) throws Exception {
+ JarUtils.createJar(initialFileName, ManifestFileName,
+ anotherClassFilename);
+ SecurityTools.jarsigner("-keystore", keystoreFileName, "-storetype",
+ "JKS", "-storepass", "changeit", "-debug", initialFileName,
+ alias)
+ .shouldHaveExitValue(0);
+ JarUtils.updateJar(initialFileName, updatedFileName,
+ testClassFilename);
+ verifyJarSignature(updatedFileName);
+ }
+
+ static void verifyJarSignature(String jarFileName) throws Exception {
+ // actually sign the jar
+ SecurityTools.jarsigner("-keystore", keystoreFileName, "-storetype",
+ "JKS", "-storepass", "changeit", "-debug", jarFileName, alias)
+ .shouldHaveExitValue(0);
+
+ try (
+ JarFile jar = new JarFile(jarFileName);
+ ) {
+ verifyClassNameLineBroken(jar, testClassFilename);
+ verifyCodeSigners(jar, jar.getJarEntry(testClassFilename));
+ }
+ }
+
+ /**
+ * it would be too easy to miss the actual test case by just renaming an
+ * identifier so that the multi-byte encoded character would not any longer
+ * be broken across a line break.
+ *
+ * this test verifies that the actual test case is tested based on the
+ * manifest and not based on the signature file because at the moment, the
+ * signature file does not even contain the desired entry at all.
+ *
+ * this relies on {@link Manifest} breaking lines unaware of bytes that
+ * belong to the same multi-byte utf characters.
+ */
+ static void verifyClassNameLineBroken(JarFile jar, String className)
+ throws IOException {
+ byte[] eAcute = "\u00E9".getBytes(UTF_8); // e acute
+ byte[] eAcuteBroken =
+ new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
+
+ if (jar.getManifest().getAttributes(className) == null) {
+ throw new AssertionError(className + " not found in manifest");
+ }
+
+ JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
+ try (
+ InputStream manifestIs = jar.getInputStream(manifestEntry);
+ ) {
+ int bytesMatched = 0;
+ for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
+ if ((byte) b == eAcuteBroken[bytesMatched]) {
+ bytesMatched++;
+ if (bytesMatched == eAcuteBroken.length) {
+ break;
+ }
+ } else {
+ bytesMatched = 0;
+ }
+ }
+ if (bytesMatched < eAcuteBroken.length) {
+ throw new AssertionError("self-test failed: multi-byte "
+ + "utf-8 character not broken across lines");
+ }
+ }
+ }
+
+ static void verifyCodeSigners(JarFile jar, JarEntry jarEntry)
+ throws IOException {
+ // codeSigners is initialized only after the entry has been read
+ try (
+ InputStream inputStream = jar.getInputStream(jarEntry);
+ ) {
+ inputStream.readAllBytes();
+ }
+
+ // a check for the presence of code signers is sufficient to check
+ // bug JDK-6695402. no need to also verify the actual code signers
+ // attributes here.
+ if (jarEntry.getCodeSigners() == null
+ || jarEntry.getCodeSigners().length == 0) {
+ throw new AssertionError(
+ "no signing certificate found for " + jarEntry.getName());
+ }
+ }
+
+}
