Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
> Please review this PR which addresses the handling of invalid UTF-8 byte > sequences in the entry name of a LOC file header and a Zip file comment which > is returned via ZipFile::getComment. > > As part of the change, `ZipFile::getComment` will now return `null` if an > invalid UTF-8 byte sequence is encountered while converting the byte array to > a String. The CSR for this change has also been approved. > > Mach5 tiers 1-3 are clean with this change. Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Updates based on 1st round of feedback - Changes: - all: https://git.openjdk.org/jdk/pull/17995/files - new: https://git.openjdk.org/jdk/pull/17995/files/e76ff749..dfd49101 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17995&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17995&range=00-01 Stats: 24 lines in 3 files changed: 1 ins; 2 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/17995.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17995/head:pull/17995 PR: https://git.openjdk.org/jdk/pull/17995
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sat, 24 Feb 2024 18:51:22 GMT, Lance Andersen wrote: >> Please review this PR which addresses the handling of invalid UTF-8 byte >> sequences in the entry name of a LOC file header and a Zip file comment >> which is returned via ZipFile::getComment. >> >> As part of the change, `ZipFile::getComment` will now return `null` if an >> invalid UTF-8 byte sequence is encountered while converting the byte array >> to a String. The CSR for this change has also been approved. >> >> Mach5 tiers 1-3 are clean with this change. > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Updates based on 1st round of feedback Thank you for your input Eirik, I have pushed updates to address the suggestions you raised - PR Review: https://git.openjdk.org/jdk/pull/17995#pullrequestreview-1899544786
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sat, 24 Feb 2024 16:57:50 GMT, Eirik Bjørsnøs wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updates based on 1st round of feedback > > src/java.base/share/classes/java/util/zip/ZipFile.java line 331: > >> 329: try { >> 330: return res.zsrc.zc.toString(res.zsrc.comment); >> 331: } catch(IllegalArgumentException iae) { > > Suggestion: > > } catch (IllegalArgumentException iae) { fixed > src/java.base/share/classes/java/util/zip/ZipInputStream.java line 526: > >> 524: throw (ZipException) new ZipException( >> 525: "invalid LOC header (bad entry >> name)").initCause(ex); >> 526: } > > There is a lot going on here: > > * The creation of a ZipEntry > * The interpretation of the general purpose bit flag > * The split to call either a static or non-static toString depending on the > flag interpretation > * The exception handling, including the somewhat unusual initCause and cast > > The comment at 517 seems even more detached from the logic it tries to > describe after this change. > > Perhaps we could benefit from introducing a `zipCoderFromFlag(int flag)` > method, similar to that in ZipFile? > > This would allow the section to be rewritten to something that hide the flag > parsing details, leaves us with a single (polymorphic) toString invocation > and extracts `createZipEntry` from the try/catch scope. > > Something like this: (I also changed the exception handling here, that's a > bit orthogonal to the main points above) > > Suggestion: > > String name; > try { > name = zipCoderForFlag(flag).toString(b, len); > } catch (IllegalArgumentException ex) { > ZipException e = new ZipException("invalid LOC header (bad entry > name)"); > e.initCause(ex); > throw e; > } > ZipEntry e = createZipEntry(name); I believe you meant `ZipCoderFromPos(int flag)`. I don't think it is needed in ZipInputStream but I did move the call to createZipEntry out of the try block per your suggestion > test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 49: > >> 47: * @summary Validate that a ZipException is thrown when opening a ZIP >> file via >> 48: * ZipFile, traversing a Zip File via ZipInputStream, with invalid UTF-8 >> 49: * byte sequences in the name or comment fields fails with ZipException. > > The leading summary sentence needs a cleanup. Here's one suggestion: > > Suggestion: > > * @summary Validate that a ZipException is thrown when a ZIP file with > * invalid UTF-8 byte sequences in the name or comment fields is opened via > * ZipFile or traversed via ZipInputStream. Updated > test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 60: > >> 58: private static final byte[] INVALID_UTF8_BYTE_SEQUENCE = {(byte) >> 0xF0, (byte) 0xA4, (byte) 0xAD}; >> 59: // Expected error message when an invalid entry name or entry >> comment is >> 60: // encountered when accessing a CEN Header > > This two-line comment could benefit from an earlier line break. The break makes sense as is in IntelliJ, so please clarify > test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 64: > >> 62: >> 63: // Expected error message when an invalid entry name is encountered >> when >> 64: // accessing a LOC Header > > This two-line comment could benefit from an earlier line break. Same comment as above > test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 65: > >> 63: // Expected error message when an invalid entry name is encountered >> when >> 64: // accessing a LOC Header >> 65: private static final String LOC_BAD_ENTRY_NAME_OR_COMMENT = "invalid >> LOC header (bad entry name)"; > > Should this be renamed `LOC_BAD_ENTRY_NAME`? Yep, I thought I made that change but must have just thought about it and never went back to it > test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 128: > >> 126: * 0x93: Comment : [ZipFileComment] >> 127: */ >> 128: public static byte[] VALID_ZIP = { > > I see this byte array encoding of ZIP files is a pattern used across tests. > My preference would be to encode this in Base64 or hex, since that would make > the consumption by external command line tools easier and the encoded > representation somewhat prettier. But no big deal, this might just come down > to personal preference. (This isn't human readable anyhow for the normal > definition of "human" :) Yes, this is a style preference and plan to leave as is. > test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 182: > >> 180: /** >> 181: * Validate that the original Zip file can be opened via ZipFile. >> 182: * @throws IOException if an entry occurs >
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sat, 24 Feb 2024 18:54:07 GMT, Lance Andersen wrote: >> Please review this PR which addresses the handling of invalid UTF-8 byte >> sequences in the entry name of a LOC file header and a Zip file comment >> which is returned via ZipFile::getComment. >> >> As part of the change, `ZipFile::getComment` will now return `null` if an >> invalid UTF-8 byte sequence is encountered while converting the byte array >> to a String. The CSR for this change has also been approved. >> >> Mach5 tiers 1-3 are clean with this change. > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Updates based on 1st round of feedback Hello Lance, the changes look fine to me. `ZipFile.java` file will need a copyright year update. test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 189: > 187: try (ZipFile zf = new ZipFile(ZIP_FILE.toFile())) { > 188: var comment = zf.getComment(); > 189: System.out.printf("Comment= %s%n", comment); Should we assert for the valid expected `comment` here? - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17995#pullrequestreview-1899665517 PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501808997
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sat, 24 Feb 2024 18:54:07 GMT, Lance Andersen wrote: >> Please review this PR which addresses the handling of invalid UTF-8 byte >> sequences in the entry name of a LOC file header and a Zip file comment >> which is returned via ZipFile::getComment. >> >> As part of the change, `ZipFile::getComment` will now return `null` if an >> invalid UTF-8 byte sequence is encountered while converting the byte array >> to a String. The CSR for this change has also been approved. >> >> Mach5 tiers 1-3 are clean with this change. > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Updates based on 1st round of feedback src/java.base/share/classes/java/util/zip/ZipInputStream.java line 524: > 522: : zc.toString(b, len); > 523: } catch(Exception ex) { > 524: throw (ZipException) new ZipException( Whitespace nit: Suggestion: } catch (Exception ex) { throw (ZipException) new ZipException( - PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501809393
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sun, 25 Feb 2024 12:16:14 GMT, Jaikiran Pai wrote: > Hello Lance, the changes look fine to me. `ZipFile.java` file will need a > copyright year update. Thank you Jai, updated > test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 189: > >> 187: try (ZipFile zf = new ZipFile(ZIP_FILE.toFile())) { >> 188: var comment = zf.getComment(); >> 189: System.out.printf("Comment= %s%n", comment); > > Should we assert for the valid expected `comment` here? Sure I can add the validation - PR Comment: https://git.openjdk.org/jdk/pull/17995#issuecomment-1962954345 PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501822988
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sat, 24 Feb 2024 18:54:07 GMT, Lance Andersen wrote: >> Please review this PR which addresses the handling of invalid UTF-8 byte >> sequences in the entry name of a LOC file header and a Zip file comment >> which is returned via ZipFile::getComment. >> >> As part of the change, `ZipFile::getComment` will now return `null` if an >> invalid UTF-8 byte sequence is encountered while converting the byte array >> to a String. The CSR for this change has also been approved. >> >> Mach5 tiers 1-3 are clean with this change. > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Updates based on 1st round of feedback Thanks for the feedback. Updated accordingly - PR Review: https://git.openjdk.org/jdk/pull/17995#pullrequestreview-1899679545
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sun, 25 Feb 2024 12:20:36 GMT, Eirik Bjørsnøs wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updates based on 1st round of feedback > > src/java.base/share/classes/java/util/zip/ZipInputStream.java line 524: > >> 522: : zc.toString(b, len); >> 523: } catch(Exception ex) { >> 524: throw (ZipException) new ZipException( > > Whitespace nit: > > Suggestion: > > } catch (Exception ex) { > throw (ZipException) new ZipException( fixed - PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501822894