RFR: JDK-8056934: ZipInputStream does not correctly handle local header data descriptors with the optional signature missing
Hi Xueming and Alan, I'd like you to do a code review. https://bugs.openjdk.java.net/browse/JDK-8056934 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/zip-DataDescriptorSignat... This seems like an atypical off-by-one, so I'm not sure how it happened, and I have the nagging feeling I'm missing something. Greg, this java code review contains a python program!
On 29/08/2014 20:12, Martin Buchholz wrote:
Hi Xueming and Alan,
I'd like you to do a code review.
https://bugs.openjdk.java.net/browse/JDK-8056934 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/zip-DataDescriptorSignat... <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk9/zip-DataDescriptorSignatureMissing/>
This seems like an atypical off-by-one, so I'm not sure how it happened, and I have the nagging feeling I'm missing something.
Greg, this java code review contains a python program! I looked at 4.3.9.3. For ZIP64 then we'll read 24 bytes which is 4 bytes too much if the signature is not present. The offset of those 4 bytes is position 20 (ZIP64_EXTHDR - ZIP64_EXTHDR with the current constants). With ZIP then we'll need 16 bytes and need to unread 4 bytes from position 12 (EXTHDR - EXTCRC with the current constants).
So I think you're right that the -1, I can only assume that we haven't encountered zip files that have a data descriptor without the signature. For the comment in ZipInputStream then it might be better to move that to readEnd. My preference would be to not include the part starting "As of 2014-08, phyton ..." as that might not interesting in years to come. -Alan.
On 9/1/14 9:05 AM, Alan Bateman wrote:
On 29/08/2014 20:12, Martin Buchholz wrote:
Hi Xueming and Alan,
I'd like you to do a code review.
https://bugs.openjdk.java.net/browse/JDK-8056934 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/zip-DataDescriptorSignat... <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk9/zip-DataDescriptorSignatureMissing/>
This seems like an atypical off-by-one, so I'm not sure how it happened, and I have the nagging feeling I'm missing something.
Greg, this java code review contains a python program! I looked at 4.3.9.3. For ZIP64 then we'll read 24 bytes which is 4 bytes too much if the signature is not present. The offset of those 4 bytes is position 20 (ZIP64_EXTHDR - ZIP64_EXTHDR with the current constants). With ZIP then we'll need 16 bytes and need to unread 4 bytes from position 12 (EXTHDR - EXTCRC with the current constants).
So I think you're right that the -1, I can only assume that we haven't encountered zip files that have a data descriptor without the signature.
The zip64 part was the copy/paste of the existing code, and we did not add any test for this corner case we implemented zip64. Before the fix of 4635869, the implementation simply throws an exception. With the "fix" it appears it can work well with the first entry of such a zip file, but skip the rest...my guess is that the test case zip file in original bug report happens to be a single entry zip file. -Sherman
For the comment in ZipInputStream then it might be better to move that to readEnd. My preference would be to not include the part starting "As of 2014-08, phyton ..." as that might not interesting in years to come.
-Alan.
I tried and failed to expand the test to do ZIP64 as well - I ran into 32-bit limitations in python. The test is useful, but too brittle to be a permanent jtreg test, so I @ignored it. I'm ready to submit this.
For the comment in ZipInputStream then it might be better to move that to readEnd. My preference would be to not include the part starting "As of 2014-08, phyton ..." as that might not interesting in years to come.
I moved the comment into readEnd's javadoc. I think laying out the compatibility landscape is sufficiently useful that the comment about python's implementation should stay, especially given that python has no plans to "fix" theirs.
On 11/09/2014 01:21, Martin Buchholz wrote:
For the comment in ZipInputStream then it might be better to move that to readEnd. My preference would be to not include the part starting "As of 2014-08, phyton ..." as that might not interesting in years to come.
I moved the comment into readEnd's javadoc. I think laying out the compatibility landscape is sufficiently useful that the comment about python's implementation should stay, especially given that python has no plans to "fix" theirs.
Having the first 3 paragraphs in readEnd should be good (thanks), I would think that the notes on python and the link to its issue tracker might be better put into the JIRA issue for anyone that wants to dig into more of the history. -Alan.
On Thu, Sep 11, 2014 at 12:34 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 11/09/2014 01:21, Martin Buchholz wrote:
For the comment in ZipInputStream then it might be better to move that to readEnd. My preference would be to not include the part starting "As of 2014-08, phyton ..." as that might not interesting in years to come.
I moved the comment into readEnd's javadoc. I think laying out the compatibility landscape is sufficiently useful that the comment about python's implementation should stay, especially given that python has no plans to "fix" theirs.
Having the first 3 paragraphs in readEnd should be good (thanks), I would think that the notes on python and the link to its issue tracker might be better put into the JIRA issue for anyone that wants to dig into more of the history.
As you wish!
On 8/29/14 12:12 PM, Martin Buchholz wrote:
Hi Xueming and Alan,
I'd like you to do a code review.
https://bugs.openjdk.java.net/browse/JDK-8056934 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/zip-DataDescriptorSignat... <http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk9/zip-DataDescriptorSignatureMissing/>
This seems like an atypical off-by-one, so I'm not sure how it happened, and I have the nagging feeling I'm missing something.
Greg, this java code review contains a python program!
Agreed, it appears to be off-by-1. I don't know how it happened as well. The code was put in back to 1.4.2 (4635869) to fix exactly the same issue (a local descriptors without signature bits). Obviously we did not put into the regression test for it :-( The fix looks fine. Though the comment part is a little long:-) It might be desirable to simply keep the first part and move it into readEND? -Sherman
participants (3)
-
Alan Bateman
-
Martin Buchholz
-
Xueming Shen