Re: Manifest Related Bugs JDK-6372077, JDK-6202130, JDK-8176843, JDK-4842483, JDK-6443578, JDK-6910466, JDK-4935610, and JDK-4271239
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/shar... http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar... http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
Furthermore, Attributes.map should declare appropriate type parameters: http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar... 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-S...) 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/shar... 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/shar... 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@paratix.ch <mailto:philipp.kunz@paratix.ch>
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... 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...), 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/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
Furthermore, Attributes.map should declare appropriate type parameters: http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
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-S...) 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/shar...
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/shar...
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@paratix.ch <mailto:philipp.kunz@paratix.ch>
-- Gruss Philipp ------------------------------------------------------------------------ Paratix GmbH St Peterhofstatt 11 8001 Zürich +41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
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... 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...), 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/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
Furthermore, Attributes.map should declare appropriate type parameters: http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
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-S...) 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/shar...
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/shar...
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@paratix.ch <mailto:philipp.kunz@paratix.ch>
--
Gruss Philipp
------------------------------------------------------------------------
Paratix GmbH St Peterhofstatt 11 8001 Zürich
+41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
-- Gruss Philipp ------------------------------------------------------------------------ Paratix GmbH St Peterhofstatt 11 8001 Zürich +41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
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... 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...), 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
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... 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...), 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/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
Furthermore, Attributes.map should declare appropriate type parameters: http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
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-S...) 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/shar...
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/shar...
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@paratix.ch <mailto:philipp.kunz@paratix.ch>
--
Gruss Philipp
------------------------------------------------------------------------
Paratix GmbH St Peterhofstatt 11 8001 Zürich
+41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
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... / https://docs.oracle.com/javase/9/docs/specs/jar/jar.html#Notes_on_Manifest_a... [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050500.ht... [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... 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...), 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/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
Furthermore, Attributes.map should declare appropriate type parameters: http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
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-S...) 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/shar...
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/shar...
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@paratix.ch <mailto:philipp.kunz@paratix.ch>
--
Gruss Philipp
------------------------------------------------------------------------
Paratix GmbH St Peterhofstatt 11 8001 Zürich
+41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
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... / https://docs.oracle.com/javase/9/docs/specs/jar/jar.html#Notes_on_Manifest_a... [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050500.ht... [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... 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...), 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/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
Furthermore, Attributes.map should declare appropriate type parameters: http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar...
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-S...) 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/shar...
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/shar...
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@paratix.ch <mailto:philipp.kunz@paratix.ch>
--
Gruss Philipp
------------------------------------------------------------------------
Paratix GmbH St Peterhofstatt 11 8001 Zürich
+41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
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... / https://docs.oracle.com/javase/9/docs/specs/jar/jar.html#Notes_on_Manifest_a... [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050500.ht... [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... 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...), 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/shar... > > http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar... > > http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar... > > > Furthermore, Attributes.map should declare appropriate type > parameters: > http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar... > > 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-S...) > 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/shar... > > 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/shar... > > 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@paratix.ch <mailto:philipp.kunz@paratix.ch>
--
Gruss Philipp
------------------------------------------------------------------------
Paratix GmbH St Peterhofstatt 11 8001 Zürich
+41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
-- Gruss Philipp ------------------------------------------------------------------------ Paratix GmbH St Peterhofstatt 11 8001 Zürich +41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
Please review this contribution from Philipp Kunz to handle manifest attribute names up to 70 bytes. The change passes the available regression tests. Manifest handling is somewhat sensitive so an additional review is appreciated. Webrev: (rebased from the original patch of 2/22/18) http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/ Issue: https://bugs.openjdk.java.net/browse/JDK-6372077 Thanks, Roger On 2/2/2018 1:52 PM, Philipp Kunz wrote:
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... / https://docs.oracle.com/javase/9/docs/specs/jar/jar.html#Notes_on_Manifest_a... [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050500.ht... [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... 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...), 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/shar... >> >> http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar... >> >> http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar... >> >> >> Furthermore, Attributes.map should declare appropriate type >> parameters: >> http://hg.openjdk.java.net/jdk10/master/file/a0116bcc65b7/src/java.base/shar... >> >> 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-S...) >> 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/shar... >> >> 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/shar... >> >> 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@paratix.ch <mailto:philipp.kunz@paratix.ch>
--
Gruss Philipp
------------------------------------------------------------------------
Paratix GmbH St Peterhofstatt 11 8001 Zürich
+41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
--
Gruss Philipp
------------------------------------------------------------------------
Paratix GmbH St Peterhofstatt 11 8001 Zürich
+41 (0)76 397 79 35 philipp.kunz@paratix.ch <mailto:philipp.kunz@paratix.ch>
On 23/02/2018 19:58, Roger Riggs wrote:
Please review this contribution from Philipp Kunz to handle manifest attribute names up to 70 bytes.
The change passes the available regression tests. Manifest handling is somewhat sensitive so an additional review is appreciated.
Webrev: (rebased from the original patch of 2/22/18) http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/
Issue: https://bugs.openjdk.java.net/browse/JDK-6372077 I looked at the changes to the manifest code and I think they are okay. I don't have time to study the tests in detail.
I think it would be useful to copy the clarification note "(there must be a colon and a SPACE after the name)" from the JAR file spec to the equivalent sentence in the Attributes class description. That will help anyone wondering why the limit is 70 rather than 72. This change shouldn't need a CSR as it's not adding any new assertions. -Alan
Hi Alan, Thanks for the review. I added: "There must be a colon and a SPACE after the name; the combined length will not exceed 72 characters." Webrev updated in place: http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/ Thanks, Roger On 2/26/2018 6:50 AM, Alan Bateman wrote:
On 23/02/2018 19:58, Roger Riggs wrote:
Please review this contribution from Philipp Kunz to handle manifest attribute names up to 70 bytes.
The change passes the available regression tests. Manifest handling is somewhat sensitive so an additional review is appreciated.
Webrev: (rebased from the original patch of 2/22/18) http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/
Issue: https://bugs.openjdk.java.net/browse/JDK-6372077 I looked at the changes to the manifest code and I think they are okay. I don't have time to study the tests in detail.
I think it would be useful to copy the clarification note "(there must be a colon and a SPACE after the name)" from the JAR file spec to the equivalent sentence in the Attributes class description. That will help anyone wondering why the limit is 70 rather than 72. This change shouldn't need a CSR as it's not adding any new assertions.
-Alan
On 01/03/2018 19:59, Roger Riggs wrote:
Hi Alan,
Thanks for the review. I added:
"There must be a colon and a SPACE after the name; the combined length will not exceed 72 characters."
Webrev updated in place: http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/ Thanks, looks okay to me now.
-Alan
+1 On 03/01/2018 11:59 AM, Roger Riggs wrote:
Hi Alan,
Thanks for the review. I added:
"There must be a colon and a SPACE after the name; the combined length will not exceed 72 characters."
Webrev updated in place: http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/
Thanks, Roger
On 2/26/2018 6:50 AM, Alan Bateman wrote:
On 23/02/2018 19:58, Roger Riggs wrote:
Please review this contribution from Philipp Kunz to handle manifest attribute names up to 70 bytes.
The change passes the available regression tests. Manifest handling is somewhat sensitive so an additional review is appreciated.
Webrev: (rebased from the original patch of 2/22/18) http://cr.openjdk.java.net/~rriggs/webrev-jar-6372077.patch/
Issue: https://bugs.openjdk.java.net/browse/JDK-6372077 I looked at the changes to the manifest code and I think they are okay. I don't have time to study the tests in detail.
I think it would be useful to copy the clarification note "(there must be a colon and a SPACE after the name)" from the JAR file spec to the equivalent sentence in the Attributes class description. That will help anyone wondering why the limit is 70 rather than 72. This change shouldn't need a CSR as it's not adding any new assertions.
-Alan
participants (5)
-
Alan Bateman
-
Philipp Kunz
-
Roger Riggs
-
Sean Mullan
-
Xueming Shen