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 <weijun.w...@oracle.com> 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 <weijun.w...@oracle.com> 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 <weijun.w...@oracle.com> wrote:
On Sep 26, 2017, at 1:37 PM, Philipp Kunz <philipp.k...@paratix.ch> 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 <philipp.k...@paratix.ch>" 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 <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 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 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,41 @@
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,
+ entries.put(new String(nameBuf.toByteArray(), UTF_8),
+ new Entry(start, sectionLen, sectionLenWithBlank,
rawBytes));
-
- } catch (java.io.UnsupportedEncodingException uee) {
- throw new IllegalStateException(
- "UTF8 not available on platform");
- }
}
}
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 Wed Sep 27 07:40:53 2017 +0200
@@ -0,0 +1,185 @@
+/*
+ * 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 name will break across lines in MANIFEST.MF at the
+ * middle of a two-byte utf-8 encoded character due to its e acute letter
+ * at its exact position.
+ *
+ * because no file with such a name exists {@link JarUtils} will add the
+ * name itself as contents to the jar entry which would have contained a
+ * compiled class in the original bug. For this test, the contents of the
+ * files contained in the jar file is not important as long as they get
+ * signed.
+ *
+ * @see #verifyClassNameLineBroken(JarFile, String)
+ */
+ static final String testClassName =
+ "LineBrokenMultiByteCharacterA1234567890B1234567890C123456789D12\u00E9xyz.class";
+
+ static final String anotherName =
+ "LineBrokenMultiByteCharacterA1234567890B1234567890C123456789D1234567890.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));
+ }
+
+ static void testSignJar(String jarFileName) throws Exception {
+ JarUtils.createJar(jarFileName, manifestFileName, testClassName);
+ verifyJarSignature(jarFileName);
+ }
+
+ static void testSignJarNoManifest(String jarFileName) throws Exception {
+ JarUtils.createJar(jarFileName, testClassName);
+ verifyJarSignature(jarFileName);
+ }
+
+ static void testSignJarUpdate(
+ String initialFileName, String updatedFileName) throws Exception {
+ JarUtils.createJar(initialFileName, manifestFileName, anotherName);
+ SecurityTools.jarsigner("-keystore", keystoreFileName, "-storetype",
+ "JKS", "-storepass", "changeit", "-debug", initialFileName,
+ alias).shouldHaveExitValue(0);
+ JarUtils.updateJar(initialFileName, updatedFileName, testClassName);
+ 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, testClassName);
+ verifyCodeSigners(jar, jar.getJarEntry(testClassName));
+ }
+ }
+
+ /**
+ * 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 check here 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 java.util.jar.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);
+ 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());
+ }
+ }
+
+}
