Manifest Related Bugs JDK-6372077, JDK-6202130, JDK-8176843, JDK-4842483, JDK-6443578, JDK-6910466, JDK-4935610, and JDK-4271239

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Manifest Related Bugs JDK-6372077, JDK-6202130, JDK-8176843, JDK-4842483, JDK-6443578, JDK-6910466, JDK-4935610, and JDK-4271239

Philipp Kunz
Hi,

While fixing JDK-6695402 I came across other related bugs to manifests such as JDK-6372077, JDK-6202130, JDK-8176843, JDK-4842483, and JDK-6443578 which all relate to manifest reading and writing. Somewhere bug 7071055 is mentioned but I cannot find anywhere. Another group of bugs, JDK-6910466, JDK-4935610, and JDK-4271239 concern the mandatory manifest main attributes Manifest-Version or Signature-Version and at first glance are duplicates. If you know of more known bugs, not necessarily present in jira, I'd be glad to get notified.

There are also some comments about utf handling and line breaking in the code of Manifest:
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l299
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l327
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l370

Furthermore, Attributes.map should declare appropriate type parameters:
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l61
The specification would also require that `header names must not start with _, - or "From"` (http://docs.oracle.com/javase/7/docs/technotes/guides/jar/jar.html#Section-Specification) but I would opt not to add this as a hard restriction because I guess it can be assumed that such names are in use now after having been working for years. A warning to a logger might be conceivable such as in
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l424
Attribute values are never checked at all and invalid characters such as line breaks are never detected except that when reading the manifest again the values are cut off.
The tab character (U+0008) does not work in manifest values.
I suspect that there are also issues regarding the iteration order but I haven't got a prove yet unlike for the other points above:
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Manifest.java#l54
There is duplicated or very similar code in Attributes and Manifest: Attributes.write-Manifest.write-Attributes.writeMain and Attributes.read-Manifest.read.
Resolving JDK-6202130 would have the additional benefit to be able to view manifests with any utf-8 capable editor even if multi-byte characters break across lines.

Fixing these issues individually looks like more complicated work than fixing them all at once, I guess, also because of a very low current test coverage. So I'd suggest to add some thorough tests along with fixing these issues. But before I start I would like to get some guidance, assistance or at least confirmation on how to proceed. I'm new to open jdk and have submitted only one patch so far.

Is it ok to add tests for things that have worked before?
Is it ok to refactor duplicated code just for the added value to reduce effort for testing?
I assume compatibility to and from existing manifests is the highest priority, correct? This would also be the hard part in adding as complete test coverage as possible. What would be acceptable criteria to meet?
Why does Attributes not extend LinkedHashMap and why does Manifest not extend HashMap? Any objection?
While I would not want to write code that looks slow or change more than necessary one can never know before having performance actually measured. Is there some way this is dealt with or should I wait for such feedback until after patch submission?

Would someone sponsor?

Regards,
Philipp








Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Manifest Related Bugs JDK-6372077, JDK-6202130, JDK-8176843, JDK-4842483, JDK-6443578, JDK-6910466, JDK-4935610, and JDK-4271239

Magnus Ihse Bursie
On 2017-10-02 19:24, Philipp Kunz wrote:
Hi,

While fixing JDK-6695402 I came across other related bugs to manifests such as JDK-6372077, JDK-6202130, JDK-8176843, JDK-4842483, and JDK-6443578 which all relate to manifest reading and writing. Somewhere bug 7071055 is mentioned but I cannot find anywhere.

You cannot find JDK-7071055 since it's classified Confidential. I'm not sure why, but it has been closed as a duplicate of JDK-6695402, so it does not really matter.

I'm happy to see we've gotten such a thorough contributor! :-) Keep up the good work.

/Magnus


Another group of bugs, JDK-6910466, JDK-4935610, and JDK-4271239 concern the mandatory manifest main attributes Manifest-Version or Signature-Version and at first glance are duplicates. If you know of more known bugs, not necessarily present in jira, I'd be glad to get notified.

There are also some comments about utf handling and line breaking in the code of Manifest:
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l299
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l327
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l370

Furthermore, Attributes.map should declare appropriate type parameters:
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l61
The specification would also require that `header names must not start with _, - or "From"` (http://docs.oracle.com/javase/7/docs/technotes/guides/jar/jar.html#Section-Specification) but I would opt not to add this as a hard restriction because I guess it can be assumed that such names are in use now after having been working for years. A warning to a logger might be conceivable such as in
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Attributes.java#l424
Attribute values are never checked at all and invalid characters such as line breaks are never detected except that when reading the manifest again the values are cut off.
The tab character (U+0008) does not work in manifest values.
I suspect that there are also issues regarding the iteration order but I haven't got a prove yet unlike for the other points above:
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/share/classes/java/util/jar/Manifest.java#l54
There is duplicated or very similar code in Attributes and Manifest: Attributes.write-Manifest.write-Attributes.writeMain and Attributes.read-Manifest.read.
Resolving JDK-6202130 would have the additional benefit to be able to view manifests with any utf-8 capable editor even if multi-byte characters break across lines.

Fixing these issues individually looks like more complicated work than fixing them all at once, I guess, also because of a very low current test coverage. So I'd suggest to add some thorough tests along with fixing these issues. But before I start I would like to get some guidance, assistance or at least confirmation on how to proceed. I'm new to open jdk and have submitted only one patch so far.

Is it ok to add tests for things that have worked before?
Is it ok to refactor duplicated code just for the added value to reduce effort for testing?
I assume compatibility to and from existing manifests is the highest priority, correct? This would also be the hard part in adding as complete test coverage as possible. What would be acceptable criteria to meet?
Why does Attributes not extend LinkedHashMap and why does Manifest not extend HashMap? Any objection?
While I would not want to write code that looks slow or change more than necessary one can never know before having performance actually measured. Is there some way this is dealt with or should I wait for such feedback until after patch submission?

Would someone sponsor?

Regards,
Philipp








Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]