On Sun, 14 Nov 2021 15:12:16 GMT, Alan Bateman <al...@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
