RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND

[Lance Andersen]

On Sun, 14 Nov 2021 15:12:16 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> The ZipOutputStream class may create bogus zip data which cannot be opened by the ZipFile. The root cause is how the comment field is stored by the ZipOutputStream. According to the zip specification, the comment field should not be longer than 0xFFFF bytes, and we try to validate the length of the comment, but unfortunately, we do this after the comment was assigned already. So if the application saves the comment based on the user's input and then gets an exception from the ZipOutputStream.setComment() it may assume that the comment is too long and it will be ignored, but it will be saved as-is to the file.
>> 
>> Please take a look at [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117) refactoring, and note:
>>  * The comment field is assigned before the length check
>>  * The null comment is ignored
>> 
>> The current fix will move the length validation before being assigned and will use the null comment as an empty text. Note that the behavior of the null parameter is not specified in the method/class/package so we are free here to implement it in any way, any thoughts/suggestions on which is better?
>
> src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 154:
> 
>> 152:             }
>> 153:         }
>> 154:         this.comment = bytes;
> 
> The implementation change looks okay. I assume this regression slipped through due to lack of tests.
> 
> The method description doesn't make it clear that the comment can be null (ZipEntry.setComment has the same issue) so we should fix this while we are in the area, as a separate JBS of course as it will need a CSR to track the spec clarification.

ZipEntry::setComment indicates that the comment will be truncated if needed and ZipOutputStream takes care of this.

Perhaps writeEND() should also be updated to  something like:
`writeBytes(comment, 0, Math.min(comment.length, 0xffff))`

Which is similar to what happens in writeCEN

Yes it would be nice to clarify that a null is accepted by setComment.  When null is specified, the comment length is written as 0

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

PR: https://git.openjdk.java.net/jdk/pull/6380