JDK-6372077: JarFile.getManifest() should handle manifest attribute names up to 70 bytes

Mon Dec 18 15:46:03 UTC 2017

Hi Phillip,

Sorry for the silence...

I/we haven't had time to full understand the ramifications of the change 
you propose.
It seems there is a difficult/unresolvable conflict in the 
specifications between the line length
requirements and the header specs.

Regards, Roger

On 11/21/2017 1:18 AM, 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
>>
>>
>> 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>
>