8304013: Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries #12231

eirbjo · 2023-01-26T18:49:47Z

The TestTooManyEntries test was originally added to validate that ZIP64 files with CEN sizes exceeding what ZipFile supports are rejected with a ZipException. The test does this by creating a large ZIP file (several gigabytes) with many enties. Because this is resource intensive, the test is currently tagged as manual. (See #6927)

It would be useful to have a test which asserts the CEN size enforcement, but without the CPU, disk, memory and run time requirements of TestTooManyEntries. Such a fast test can run non-manual, without the @requires and manual tags as found in TestTooManyEntries.

This PR adds the EndOfCenValidation test which creates sparse test ZIPs where the CEN is "inflated" such that is matches the size declared in the "End of central directory" records.

While thee sparse files look large, they consume very little disk space on file systems supporting sparse files:

16 -rw-r--r--  1   2147483702 Feb  6 18:54 bad-cen-offset.zip
16 -rw-r--r--  1   2147483703 Feb  6 18:54 cen-size-too-large.zip
 8 -rw-r--r--  1          132 Feb  6 18:54 invalid-zen-size.zip

For good measure, two new test methods are added to excercise the remaining ZipExceptions which ZipFile may throw during validation of the END record .

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8304013: Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries

Reviewers

Lance Andersen (@LanceAndersen - Reviewer)
Martin Buchholz (@Martin-Buchholz - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12231

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

Using diff file

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

…d of central directory" records. This removes the need to a create several GB big "real" ZIP64 file. To facilitate testing, the validation of CEN size > limit is moved from initCEN into findEnd. Since the test now runs fast, make it non-manual.

bridgekeeper · 2023-01-26T18:50:58Z

👋 Welcome back eirbjo! 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 · 2023-01-26T18:52:52Z

@eirbjo 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.

… cases for regular CEN sizes exactly on and exceeding the limit. Move enforcement of the size limit back to initCEN, but before the short-circuiting of empty files.

…s are checked

…ze to the wanted sizes. Remove the ZIP64 logic, since the purpose of the test is to verify rejection of CEN sizes too large, not where the CEN is found.

… runs fast and non-manual

LanceAndersen

Thanks for the updates.

Please see the additional suggestions below

LanceAndersen · 2023-01-31T11:36:25Z

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

+     * The resulting ZIP is technically not valid, but it does allow us
+     * to test that the large CEN size is rejected
+     */
+    private Path zipWithCenSize(String name, int cenSize) throws IOException {


You could speed this up a bit more by just creating the array representing the original Zip 1 time and modify it each pass.(though it might not speed things up that much)

I introduced the templateZip() method which returns a byte[]. While the speed is insignificat here, I think it makes it more explicit that we're producing ZIPs from a template.

LanceAndersen · 2023-01-31T11:37:29Z

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

+    private static final int ENDHDR = 22;          // End of central directory record size
+    private static final int CEN_SIZE_OFFSET = 12; // Offset of CEN size field within ENDHDR
+    private Path huge; // ZIP file with CEN size exceeding limit
+    private Path big;  // ZIP file with CEN size exactly on the limit


You might consider more meaningful names for the Paths

I've removed these fields now and instead produce the files in the test methods directly, removing the need to name the fields.

LanceAndersen · 2023-01-31T11:38:07Z

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

+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+
+public class CenSizeTooLarge {


I would add a comment which also points to the original test

Added a comment referring to TestTooManyEntries explaining why this test exists.

LanceAndersen · 2023-01-31T11:49:05Z

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

+     */
+    private void assertRejected(Path zip, String expectedMsg) throws IOException {
+        try (ZipFile zf = new ZipFile(zip.toFile())) {
+            fail("Expected ZipFile to throw ZipException");


You could consider using expectThrows and then validation the value of getMessage()

Replaced with expectedExceptions and expectedExceptionsMessageRegExp

Now that I finally understand what you meant by this, I've update to use expectThrows and assertEquals :-)

…ment referencing existing CenSizeTooLarge, clarify MAX_CEN_SIZE, use expectedExceptions and expectedExceptionsMessageRegExp

… declared in the End of central directory record. Add a new test which verifies the rejection of too-large CEN offset. Revert the change to END validation in ZipFile which is no longer needed.

…uals

eirbjo · 2023-03-11T07:40:53Z

/issue 8304013

eirbjo · 2023-03-24T17:48:20Z

Thanks for all your help and careful reviews getting this ball across the net, Lance! Feels good to have one less manual test.

/integrate

openjdk · 2023-03-24T17:49:40Z

@eirbjo
Your change (at version e348201) is now ready to be sponsored by a Committer.

Martin-Buchholz · 2023-03-24T18:26:02Z

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

+import static org.testng.Assert.*;
+
+/**
+ * This test augments {@link TestTooManyEntries}. It creates sparse ZIPs where the


remove the "the" stutter

"the" stutter has been removed 👍

Martin-Buchholz · 2023-03-24T18:28:36Z

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

+
+    /**
+     * Validate that an end of central directory record with a
+     * CEN size which exceeds the position of the EOC record is rejected.


EOC is not a standard zip file technical term. Did you mean "END header" ?

If by "standard" you mean "APPNOTE.TXT", then that uses "end of central directory record" which is a bit long.

The Java implementation seems to use END, like in ZipFile.ENDHDR, "END header", etc.

It is probably better to be consistent than correct here, so I've changed the test to use "END header" consistently when referring to the "end of central directory record".

So, what I would suggest is adding a comment somewhere regarding end of central directory record and note the reference to ENDHDR or END Header

I don't think you want to spend too much time here just the wording reference the end of central directory record was clear and where you were abbreviating, then I would reference ENDHDR

I would be looking at the APP.NOTE when looking at this test, not the ZipFile constant verbiage, but that is just me :-)

Neither APPNOTE nor JDK doc zip terminology is great, but a test is not the place to invent something better.

(But I'm surprised no one seems to be using "EOCD")

I think that would be “EOCDR”. (Hides behind the bike shed..)

Martin-Buchholz · 2023-03-24T18:30:26Z

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

+    /**
+     * Validate that an end of central directory record with a CEN offset which
+     * is larger than the EOC position minus the CEN size is rejected
+     * @throws IOException


I don't think @throws javadoc comments in tests are useful; especially checked ones

I recently added a @throws by review comment from Lance. I tend to agree that is not very useful, so I'm removing both ocurrences as per your recommendation.

If Lance wants it back, he can add a review comment saying so. I don't particularly care which professor gets the last word in this matter :-)

Please add them back before pushing with @throws IOException if an error occurs

Yes I understand they do not add much value but it will reduce noise from IDEs and if/when we add more checks for missing javadoc tags, it will save us from having to revisit this

Thanks, I've added @throws IOException if an error occurs to all methods throwing IOException.

For the record, let me state my personal (rather strong) opinion:

I think this a pretty crazy level of boilerplate noise for a test and that it affects readability negatively. My IDE (IntelliJ) does not complain about this at all. Any IDE which complains should be fixed or configured to not complain. Changing Javadoc to complain about this in a scope like this would be a poor decision.

I think the PR is ready to be sponsored after this.

I hear you but the option is not to use javadoc comments and use block comments :-)

I think we can agree to disagree on impacting readability ;-) this comes down to personal preferences

Again thank you for making the change.

I think the PR is ready to be sponsored after this.

Please see my comment regarding the end of central directory record.

I would prefer to tweak that in a fashion similar to what I indicated as I thought your original version was clearer. I understand what Martin was indicating of the use of EOC and that could have been addressed by adding (EOC) or ENDHDR after end of central directory record to make it clearer

I will leave it up to you as to whether you want to make the change but it would be clearer as I think we took a small step backwards.

Either way you should not need a sponsor and should be good to integrated when you are ready

I hear you but the option is not to use javadoc comments and use block comments :-)

I learned something new today: Javadoc comments and block comments are not the same!

So I changed the references to this ultimate ZIP structure to read like this:

'End of central directory record' (END header)

Example:

/** * Validate that an 'End of central directory record' (END header) * where the value of the CEN size field exceeds the position of * the END header is rejected.

This way we stick to the 'official' ZIP verbiage but also allow using the shorter 'END header'.

What do you think?

Either way you should not need a sponsor and should be good to integrated when you are ready

As a non-committer, I still think I need help from a committer to /sponsor this change, right?

I think this a pretty crazy level of boilerplate noise for a test and that it affects readability negatively.

I'm with Eirik here.

I've maintained jdk tests for 20 years and have never had to worry about nagging to add javadoc boilerplate.

doclint should restrict itself to checking what's there, not to check for completeness, in tests and private APIs.

And it looks like the jdk makefiles are in fact set up to work this way.

I see:
./make/Docs.gmk:102:JAVADOC_DISABLED_DOCLINT_WARNINGS := missing

Especially for test methods, the contract of a test is very simple: "do nothing", but possibly throw an informative exception. If the compiler requires a throws declaration, then I like throws Throwable which evokes this contract. A javadoc @throws declaration is the sort of useless boilerplate that makes people hate Java.

eirbjo · 2023-03-25T06:23:10Z

/label remove sponsor

openjdk · 2023-03-25T06:24:36Z

@eirbjo
The label sponsor is not a valid label.
These labels are valid:

serviceability
hotspot
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
hotspot-runtime
jmx
build
nio
client
core-libs
compiler
net
hotspot-gc
hotspot-jfr

…central directory record".

eirbjo · 2023-03-25T06:54:59Z

@LanceAndersen Would you like to do a final review after the changes suggested by Martin in his late review?

It is mostly comment changes, and some stray variable renames. I have run the test locally after the last changes, and it passes on my machine.

This reverts commit 623d374.

…ethods throwing IOException

…of central directory record' (END header)

LanceAndersen

Thank you for the updates here Eirik. They look fine.

You are right, you do need a sponsor still and I will do so Monday morning so that Martin has a chance to provide any last input on your comment updates regarding the End of central directory record.

eirbjo · 2023-03-25T16:02:09Z

Thanks Lance! Allow me to underline just how much I appreciate that we can have differences of opinion while still being friends and making productive progress. Looking around the world, this is not always a given. I see this as a sign of a healty community.

LanceAndersen · 2023-03-26T21:03:59Z

/sponsor

openjdk · 2023-03-26T21:05:03Z

@LanceAndersen The PR has been updated since the change author (@eirbjo) issued the integrate command - the author must perform this command again.

eirbjo · 2023-03-26T21:14:20Z

/integrate

openjdk · 2023-03-26T21:16:32Z

@eirbjo
Your change (at version a93248f) is now ready to be sponsored by a Committer.

LanceAndersen · 2023-03-26T22:58:12Z

/sponsor

openjdk · 2023-03-26T23:00:28Z

Going to push as commit 65e01da.
Since your change was applied there have been 28 commits pushed to the master branch:

38e1714: 8304258: x86: Improve the code generation of VectorRearrange with int and float
765a942: 8304136: Match allocation and free in sspi.cpp
3f59b75: 8304898: Fix Copyright Headers for JLink Source Files
501b606: 8298725: Add BitMap support for reverse iteration
9764948: 8273986: JEditorPane HTML Demo - Accessibility issues
5727610: 8304353: Add lib-test tier1 testing in GHA
d8ba227: 8304069: ClassFileParser has ad-hoc hashtables
9a8a60f: 8304833: (fc) Remove dead code in sun.nio.ch.FileChannelImpl::implCloseChannel
f96aee7: 8291154: Create a non static nested class without enclosing class throws VerifyError
4ec720d: 8297977: vmTestbase/nsk/stress/except/except012.java fails with unexpected Exception
... and 18 more: https://git.openjdk.org/jdk/compare/63d4afbeb17df4eff0f65041926373ee62a8a33a...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-26T23:00:43Z

@LanceAndersen @eirbjo Pushed as commit 65e01da.

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

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 26, 2023

eirbjo changed the title ~~Update TestTooManyEntries run non-manual with less resources~~ Update TestTooManyEntries to run non-manual with less resources Jan 26, 2023

eirbjo added 10 commits January 27, 2023 09:16

Regular, non-ZIP64 files can also exceed the CEN size limit. Add test…

51dd9de

… cases for regular CEN sizes exactly on and exceeding the limit. Move enforcement of the size limit back to initCEN, but before the short-circuiting of empty files.

Update copyright year

b10bbbe

Fix copyright years

4ef2cd9

Remove ZIP64 from the summary now that also regular ZIP file CEN size…

adcf9d6

…s are checked

Style: Add a missing whitespace

8659342

Merge branch 'master' into zip-too-many-entries

39b1466

Use ZipOutoutStream to create a real ZIP file, then update the CEN si…

2e162c6

…ze to the wanted sizes. Remove the ZIP64 logic, since the purpose of the test is to verify rejection of CEN sizes too large, not where the CEN is found.

CEN size checking can be moved after the empty-ZIP check

bc0a79b

Add a comment for the CEN size update

9d47aa7

Revert changes to TestTooManyEntries and instead add a new test which…

8396bb2

… runs fast and non-manual

eirbjo changed the title ~~Update TestTooManyEntries to run non-manual with less resources~~ Add test CenSizeTooLarge as a fast, non-manual alternative to TestTooManyEntries Jan 30, 2023

LanceAndersen reviewed Jan 31, 2023

View reviewed changes

eirbjo added 4 commits February 2, 2023 13:53

Updates from review comments: put ZIP template in byte array, add com…

8bcb4d2

…ment referencing existing CenSizeTooLarge, clarify MAX_CEN_SIZE, use expectedExceptions and expectedExceptionsMessageRegExp

No need to pass template ZIP as parameter

c4c3143

Inflate the CEN size using sparse files such that it matches the size…

12378d9

… declared in the End of central directory record. Add a new test which verifies the rejection of too-large CEN offset. Revert the change to END validation in ZipFile which is no longer needed.

Fix spelling in comment

fb49e7f

eirbjo changed the title ~~Add test CenSizeTooLarge as a fast, non-manual alternative to TestTooManyEntries~~ Add test EndOfCenValidation as a fast, non-manual alternative to TestTooManyEntries Feb 6, 2023

eirbjo added 3 commits February 14, 2023 14:43

Merge branch 'master' into zip-too-many-entries

eec61f9

Replace expectedExceptions annotations with expectThrows and assertEq…

bff9923

…uals

Merge branch 'master' into zip-too-many-entries

96977ae

eirbjo changed the title ~~Add test EndOfCenValidation as a fast, non-manual alternative to TestTooManyEntries~~ Add test EndOfCenValidation as non-manual alternative to TestTooManyEntries Mar 11, 2023

eirbjo marked this pull request as draft March 11, 2023 07:35

eirbjo changed the title ~~Add test EndOfCenValidation as non-manual alternative to TestTooManyEntries~~ Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries Mar 11, 2023

openjdk bot changed the title ~~Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries~~ 8304013: Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries Mar 11, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 24, 2023

Martin-Buchholz suggested changes Mar 24, 2023

View reviewed changes

eirbjo added 3 commits March 25, 2023 07:26

Remove "the the" repetition

2a239b3

Consistently use the term "END header" when referring to the "End of …

ef9c5d2

…central directory record".

Remove @throws as per review feedback

623d374

openjdk bot removed the sponsor Pull request is ready to be sponsored label Mar 25, 2023

eirbjo added 3 commits March 25, 2023 11:50

Revert "Remove @throws as per review feedback"

700154f

This reverts commit 623d374.

Add consistent '@throws IOException if an error occurs' tags to all m…

1b253a1

…ethods throwing IOException

Consistently refer to the ultimate ZIP header/record like this: 'End …

a93248f

…of central directory record' (END header)

LanceAndersen approved these changes Mar 25, 2023

View reviewed changes

Martin-Buchholz approved these changes Mar 26, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 26, 2023

openjdk bot added the integrated Pull request has been integrated label Mar 26, 2023

openjdk bot closed this Mar 26, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 26, 2023

8304013: Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries #12231

8304013: Add a fast, non-manual alternative to test/jdk/java/util/zip/ZipFile/TestTooManyEntries #12231

Conversation

eirbjo commented Jan 26, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 26, 2023

openjdk bot commented Jan 26, 2023

LanceAndersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirbjo commented Mar 11, 2023

eirbjo commented Mar 24, 2023

openjdk bot commented Mar 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LanceAndersen Mar 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirbjo commented Mar 25, 2023

openjdk bot commented Mar 25, 2023

eirbjo commented Mar 25, 2023

LanceAndersen left a comment

Choose a reason for hiding this comment

eirbjo commented Mar 25, 2023

LanceAndersen commented Mar 26, 2023

openjdk bot commented Mar 26, 2023

eirbjo commented Mar 26, 2023

openjdk bot commented Mar 26, 2023

LanceAndersen commented Mar 26, 2023

openjdk bot commented Mar 26, 2023

openjdk bot commented Mar 26, 2023

eirbjo commented Jan 26, 2023 •

edited by openjdk bot

LanceAndersen Mar 25, 2023 •

edited