Hi everyone,
After I haven't found a sponsor so far, I try to make it even easier
this time by providing a hopefully ready patch this time. See
http://files.paratix.ch/jdk/6372077/webrev.01/ or the attached
6372077.patch.
In short, this patch increases the manifest files line width by two
bytes in order to provide enough space for up to 70 bytes of a header
name and the following two bytes of the ": " delimiter in order to
fulfill the manifest specification that does not allow header names and
the delimiter to be broken across lines in
Manifest#make72Safe(StringBuffer). It also changes that make72Safe is
now not anymore invoked with the trailing line break in the StringBuffer
argument but appended only after that call which makes make72Safe
simpler by one if and one subtraction and affects three places where it
is called.
While the bug says in its title it relates to JarFile.getManifest() it
is actually caused in class Manifest. Therefore I added a test about
exactly what the bug says in Bug6372077jarFileManifestAttr70 along with
another test LineBreakLineWidth that covers the actual fix.
All my other considerations and reasoning are already written down in
either the preceding mails included below or as comments in the patch.
I'd be glad to get any kind of feedback and am looking forward to
closing another bug.
Regards,
Philipp
On 21.11.2017 07:18, Philipp Kunz wrote:
Hi everyone,
I haven't got any reply now for around three weeks and now i start to
wonder if I just missed it or if I should refine my approach to find a
sponsor or if it helped if I presented a ready patch or if noone
considers this important enough or anything else whatever. This is
only my second contribution hence I don't know the procedure well.
One point maybe worth to point out again is that I don't want to
support manifest headers longer than 70 character, just up to 70,
which has always been the intention but has only worked up to 68. This
might have been written confusingly in my last email.
As far as I understood, I should first get a sponsor. In any case, is
there any suggestion for how to proceed?
Regards,
Philipp
On 03.11.2017 00:04, Philipp Kunz wrote:
Hi Sean and Max and all others,
Thank you Sean for the hint about the right mailing list. And thanks
also for his hint to Max to make smaller portions of changes.
I would like to contribute a fix for JDK-6372077 which is about
JarFile.getManifest() should handle manifest attribute name[s longer
than] 70 bytes.
It looks like the bug is caused by Manifest.make72Safe breaking lines
at 70 bytes instead of 72 despite its comment and name
(http://hg.openjdk.java.net/jdk10/master/file/tip/src/java.base/share/classes/java/util/jar/Manifest.java#l176).The
resulting StringBuffer has then lines of 72 bytes maximum each
including the following line break. Without the line break that
leaves 70 bytes of characters per line. On the other hand, header
names can be up to 70 bytes (only single-byte utf-8 characters) and
cannot be broken across a line break and need to be followed by a
colon and a space which must be on the same line too according to the
specification. When breaking at 70 bytes excluding the line break,
however, long header names don't fit in one line together with the
colon space delimiter because there is not sufficient space.
Manifests with names up to 70 bytes long can still be written without
immediate exception but the resulting manifests are illegal in my
opinion. When later reading such manifests
(http://hg.openjdk.java.net/jdk10/master/file/tip/src/java.base/share/classes/java/util/jar/Attributes.java#l406),
an error occurs as a consequence of the bad manifest. This is more or
less the current situation and also what JDK-6372077 already knew.
--> After all, in order to fix it, i'd like to propose to make
manifest file lines wider by two bytes.
The only proper alternative that came into my mind would be to change
the manifest specification and reduce the maximum header name length
there by two and also in the code. If that would break any existing
code i guess that would affect code only that produced invalid
manifests and would be acceptable.
Supporting all existing and possibly invalid manifests would mean to
add support for reading headers the delimiters of which are broken
onto the next line which I consider too complex with respect to the
value added and even more so considering that those invalid manifest
can be assumed to have been detected as such by reading them and also
because it would be a feature that would be used less and less over
time if the code to write manifest is changed at the same time to
produce only valid manifests in the discussed respect here. I don't
think this should be actually done.
Before I actually do the leg work, i'd like to ask, if there are
concerns or objections or tips for such a change or if anyone can or
cannot follow the reasoning and the conclusion to make manifests 2
bytes wider or if i missed an important point altogether.
In case there will be a consent about how to solve this, would
someone volunteer to sponsor? That may be less urgent at the moment
than the question above about how to proceed.
Philipp
diff -r be620a591379 src/java.base/share/classes/java/util/jar/Attributes.java
--- a/src/java.base/share/classes/java/util/jar/Attributes.java Mon Oct 30 21:23:10 2017 +0100
+++ b/src/java.base/share/classes/java/util/jar/Attributes.java Fri Dec 08 23:39:08 2017 +0100
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2015, 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
@@ -311,9 +311,8 @@
value = new String(vb, 0, 0, vb.length);
}
buffer.append(value);
-
+ Manifest.make72Safe(buffer);
buffer.append("\r\n");
- Manifest.make72Safe(buffer);
os.writeBytes(buffer.toString());
}
os.writeBytes("\r\n");
@@ -356,9 +355,8 @@
value = new String(vb, 0, 0, vb.length);
}
buffer.append(value);
-
+ Manifest.make72Safe(buffer);
buffer.append("\r\n");
- Manifest.make72Safe(buffer);
out.writeBytes(buffer.toString());
}
}
diff -r be620a591379 src/java.base/share/classes/java/util/jar/Manifest.java
--- a/src/java.base/share/classes/java/util/jar/Manifest.java Mon Oct 30 21:23:10 2017 +0100
+++ b/src/java.base/share/classes/java/util/jar/Manifest.java Fri Dec 08 23:39:08 2017 +0100
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2013, 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
@@ -157,8 +157,8 @@
value = new String(vb, 0, 0, vb.length);
}
buffer.append(value);
+ make72Safe(buffer);
buffer.append("\r\n");
- make72Safe(buffer);
dos.writeBytes(buffer.toString());
e.getValue().write(dos);
}
@@ -170,13 +170,11 @@
*/
static void make72Safe(StringBuffer line) {
int length = line.length();
- if (length > 72) {
- int index = 70;
- while (index < length - 2) {
- line.insert(index, "\r\n ");
- index += 72;
- length += 3;
- }
+ int index = 72;
+ while (index < length) {
+ line.insert(index, "\r\n ");
+ index += 74; // + line width + line break ("\r\n")
+ length += 3; // + line break ("\r\n") and space
}
return;
}
diff -r be620a591379 test/jdk/java/util/jar/JarFile/Bug6372077jarFileManifestAttr70.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/util/jar/JarFile/Bug6372077jarFileManifestAttr70.java Fri Dec 08 23:39:08 2017 +0100
@@ -0,0 +1,75 @@
+/*
+ * 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.
+ */
+
+import static java.util.jar.Attributes.Name.MANIFEST_VERSION;
+
+import java.io.IOException;
+import java.nio.file.Paths;
+import java.util.jar.JarFile;
+import java.util.jar.Manifest;
+
+import org.testng.annotations.Test;
+
+/**
+ * @test
+ * @bug 6372077
+ * @library /lib/testlibrary
+ * @build JarUtils
+ * @run testng Bug6372077jarFileManifestAttr70
+ * @summary tests JarFile.getManifest() with a manifest attribute with 70
+ * characters/bytes in its manifest
+ */
+public class Bug6372077jarFileManifestAttr70 {
+
+ @Test
+ public void readJarFileManifestAttr70() throws IOException {
+ /*
+ * string with 70 (single-byte utf-8 characters) as it says in the
+ * bug description and actually comes from
+ * Attributes.Name#isValid(String)
+ */
+ String name70 = "1ABCDEFGEH2ABCDEFGEH3ABCDEFGEH"
+ + "4ABCDEFGEH5ABCDEFGEH6ABCDEFGEH7ABCDEFGEH";
+
+ Manifest mf = new Manifest();
+ mf.getMainAttributes().putValue(MANIFEST_VERSION.toString(), "1.0");
+ mf.getMainAttributes().putValue(name70, "some value");
+ mf.write(System.out);
+
+ String jarFileName = "manifattr70.jar";
+ JarUtils.createJarFile(Paths.get(jarFileName), mf, Paths.get("."));
+ try (JarFile jar = new JarFile(jarFileName)) {
+ jar.getManifest();
+ /*
+ * bug 6372077:
+ * java.io.IOException: invalid header field
+ * at java.base/java.util.jar.Attributes.read(Attributes.java:408)
+ * at java.base/java.util.jar.Manifest.read(Manifest.java:209)
+ * at java.base/java.util.jar.Manifest.<init>(Manifest.java:69)
+ * at java.base/java.util.jar.JarFile.getManifestFromReference(JarFile.java:413)
+ * at java.base/java.util.jar.JarFile.getManifest(JarFile.java:399)
+ */
+ }
+ }
+
+}
\ No newline at end of file
diff -r be620a591379 test/jdk/java/util/jar/Manifest/LineBreakLineWidth.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/util/jar/Manifest/LineBreakLineWidth.java Fri Dec 08 23:39:08 2017 +0100
@@ -0,0 +1,280 @@
+/*
+ * 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.
+ */
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.util.jar.Manifest;
+import java.util.jar.Attributes;
+import java.util.jar.Attributes.Name;
+
+import org.testng.annotations.Test;
+import static org.testng.Assert.*;
+
+/**
+ * @test
+ * @bug 6372077
+ * @run testng LineBreakLineWidth
+ * @summary write valid manifests with respect to line breaks
+ * and read any line width
+ */
+public class LineBreakLineWidth {
+
+ /**
+ * maximum header name length from {@link Name#isValid(String)}
+ */
+ final static int MAX_HEADER_NAME_LENGTH = 70;
+
+ /**
+ * range of one..{@link #TEST_WIDTH_RANGE} covered in this test that
+ * exceeds the range of allowed header name lengths or line widths
+ * in order to also cover invalid cases beyond the valid boundaries
+ * and to keep it somewhat independent from the actual manifest width.
+ * <p>
+ * bigger than 72 (maximum manifest header line with in bytes (not utf-8
+ * encoded characters) but otherwise arbitrarily chosen
+ */
+ final static int TEST_WIDTH_RANGE = 123;
+
+ /**
+ * tests if only valid manifest files can written depending on the header
+ * name length or that an exception occurs already on the attempt to write
+ * an invalid one otherwise and that no invalid manifest can be written.
+ * <p>
+ * due to bug JDK-6372077 it was possible to write a manifest that could
+ * not be read again. independent of the actual manifest line width, such
+ * a situation should never happen, which is the subject of this test.
+ */
+ @Test
+ public void testWriteValidManifestOrException() throws IOException {
+ /*
+ * multi-byte utf-8 characters cannot occur in header names,
+ * only in values which are not subject of this test here.
+ * hence, each character in a header name uses exactly one byte and
+ * variable length utf-8 character encoding doesn't affect this test.
+ */
+
+ String name = "";
+ for (int l = 1; l <= TEST_WIDTH_RANGE; l++) {
+ name += "x";
+ System.out.println("name = " + name + ", "
+ + "name.length = " + name.length());
+
+ if (l <= MAX_HEADER_NAME_LENGTH) {
+ writeValidManifest(name, "somevalue");
+ } else {
+ writeInvalidManifestThrowsException(name, "somevalue");
+ }
+ }
+ }
+
+ static void writeValidManifest(String name, String value)
+ throws IOException {
+ byte[] mfBytes = writeManifest(name, value);
+ Manifest mf = new Manifest(new ByteArrayInputStream(mfBytes));
+ assertMainAndSectionValues(mf, name, value);
+ }
+
+ static void writeInvalidManifestThrowsException(String name, String value)
+ throws IOException {
+ try {
+ writeManifest(name, value);
+ } catch (IllegalArgumentException e) {
+ // no invalid manifest was produced which is considered acceptable
+ return;
+ }
+
+ fail("no error writing manifest considered invalid");
+ }
+
+ /**
+ * tests that manifest files can be read even if the line breaks are
+ * placed in different positions than where the current JDK's
+ * {@link Manifest#write(java.io.OutputStream)} would have put it provided
+ * the manifest is valid otherwise.
+ * <p>
+ * the <a href="{@docRoot}/../specs/jar/jar.html#Notes_on_Manifest_and_Signature_Files">
+ * "Notes on Manifest and Signature Files" in the "JAR File
+ * Specification"</a> state that "no line may be longer than 72 bytes
+ * (not characters), in its utf8-encoded form." but allows for earlier or
+ * additional line breaks.
+ * <p>
+ * the most important purpose of this test case is probably to make sure
+ * that manifest files broken at 70 bytes line width written with the
+ * previous version of {@link Manifest} before this fix still work well.
+ */
+ @Test
+ public void testReadDifferentLineWidths() throws IOException {
+ /*
+ * uses only one-byte utf-8 encoded characters as values.
+ * correctly breaking multi-byte utf-8 encoded characters
+ * would be subject of another test if there was one such.
+ */
+
+ // line width
+ // 6 minimum required for section names starting with "Name: "
+ for (int w = 6; w <= TEST_WIDTH_RANGE; w++) {
+
+ // header name length
+ String name = "";
+ // - 2 due to the delimiter ": " that has to fit on the same
+ // line as the name
+ for (int ln = 1; ln <= w - 2; ln++) {
+ name += "x";
+
+ // value length
+ String value = "";
+ for (int lv = 1; lv <= TEST_WIDTH_RANGE; lv++) {
+ value += "y";
+ }
+
+ System.out.println("lineWidth = " + w);
+ System.out.println("name = " + name + ""
+ + ", name.length = " + name.length());
+ System.out.println("value = " + value + ""
+ + ", value.length = " + value.length());
+
+ readSpecificLineWidthManifest(name, value, w);
+ }
+ }
+ }
+
+ static void readSpecificLineWidthManifest(String name, String value,
+ int lineWidth) throws IOException {
+ /*
+ * breaking header names is not allowed and hence cannot be reasonably
+ * tested. it cannot easily be helped, that invalid manifest files
+ * written by the previous Manifest version implementation are illegal
+ * if the header name is 69 or 70 bytes and in that case the name/value
+ * delimiter ": " was broken on a new line.
+ *
+ * changing the line width in Manifest#make72Safe(StringBuffer),
+ * however, also affects at which positions values are broken across
+ * lines (should always have affected values only and never header
+ * names or the delimiter) which is tested here.
+ *
+ * ideally, any previous Manifest implementation would have been used
+ * here to provide manifest files to test reading but these are no
+ * longer available in this version's sources and there might as well
+ * be other libraries writing manifests. Therefore, in order to be able
+ * to test any manifest file considered valid with respect to line
+ * breaks that could not possibly be produced with the current Manifest
+ * implementation, this test provides its own manifests in serialized
+ * form.
+ */
+ String lineBrokenSectionName = breakLines(lineWidth, "Name: " + name);
+ String lineBrokenNameAndValue = breakLines(lineWidth, name + ": " + value);
+
+ ByteArrayOutputStream mfBuf = new ByteArrayOutputStream();
+ mfBuf.write("Manifest-Version: 1.0".getBytes(UTF_8));
+ mfBuf.write("\r\n".getBytes(UTF_8));
+ mfBuf.write(lineBrokenNameAndValue.getBytes(UTF_8));
+ mfBuf.write("\r\n".getBytes(UTF_8));
+ mfBuf.write("\r\n".getBytes(UTF_8));
+ mfBuf.write(lineBrokenSectionName.getBytes(UTF_8));
+ mfBuf.write("\r\n".getBytes(UTF_8));
+ mfBuf.write(lineBrokenNameAndValue.getBytes(UTF_8));
+ mfBuf.write("\r\n".getBytes(UTF_8));
+ mfBuf.write("\r\n".getBytes(UTF_8));
+ byte[] mfBytes = mfBuf.toByteArray();
+ printManifest(mfBytes);
+
+ boolean nameValid = name.length() <= MAX_HEADER_NAME_LENGTH;
+ System.out.println("nameValid = " + nameValid); // FIXME
+
+ Manifest mf;
+ try {
+ mf = new Manifest(new ByteArrayInputStream(mfBytes));
+ } catch (IOException e) {
+ if (!nameValid &&
+ e.getMessage().startsWith("invalid header field")) {
+ // expected because the name is not valid
+ return;
+ }
+
+ throw new AssertionError(e.getMessage(), e);
+ }
+
+ assertTrue(nameValid, "failed to detect invalid manifest");
+
+ assertMainAndSectionValues(mf, name, value);
+ }
+
+ static String breakLines(int lineWidth, String nameAndValue) {
+ String lineBrokenNameAndValue = "";
+ for (int i = 0; i < nameAndValue.length(); i++) {
+ lineBrokenNameAndValue += nameAndValue.substring(i, i + 1);
+ if (0 < i && i < nameAndValue.length() - 1 && (i % lineWidth) == 0) {
+ lineBrokenNameAndValue += "\r\n ";
+ }
+ }
+ return lineBrokenNameAndValue;
+ }
+
+ static byte[] writeManifest(String name, String value) throws IOException {
+ /*
+ * writing manifest main headers is implemented separately from
+ * writing named sections manifest headers:
+ * - java.util.jar.Attributes.writeMain(DataOutputStream)
+ * - java.util.jar.Attributes.write(DataOutputStream)
+ * which is why this is also covered separately in this test by
+ * always adding the same value twice, in the main attributes as
+ * well as in a named section (using the header name also as the
+ * section name).
+ */
+
+ Manifest mf = new Manifest();
+ mf.getMainAttributes().put(Name.MANIFEST_VERSION, "1.0");
+ mf.getMainAttributes().putValue(name, value);
+
+ Attributes section = new Attributes();
+ section.putValue(name, value);
+ mf.getEntries().put(name, section);
+
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
+ mf.write(out);
+ byte[] mfBytes = out.toByteArray();
+ printManifest(mfBytes);
+ return mfBytes;
+ }
+
+ private static void printManifest(byte[] mfBytes) {
+ final String sepLine = "----------------------------------------------"
+ + "---------------------|-|-|"; // |-positions: ---68-70-72
+ System.out.println(sepLine);
+ System.out.print(new String(mfBytes, UTF_8));
+ System.out.println(sepLine);
+ }
+
+ private static void assertMainAndSectionValues(Manifest mf, String name,
+ String value) {
+ String mainValue = mf.getMainAttributes().getValue(name);
+ String sectionValue = mf.getAttributes(name).getValue(name);
+
+ assertEquals(value, mainValue, "value different in main section");
+ assertEquals(value, sectionValue, "value different in named section");
+ }
+
+}
diff -r be620a591379 test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java
--- a/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java Mon Oct 30 21:23:10 2017 +0100
+++ b/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java Fri Dec 08 23:39:08 2017 +0100
@@ -58,7 +58,7 @@
* @see #verifyClassNameLineBroken(JarFile, String)
*/
static final String testClassName =
- "LineBrokenMultiByteCharacterA1234567890B1234567890C123456789D12\u00E9xyz.class";
+ "LineBrokenMultiByteCharacterA1234567890B1234567890C123456789D1234\u00E9xyz.class";
static final String anotherName =
"LineBrokenMultiByteCharacterA1234567890B1234567890C123456789D1234567890.class";
