JDK-6372077: JarFile.getManifest() should handle manifest attribute name 70 bytes

Thu Nov 2 23:04:44 UTC 2017

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

On 12.10.2017 22:32, Sean Mullan wrote:
> Hi Phillip,
>
> All of these bugs are in the core-libs area, so I am copying the 
> core-libs-dev list since that is where they should be discussed and 
> reviewed. I have -bcc-ed security-dev (where this was originally posted).
>
> --Sean
>
> On 10/2/17 1:24 PM, 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. 
>> 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
>> philipp.kunz at paratix.ch <mailto:philipp.kunz at paratix.ch>

-- 

Gruss Philipp

------------------------------------------------------------------------

Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
philipp.kunz at paratix.ch <mailto:philipp.kunz at paratix.ch>