8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment #17995

LanceAndersen · 2024-02-24T14:56:01Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change requires CSR request JDK-8321927 to be approved
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment (Bug - P4)
JDK-8321927: Uncaught exceptions when parsing ZIP files with certain entry names or comments (CSR)

Reviewers

Jaikiran Pai (@jaikiran - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17995/head:pull/17995
$ git checkout pull/17995

Update a local copy of the PR:
$ git checkout pull/17995
$ git pull https://git.openjdk.org/jdk.git pull/17995/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17995

View PR using the GUI difftool:
$ git pr show -t 17995

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17995.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-24T14:56:49Z

👋 Welcome back lancea! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-02-24T14:57:51Z

@LanceAndersen The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-02-24T15:01:14Z

Webrevs

eirbjo

Left a few comment on the code changes and the test. Some may be a matter of preference.

test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java

src/java.base/share/classes/java/util/zip/ZipInputStream.java

src/java.base/share/classes/java/util/zip/ZipFile.java

eirbjo · 2024-02-24T17:15:02Z

Since the CSR is already approved, I'll add a question here:

ZipFile performs a lot of validation while opening ZIP files, including throwning ZipException for invalid entry names or comments. Why handle the ZIP file comment differently (lazily)? If this comment was also validated by the constructor, then the API change for ZipFile::getComment would not be needed.

Do we have reason to belive the encoding quality of ZIP file comments is less reliable than that of ZIP entry comments? Or is there some other reason this validation is done lazily?

LanceAndersen

Thank you for your input Eirik, I have pushed updates to address the suggestions you raised

src/java.base/share/classes/java/util/zip/ZipFile.java

test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java

src/java.base/share/classes/java/util/zip/ZipInputStream.java

LanceAndersen · 2024-02-24T19:09:59Z

Since the CSR is already approved, I'll add a question here:

ZipFile performs a lot of validation while opening ZIP files, including throwning ZipException for invalid entry names or comments. Why handle the ZIP file comment differently (lazily)? If this comment was also validated by the constructor, then the API change for ZipFile::getComment would not be needed.

Do we have reason to belive the encoding quality of ZIP file comments is less reliable than that of ZIP entry comments? Or is there some other reason this validation is done lazily?

Yes, there are some libraries/tools that are using the Zip file comment for its own purposes such as idea_rt.jar which is part of IntelliJ.

jaikiran

Hello Lance, the changes look fine to me. ZipFile.java file will need a copyright year update.

openjdk · 2024-02-25T12:17:10Z

@LanceAndersen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment

Reviewed-by: jpai, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

1799ffe: 8310351: Typo in ImmutableCollections
d10f277: 8326006: Allow TEST_VM_FLAGLESS to set flagless mode
63f6a56: 8326530: Widen allowable error bound of Math.tan

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java

eirbjo · 2024-02-25T12:20:36Z

src/java.base/share/classes/java/util/zip/ZipInputStream.java

+        } catch(Exception ex) {
+            throw  (ZipException) new ZipException(


Whitespace nit:

Suggested change

} catch(Exception ex) {

throw (ZipException) new ZipException(

} catch (Exception ex) {

throw (ZipException) new ZipException(

LanceAndersen

Thanks for the feedback. Updated accordingly

LanceAndersen · 2024-02-25T13:34:41Z

src/java.base/share/classes/java/util/zip/ZipInputStream.java

+        } catch(Exception ex) {
+            throw  (ZipException) new ZipException(


test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java

LanceAndersen · 2024-02-25T14:13:48Z

Hello Lance, the changes look fine to me. ZipFile.java file will need a copyright year update.

Thank you Jai, updated

jaikiran

Thank you for the updates, Lance. The changes look good to me.

AlanBateman · 2024-02-25T15:26:35Z

src/java.base/share/classes/java/util/zip/ZipFile.java

-     * Returns the zip file comment, or null if none.
+     * Returns the zip file comment. If a comment does not exist or an error is
+     * encountered decoding the comment using the charset specified
+     * when opening the Zip file, then {@code null} is returned.


I've previously discussed options with Lance around this issue and I agree with the proposal to specify that it returns null when the decoding fails.

(In passing, the casing of "zip file" is very inconsistent in the javadoc, it's "ZIP file" in some places, "zip file" in others, the change here uses "Zip file").

(In passing, the casing of "zip file" is very inconsistent in the javadoc, it's "ZIP file" in some places, "zip file" in others, the change here uses "Zip file").

Thank you for pointing out the discrepancy above.

Let's decide on which way to go and I will update under a separate PR.

I would suggest "ZIP file" based on the PKWare APPNOTE.TXT, but the man pages for info-zip command zip command uses "zip file"

Apache commons also uses "ZIP file" or "ZIP archive"

I would suggest "ZIP file" based on the PKWare APPNOTE.TXT,

I think using this case would be good.

I would suggest "ZIP file" based on the PKWare APPNOTE.TXT, but the man pages for info-zip command zip command uses "zip file"

"ZIP" is not an acronym so I assume this is why they use "zip" in their docs.

AlanBateman

Spec + implementation change look fine. I haven't spent time looking at the test.

LanceAndersen · 2024-02-26T15:07:31Z

/integrate

LanceAndersen · 2024-02-26T15:18:26Z

/integrate

openjdk · 2024-02-26T15:32:16Z

Going to push as commit 20c71ce.
Since your change was applied there have been 9 commits pushed to the master branch:

c042f08: 8326653: Remove jdk.internal.reflect.UTF8
bb6b048: 8326099: GZIPOutputStream should use Deflater.getBytesRead() instead of Deflater.getTotalIn()
490825f: 8325589: C2 SuperWord refactoring: create VLoopAnalyzer with Submodules
3a00fc7: 8323698: Class use page does not include extends/implements type arguments
f32f574: 8326575: Remove unused ContiguousSpace::set_top_for_allocations
09f755a: 8325857: G1 Full GC flushes mark stats cache too early
1799ffe: 8310351: Typo in ImmutableCollections
d10f277: 8326006: Allow TEST_VM_FLAGLESS to set flagless mode
63f6a56: 8326530: Widen allowable error bound of Math.tan

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-26T15:33:09Z

@LanceAndersen Pushed as commit 20c71ce.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk · 2024-02-26T15:39:28Z

@LanceAndersen The command integrate can only be used in open pull requests.

LanceAndersen added 3 commits February 23, 2024 12:55

Initial changes for JDK-8321156

4df1dcd

Merge branch 'master' into JDK-8321156

83e6537

Add tests for JDK-8321156

e76ff74

openjdk bot added the rfr Pull request is ready for review label Feb 24, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Feb 24, 2024

eirbjo reviewed Feb 24, 2024

View reviewed changes

Updates based on 1st round of feedback

dfd4910

LanceAndersen commented Feb 24, 2024

View reviewed changes

jaikiran approved these changes Feb 25, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 25, 2024

jaikiran reviewed Feb 25, 2024

View reviewed changes

test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java Outdated Show resolved Hide resolved

eirbjo reviewed Feb 25, 2024

View reviewed changes

Updates based on 2nd round of feedback

637acee

LanceAndersen commented Feb 25, 2024

View reviewed changes

jaikiran approved these changes Feb 25, 2024

View reviewed changes

AlanBateman reviewed Feb 25, 2024

View reviewed changes

AlanBateman approved these changes Feb 25, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 26, 2024

openjdk bot closed this Feb 26, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Feb 26, 2024

openjdk bot removed the rfr Pull request is ready for review label Feb 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment #17995

8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment #17995

LanceAndersen commented Feb 24, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 24, 2024

openjdk bot commented Feb 24, 2024

mlbridge bot commented Feb 24, 2024 •

edited

Loading

eirbjo left a comment •

edited

Loading

eirbjo commented Feb 24, 2024

LanceAndersen left a comment

LanceAndersen commented Feb 24, 2024

jaikiran left a comment

openjdk bot commented Feb 25, 2024 •

edited

Loading

eirbjo Feb 25, 2024

LanceAndersen Feb 25, 2024

LanceAndersen left a comment

LanceAndersen Feb 25, 2024

LanceAndersen commented Feb 25, 2024

jaikiran left a comment

AlanBateman Feb 25, 2024

LanceAndersen Feb 25, 2024 •

edited

Loading

jaikiran Feb 26, 2024

AlanBateman Feb 26, 2024

AlanBateman left a comment

LanceAndersen commented Feb 26, 2024

LanceAndersen commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

		} catch(Exception ex) {
		throw (ZipException) new ZipException(

8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment #17995

8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment #17995

Conversation

LanceAndersen commented Feb 24, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 24, 2024

openjdk bot commented Feb 24, 2024

mlbridge bot commented Feb 24, 2024 • edited Loading

Webrevs

eirbjo left a comment • edited Loading

Choose a reason for hiding this comment

eirbjo commented Feb 24, 2024

LanceAndersen left a comment

Choose a reason for hiding this comment

LanceAndersen commented Feb 24, 2024

jaikiran left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 25, 2024 • edited Loading

eirbjo Feb 25, 2024

Choose a reason for hiding this comment

LanceAndersen Feb 25, 2024

Choose a reason for hiding this comment

LanceAndersen left a comment

Choose a reason for hiding this comment

LanceAndersen Feb 25, 2024

Choose a reason for hiding this comment

LanceAndersen commented Feb 25, 2024

jaikiran left a comment

Choose a reason for hiding this comment

AlanBateman Feb 25, 2024

Choose a reason for hiding this comment

LanceAndersen Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

jaikiran Feb 26, 2024

Choose a reason for hiding this comment

AlanBateman Feb 26, 2024

Choose a reason for hiding this comment

AlanBateman left a comment

Choose a reason for hiding this comment

LanceAndersen commented Feb 26, 2024

LanceAndersen commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

LanceAndersen commented Feb 24, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 24, 2024 •

edited

Loading

eirbjo left a comment •

edited

Loading

openjdk bot commented Feb 25, 2024 •

edited

Loading

LanceAndersen Feb 25, 2024 •

edited

Loading