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

Fri Feb 2 18:52:55 UTC 2018

Hi Roger

Glad to send the patch.
I also tried to write a meaningful and useful test. Please tell me 
ruthlessly if it makes sense or what not.
Looking forward to progress in a bug that has been open for more than 10 
years now.

Philipp

On 22.01.2018 21:03, Roger Riggs wrote:
> Hi Philipp,
>
> I'm tending to agree with the suggestion about line length interpretation.
> To meet OpenJDK IP requirements, please attach the .patch file or 
> include it in the body
> of the message.
>
> Thanks, Roger
>
> On 12/18/2017 11:17 PM, Philipp Kunz wrote:
>> Hi Roger,
>>
>> My suggested and also preferred approach is to read the manifest 
>> specification [1] in a way such that the line breaks are not included 
>> when counting the maximum line length. The specification does not 
>> state explicitly whether or not to include line breaks within the 
>> maximum line length limit but the following sentence from the 
>> specifications gives a hint:
>>
>>     Because header names cannot be continued, the maximum length of a
>>     header name is 70 bytes (there must be a colon and a SPACE after the
>>     name).
>>
>> Above statement can be true only if line breaks are not counted for 
>> the maximum line length limit. Assuming so would in my opinion allow 
>> to understand the complete manifest specification without a conflict 
>> and effectively result in wider manifest files (maximum each line), 
>> wider by two bytes of a line break.
>>
>> In the meantime since the mail you replied to, I created a patch [3] 
>> mentioned in [2] which might be useful provided the manifest 
>> specification discussion is resolved.
>>
>> Regards,
>> Philipp
>>
>>
>> [1] 
>> https://docs.oracle.com/javase/8/docs/technotes/guides/jar/jar.html#Notes_on_Manifest_and_Signature_Files 
>> / 
>> https://docs.oracle.com/javase/9/docs/specs/jar/jar.html#Notes_on_Manifest_and_Signature_Files
>> [2] 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050500.html
>> [3] http://files.paratix.ch/jdk/6372077/webrev.01/
>>
>>
>>
>> On 18.12.2017 16:46, Roger Riggs wrote:
>>> 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>
>>>>
>>>
>>
>

-- 

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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 6372077.patch
Type: text/x-patch
Size: 16060 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20180202/769203b3/6372077-0001.patch>