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 <philipp.k...@paratix.ch> 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 {
> - 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/null Thu Jan 01 00:00:00 1970 +0000
> +++ 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 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
> + * @run main MultibyteUnicodeName
> + */
> +
> +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.Manifest;
> +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 static jdk.test.lib.SecurityTools.jarsigner;
> +import static jdk.test.lib.SecurityTools.keytool;
> +import static jdk.test.lib.util.JarUtils.createJar;
> +import static jdk.test.lib.util.JarUtils.updateJar;
> +
> +public class MultibyteUnicodeName {
> +
> + /**
> + * 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 also eAcute in {@link
> MultibyteUnicodeName#verifyClassNameLineBroken(String)}
> + */
> + static class A1234567890B1234567890C1234567890D12345678éxyz { }
> + static class A1234567890B1234567890C1234567890D12345678exyz { }
> +
> + static final String refClassFilename =
> MultibyteUnicodeName.class.getSimpleName() +
> + "$" +
> A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() +
> ".class";
> + static final String testClassFilename =
> MultibyteUnicodeName.class.getSimpleName() +
> + "$" +
> A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() +
> ".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 {
> + try {
> + prepare();
> +
> + testSignJar("test.jar");
> + testSignJarNoManifest("test-no-manifest.jar");
> + testSignJarUpdate("test-update.jar", "test-updated.jar");
> +
> + } finally {
> + Files.deleteIfExists(Paths.get(keystoreFileName));
> + Files.deleteIfExists(Paths.get(ManifestFileName));
> + Files.deleteIfExists(Paths.get(refClassFilename));
> + Files.deleteIfExists(Paths.get(testClassFilename));
> + }
> + }
> +
> + static void prepare() throws Exception {
> + 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()));
> +
> Files.copy(MultibyteUnicodeName.class.getResourceAsStream(refClassFilename),
> + Paths.get(refClassFilename));
> +
> Files.copy(MultibyteUnicodeName.class.getResourceAsStream(testClassFilename),
> + Paths.get(testClassFilename));
> + }
> +
> + static void testSignJar(String jarFileName) throws Exception {
> + try {
> + createJar(jarFileName, ManifestFileName, refClassFilename,
> testClassFilename);
> + verifyJarSignature(jarFileName);
> +
> + } finally {
> + Files.deleteIfExists(Paths.get(jarFileName));
> + }
> + }
> +
> + static void testSignJarNoManifest(String jarFileName) throws Exception {
> + try {
> + createJar(jarFileName, refClassFilename, testClassFilename);
> + verifyJarSignature(jarFileName);
> +
> + } finally {
> + Files.deleteIfExists(Paths.get(jarFileName));
> + }
> + }
> +
> + static void testSignJarUpdate(String initialFileName, String
> updatedFileName) throws Exception {
> + try {
> + createJar(initialFileName, ManifestFileName, refClassFilename);
> + jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
> + "-storepass", "changeit", "-debug", initialFileName,
> alias)
> + .shouldHaveExitValue(0);
> + updateJar(initialFileName, updatedFileName, testClassFilename);
> + verifyJarSignature(updatedFileName);
> +
> + } finally {
> + Files.deleteIfExists(Paths.get(initialFileName));
> + Files.deleteIfExists(Paths.get(updatedFileName));
> + }
> + }
> +
> + static void verifyJarSignature(String jarFileName) throws Exception {
> + // actually sign the jar
> + 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));
> + verifyCodeSigners(jar, jar.getJarEntry(refClassFilename));
> + }
> + }
> +
> + /**
> + * 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 the Manifest breaking lines unaware of bytes that
> belong to the same
> + * multi-ybte utf characters.
> + */
> + static void verifyClassNameLineBroken(JarFile jar, String className)
> throws IOException {
> + byte[] eAcute = "é".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);
> + ) {
> + while (inputStream.read() > -1);
> +
> + if (jarEntry.getCodeSigners() == null ||
> jarEntry.getCodeSigners().length == 0) {
> + throw new AssertionError("no signing certificate found for "
> + jarEntry.getName());
> + }
> + }
> +
> + // 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.
> + }
> +
> +}
>
> Regards,
> Philipp
>
>
>
> On 20.09.2017 03:41, Weijun Wang wrote:
>> 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 <philipp.k...@paratix.ch>
>>> 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()));
>>> 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 testSignJarNoManifest(String jarFileName) throws Throwable {
>>> try {
>>> tool("jar", "cvf", jarFileName, refClassFilename,
>>> testClassFilename)
>>> .shouldHaveExitValue(0);
>>> verifyJarSignature(jarFileName);
>>>
>>> } finally {
>>> Files.deleteIfExists(Paths.get(jarFileName));
>>> }
>>> }
>>>
>>> static void testSignJarUpdate(String jarFileName) throws Throwable {
>>> try {
>>> tool("jar", "cvfm", jarFileName, ManifestFileName,
>>> refClassFilename)
>>> .shouldHaveExitValue(0);
>>> tool("jarsigner", "-keystore", keystoreFileName, "-storetype",
>>> "JKS",
>>> "-storepass", "changeit", "-debug", jarFileName, alias)
>>> .shouldHaveExitValue(0);
>>> tool("jar", "uvf", jarFileName, testClassFilename)
>>> .shouldHaveExitValue(0);
>>> verifyJarSignature(jarFileName);
>>>
>>> } finally {
>>> Files.deleteIfExists(Paths.get(jarFileName));
>>> }
>>> }
>>>
>>> static void testSignJarWithIndex(String jarFileName) throws Throwable {
>>> try {
>>> tool("jar", "cvfm", jarFileName,
>>> ManifestFileName, refClassFilename, testClassFilename)
>>> .shouldHaveExitValue(0);
>>> tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
>>> verifyJarSignature(jarFileName);
>>>
>>> } finally {
>>> Files.deleteIfExists(Paths.get(jarFileName));
>>> }
>>> }
>>>
>>> static void testSignJarAddIndex(String jarFileName) throws Throwable {
>>> try {
>>> tool("jar", "cvfm", jarFileName,
>>> ManifestFileName, refClassFilename, testClassFilename)
>>> .shouldHaveExitValue(0);
>>> tool("jarsigner", "-keystore", keystoreFileName, "-storetype",
>>> "JKS",
>>> "-storepass", "changeit", "-debug", jarFileName, alias)
>>> .shouldHaveExitValue(0);
>>> tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
>>> verifyJarSignature(jarFileName);
>>>
>>> } finally {
>>> Files.deleteIfExists(Paths.get(jarFileName));
>>> }
>>> }
>>>
>>> static Map<String, String> jarsignerResources = Arrays.stream(new
>>> Resources().getContents()).
>>> collect(Collectors.toMap(e -> (String) e[0], e -> (String)
>>> e[1]));
>>>
>>> static void verifyJarSignature(String jarFileName) throws Throwable {
>>> // actually sign the jar
>>> tool("jarsigner", "-keystore", keystoreFileName, "-storetype",
>>> "JKS",
>>> "-storepass", "changeit", "-debug", jarFileName, alias)
>>> .shouldHaveExitValue(0);
>>>
>>> verifyClassNameLineBroken(jarFileName);
>>>
>>> // check for no unsigned entries
>>> tool("jarsigner", "-verify", "-keystore", keystoreFileName,
>>> "-storetype", "JKS",
>>> "-storepass", "changeit", "-debug", jarFileName, alias)
>>> .shouldHaveExitValue(0)
>>> .shouldNotContain(jarsignerResources.get(
>>>
>>> "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));
>>>
>>> // check that both classes with and without multi-byte unicode
>>> characters in their names are signed
>>> tool("jarsigner", "-verify", "-verbose", "-keystore",
>>> keystoreFileName, "-storetype", "JKS",
>>> "-storepass", "changeit", "-debug", jarFileName, alias)
>>> .shouldHaveExitValue(0)
>>> .shouldMatch("^" + jarsignerResources.get("s") +
>>> jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" +
>>> refClassFilename.replaceAll("\\$", "\\\\\\$") + "$")
>>> .shouldMatch("^" + jarsignerResources.get("s") +
>>> jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" +
>>> testClassFilename.replaceAll("\\$", "\\\\\\$") + "$");
>>>
>>> // check that both classes with and without multi-byte unicode
>>> characters in their names have signing certificates
>>> tool("jarsigner", "-verify", "-verbose", "-certs", "-keystore",
>>> keystoreFileName,
>>> "-storetype", "JKS", "-storepass", "changeit", "-debug",
>>> jarFileName, alias)
>>> .shouldHaveExitValue(0)
>>> .shouldMatch(refClassFilename.replaceAll("\\$", "\\\\\\$")
>>> + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + "
>>> .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)")
>>> .shouldMatch(testClassFilename.replaceAll("\\$", "\\\\\\$")
>>> + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + "
>>> .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)");
>>> }
>>>
>>> /**
>>> * 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 the Manifest breaking lines unaware of bytes that
>>> belong to the same
>>> * multi-ybte utf characters.
>>> */
>>> static void verifyClassNameLineBroken(String jarFileName) throws
>>> IOException {
>>> byte[] eAcute = "é".getBytes(UTF_8);
>>> byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ',
>>> eAcute[1]};
>>>
>>> try (
>>> JarFile jar = new JarFile(jarFileName);
>>> ) {
>>> if (jar.getManifest().getAttributes(testClassFilename) == null)
>>> {
>>> throw new AssertionError(testClassFilename + " 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 OutputAnalyzer tool(String tool, String... args)
>>> throws Throwable {
>>> JDKToolLauncher l = JDKToolLauncher.createUsingTestJDK(tool);
>>> for (String arg : args) {
>>> if (arg .startsWith("-J")) {
>>> l.addVMArg(arg.substring(2));
>>> } else {
>>> l.addToolArg(arg);
>>> }
>>> }
>>> return ProcessTools.executeCommand(l.getCommand());
>>> }
>>>
>>> }
>>> END /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
>>>
>>>
>>> There are three e with acutes, on lines 84, 89, and 227, just in case the
>>> mail suffers bad encoding which might not be absolutely unconceivable
>>> regarding the bug's subject.
>>>
>>> In contrast to the bug description it uses a nested class with a two-byte
>>> UTF-8 character rather than one in its own file. I chose to do it that way
>>> because then the complete test goes into one file and therefore I assume
>>> the overview is kept easier. The test still demonstrates exactly
>>> JDK-6695402's problem.
>>>
>>> It may not have been necessary to also test jar files with indexes but i
>>> consider it necessary to have signing unsigned as well as partially signed
>>> jars tested, so why not have one more case tested, too?
>>>
>>> There might also be a more elegant approach to get class files into a jar
>>> file as to get their contents through the class loader. I chose to use real
>>> class files rather than dummy contents in files named *.class or for
>>> instance plain text files in the jar the signing of which to be tested in
>>> order to stay as close to the original bug problem as possible even though
>>> I don't have any notion that it would make a difference. The amount of code
>>> used to copy the class file contents around is comparatively small with
>>> respect to the whole test case amount of code.
>>>
>>> For the demonstration that the multi-byte character actually makes the
>>> alleged difference, I concluded that it was necessary to have another class
>>> name not previously affected by the bug in order to compare the effects of
>>> just different names. Otherwise the signing could fail to any other reason
>>> undetectably.
>>>
>>> In order to express a condition to tell successful from failed test runs
>>> the best approach I found was to analyze the jarsigner tool's output which
>>> is in my opinion not the most desirable option. I'd have preferred output
>>> from a clearer structured api but then I guess the output format will not
>>> change too often in the foreseeable future. For instance the check for the
>>> s, m, and k flags is more pragmatical approach than obviously perfectly
>>> failsafe when operating with regular expressions to match it with the
>>> output. Other parts such as 'X.509' and 'CN=' are not even jarsigner tool
>>> resources. I put at least a reference to
>>> sun.security.tools.jarsigner.Resources.
>>>
>>> Finally, I afforded kind of a self-test that protects the test from
>>> undetected failing because renaming the main class which would move the
>>> two-byte unicode character to a position not suitable for the test. It may
>>> not be absolutely necessary.
>>>
>>>
>>>
>>> BEGIN PATH
>>> 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 20 00:56:15 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;
>>> END PATCH
>>>
>>>
>>> The patch is mostly so big because indentation has changed in many lines.
>>>
>>> There was baos there before without apparent use. The patch renames it and
>>> puts it into use. It came in very handy because if it would not have been
>>> there already, I would have had to defined one of my own.
>>>
>>> The main point of the patch is that manifest section names that are broken
>>> across lines at possibly arbitrary bytes are no longer converted into
>>> strings for each manifest line and then joined. Parts of names broken
>>> across lines are now joined first when they are still byte sequences and
>>> only when the complete byte sequence is available after processing all
>>> manifest lines that contain the same manifest section name decoded into a
>>> unicode string.
>>>
>>> For decoding the bytes into a string I chose a different string constructor
>>> than was used before that does not any longer declare
>>> UnsupportedEncodingException rendering the try/catch redundant. It couldn't
>>> have ever occurred anyway taking into consideration that UTF-8 is mandatory
>>> for every Java platform, StandardCharsets says. The difference according to
>>> the documentation is that the previously used String constructor
>>> returned undefined strings for invalid byte sequences whereas the one used
>>> in the patch will replace unparseable portions with valid 'unknown'
>>> characters.
>>>
>>> One question I cannot still answer yet is how the ManifestDigester can be
>>> changed at all without complete test coverage or risking to break existing
>>> signatures. I already started on that but it's quite a piece of work to
>>> almost formally prove that all manifests will continue to produce identical
>>> digests, except of course for the ones now fixed.
>>>
>>> Regards,
>>> Philipp
>>>
>>>
>>> On 17.09.2017 21:25, Philipp Kunz wrote:
>>>
>>>> Hello Vincent
>>>>
>>>> I narrowed the error down so far and suspect now that it is an effect of
>>>> sun.security.util.ManifestDigester's constructor, lines 134 and 163-164,
>>>> in combination with java.util.jar.Manifest.make72Safe. Manifest breaks
>>>> multi-byte characters in manifest section names across lines and
>>>> ManifestDigester fails to restore them correctly, which both sounds wrong
>>>> but ManifestDigester sure is.
>>>>
>>>> Just fixing it would be too tempting but then I would not know (I mean not
>>>> know for sure with evidence from tests) if any existing jar-signature
>>>> would still be valid and I don't want to break existing signatures. When I
>>>> had a look at the tests specific for Manifest and ManifestDigester I found
>>>> very little. There are many more different situations and corner cases and
>>>> combinations thereof to cover with tests than it looks like at first
>>>> glance so that writing tests that cover all relevant cases looks to me
>>>> like quite an effort. I have the impression that test coverage was not a
>>>> priority when the subjected code was developed or at least the tests might
>>>> not have been contributed to the open JDK project. Hence, while I'm
>>>> continuing to complete these tests I miss, if someone has an idea
>>>> how to simplify that so that I can still convince at least myself that no
>>>> existing signature will break or where possibly more testcases are that I
>>>> haven't discovered so far, please let me know.
>>>>
>>>> I'm also wondering how much performance should be taken into
>>>> consideration. There are a few hints such as reusing byte arrays that
>>>> suggest that it is an issue. Will a patch be rejected if it slows down
>>>> some tests beyond a certain limit for so little added value or what might
>>>> be the acceptance criteria here? I don't expect insuperable difficulties
>>>> with performance but would still appreciate some general idea.
>>>>
>>>> My desired or currently preferred approach to fix JDK-6695402 would be to
>>>> let ManifestDigester any kind of use or extend Manifest in order to let
>>>> Manifest identify the manifest sections thereby preventing the parsing
>>>> duplicated that identifies the manifest sections.
>>>>
>>>> As a new contributor I could use and would appreciate some guidance
>>>> particularly about what kind and completeness of test coverage and
>>>> performance tuning applies or is suggested in the context of the given
>>>> bug. Otherwise I fear I might contribute too many tests which would have
>>>> to be reviewed and maintained or quite some work would be for nothing if a
>>>> patch would not satisfy performance requirements.
>>>>
>>>> Regards,
>>>> Philipp
>>>>
>>>>
>>>>
>>>> On 01.09.2017 15:20, Vincent Ryan wrote:
>>>>
>>>>> That all sounds fine. Let me know when your patch is ready to submit.
>>>>> Thanks.
>>>>>
>>>>>
>>>>>
>>>>>> On 1 Sep 2017, at 13:15, Philipp Kunz <philipp.k...@paratix.ch>
>>>>>> wrote:
>>>>>>
>>>>>> Hello Vincent
>>>>>>
>>>>>> Thank you for sponsoring!
>>>>>> So far, I have become a contributor by signing the OCA which has been
>>>>>> accepted. Dalibor Topic wrote that he can confirm and it's also here:
>>>>>> http://www.oracle.com/technetwork/community/oca-486395.html#p
>>>>>> -> Paratix GmbH
>>>>>> Therefore I think I have followed the steps in
>>>>>> http://openjdk.java.net/contribute/
>>>>>> at least the ones before actual patch submission.
>>>>>>
>>>>>> Currently, I'm working on getting the existing test cases running
>>>>>> locally. Unfortunately, I started with jdk 9 and switched to 10 now. I
>>>>>> figured these commands run at least the relevant tests with 9 and hope
>>>>>> this also applies to 10:
>>>>>> make run-test-tier1
>>>>>> make run-test TEST="jdk/test"
>>>>>> they report errors and failures but it hasn't been completed and
>>>>>> released which might explain it. If I run the tests I assume relevant
>>>>>> for my patch
>>>>>> make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner
>>>>>> jtreg:jdk/test/java/util/jar"
>>>>>> then it reports zero errors and failures which may be a good starting
>>>>>> point.
>>>>>> Do you think that sounds reasonable or do you have another suggestion
>>>>>> how to run the tests?
>>>>>>
>>>>>> Next, I will add a test for JDK-6695402 before actually fixing it. As an
>>>>>> example, I'll try something in the style of
>>>>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java
>>>>>> . This way, I try to demonstrate the improvement.
>>>>>>
>>>>>> I guess I have identified the following line as the cause: value = new
>>>>>> String(vb, 0, 0, vb.length);
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
>>>>>>
>>>>>> So I'll try to remove it first including the whole four line if block.
>>>>>>
>>>>>> Philipp
>>>>>>
>>>>>>
>>>>>> On 01.09.2017 10:00, Vincent Ryan wrote:
>>>>>>
>>>>>>> Hello Philipp,
>>>>>>>
>>>>>>> I’m happy to sponsor your fix for JDK 10. Have you followed these
>>>>>>> steps:
>>>>>>> http://openjdk.java.net/contribute/
>>>>>>> ?
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On 1 Sep 2017, at 08:58, Vincent Ryan <vincent.x.r...@oracle.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Moved to security-dev
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 1 Sep 2017, at 08:28, Philipp Kunz <philipp.k...@paratix.ch>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hello everyone
>>>>>>>>>
>>>>>>>>> I have been developing with Java for around 17 years now and when I
>>>>>>>>> encountered some bug I decided to attempt to fix it:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6695402
>>>>>>>>> . This also looks like it may not be too big a piece for a first
>>>>>>>>> contribution.
>>>>>>>>>
>>>>>>>>> I read through quite some guides and all kinds of documents but could
>>>>>>>>> not yet help myself with the following questions:
>>>>>>>>>
>>>>>>>>> May I login to jira to add comments to bugs? If so, how would I
>>>>>>>>> request or receive credentials? Or are mailing lists preferred?
>>>>>>>>>
>>>>>>>>> Another question is whether I should apply it to jdk9, but it may be
>>>>>>>>> too late now, or to jdk10, and backporting can be considered later.
>>>>>>>>> Probably it wouldn't even make much a difference for the patch itself.
>>>>>>>>>
>>>>>>>>> One more question I have is how or where to find the sources from
>>>>>>>>> before migration to mercurial. Because some lines of code I intend to
>>>>>>>>> change go back farther and in the history I find only 'initial
>>>>>>>>> commit'. With such a history I might be able better to understand why
>>>>>>>>> it's there and prevent to make the same mistake again.
>>>>>>>>>
>>>>>>>>> I guess the appropriate mailing list for above mentioned bug is
>>>>>>>>> security-dev. Is it correct that I can send a patch there and just
>>>>>>>>> hope for some sponsor to pick it up? Of course I'd be glad if some
>>>>>>>>> sponsor would contact me and maybe provide some assistance or if
>>>>>>>>> someone would confirm that sending a patch to the mailing list is the
>>>>>>>>> right way to find a sponsor.
>>>>>>>>>
>>>>>>>>> Philipp Kunz
>>>>>>>>>
>>>>
>>>>
>>>>
>>>>
>>>> <Mail Attachment.png>
>>>>
>>>> Paratix GmbH
>>>> St Peterhofstatt 11
>>>> 8001 Zürich
>>>>
>>>> +41 (0)76 397 79 35
>>>>
>>>> philipp.k...@paratix.ch
>
> --
>
>
> Gruss Philipp
>
>
>
>
> <Mail Attachment.png>
>
> Paratix GmbH
> St Peterhofstatt 11
> 8001 Zürich
>
> +41 (0)76 397 79 35
> philipp.k...@paratix.ch
