Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v9]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field. This brings ZipInputStream > into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 33 commits: - Merge branch 'master' into data-descriptor - Extract ZIP64_BLOCK_SIZE_OFFSET as a constant - A Zip64 extra field used in a LOC header must include both the uncompressed and compressed size fields, and does not include local header offset or disk start number fields. Conequently, a valid LOC Zip64 block must always be 16 bytes long. - Document better the zip command and options used to generate the test vector ZIP - Fix spelling of "presence" - Add a @bug reference in the test - Use the term "block size" when referring to the size of a Zip64 extra field data block - Update comment reflect that a Zip64 extended field in a LOC header has only two valid block sizes - Convert test from testNG to JUnit - Fix the check that the size of an extra field block size must not grow past the total extra field length - ... and 23 more: https://git.openjdk.org/jdk/compare/e2042421...ddff130f - Changes: https://git.openjdk.org/jdk/pull/12524/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12524=08 Stats: 238 lines in 2 files changed: 237 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files
On Sun, 12 Feb 2023 17:04:51 GMT, Alan Bateman wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field. This brings >> ZipInputStream into alignment with the APPNOTE format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > This proposed change will require a lot of review cycles and a lot of > testing. The current implementation came about because of issues with several > zip tools so we have to be very careful changing it. @AlanBateman I was planning to integrate this myself pending the OpenJDK census update for my JDK Commiter status and the Skara team linking my Github account. But there is no rush here, I'm happy to hold off until you find time to provide feedback. This can wait, please enjoy your holidays! :-) - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1867350502
Re: RFR: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile [v8]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip`, > `ZipFile/input.jar` and `ZipFile/crash.jar` > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired or moved into other test files as separate > methods. See comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. > After discussion, we decided to merge this test into `ReadZip.java`. I added > some checks to verify that reading from the stream reduces the number of > available bytes accordingly, also checking the behavior when the stream is > closed. > > `CopyJar.java` > The concern of copying entries seems to now have better coverage in the test > `zip/CopyZipFile`. We decided to retire this test rather than convert it and > instead convert `CopyZipFile` to JUnit. > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > We decided to merge this test into `ReadZip.readDirectoryEntries` rather than > keeping it as a separate test. > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was using TestNG so was converted to JUnit for consistency. Added > some comments to help understanding. > > `ReadAfterClose.java` > This uses the binary test vector `crash.jar`. The test is converted to JUnit > and moved to `ReadZip.readAfterClose´. The test is expanded to exercise more > read methods. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 14 additional commits since the last revision: - Merge branch 'master' into readzip-junit - Merge the ReadAfterClose test into ReadZip, converting it to JUnit. - Improve comment explaining what happens when ZipEntry.setCompressedSize is calledExplicitly - Convert CopyZipFile.java to JUnit - Remove trailing whitespace - Merge GetDirEntry.java into ReadZip.readDirectoryEntries() - Retire redundant test ZipFile/CopyJar - Merge Available.java into ReadZip.java - Add @bug reference 4401122 on the Available.java test - Merge branch 'master' into readzip-junit - ... and 4 more: https://git.openjdk.org/jdk/compare/82804792...85500cd8 - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/27fe1131..85500cd8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17038=07 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=06-07 Stats: 3896 lines in 272 files changed: 2360 ins; 631 del; 905 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
RFR: 8322565 (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits
This PR suggests that `Files.setPosixPermissions`as implemented by `ZipFileSystem` should preserve the leading seven bits of the 'external file attributes' field. These bits contain the 'file type', 'setuid', 'setgid', and 'sticky' bits. These are unrelated to permissions and should not be modified by this operation. The fix is to update `Entry.readCEN` to read all 16 bits and to update `ZipFileSystem.setPermissions` to preserve the leading 7 bits when updating the trailing 9 permission-related bits of the `Entry.posixPerms` field. The PR adds a new test `TestPosix.preserveRemainingBits()` which verifies that the leading 7 bits are not affected by `Files.setPosixPermissions`. Note that this PR does not aim to preserve the leading seven bits for the case when `Files.setPosixPermissions` is called with a `null` permission set. (The implementation currently interprets this as a signal that the 'external file attributes' should not be populated and the 'version made by' OS will be MSDOS instead of Unix) - Commit messages: - Preserve non-permission 'external file attributes' bits when setting posix permissions Changes: https://git.openjdk.org/jdk/pull/17170/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17170=00 Issue: https://bugs.openjdk.org/browse/JDK-8322565 Stats: 83 lines in 2 files changed: 75 ins; 4 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/17170.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17170/head:pull/17170 PR: https://git.openjdk.org/jdk/pull/17170
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v4]
On Fri, 15 Dec 2023 21:13:07 GMT, Archie Cobbs wrote:
>> `GZIPInputStream`, when looking for a concatenated stream, relies on what
>> the underlying `InputStream` says is how many bytes are `available()`. But
>> this is inappropriate because `InputStream.available()` is just an estimate
>> and is allowed (for example) to always return zero.
>>
>> The fix is to ignore what's `available()` and just proceed and see what
>> happens. If fewer bytes are available than required, the attempt to extend
>> to another stream is canceled just as it was before, e.g., when the next
>> stream header couldn't be read.
>
> Archie Cobbs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address third round of review comments.
The current behavior of allowing/ignoring trailing malformed data seems to have
a complicated history:
- GZipInputStream was updated to throw ZipExeption instead of IOException on
malformed GZIP data in [4263582](https://bugs.openjdk.org/browse/JDK-4263582)
- The ability to read concatenated GZ files was added in
[JDK-4691425](https://bugs.openjdk.org/browse/JDK-4691425) This change
interestingly also introduced the current behavior of ignoring any trailing
malformed data in the stream.
- [7021870](https://bugs.openjdk.org/browse/JDK-7021870) fixed a bug where
GZipInputStream closed the underlying input stream. The change also introduced
the test GZIPInZip which verified that reads from a wrapped ZipInputStream does
not close the stream
- Some months later GZIPInZip was updated in fix a test failure, but the change
also modified the test to verifiy that malformed trailing data was ignored. The
JBS issue is not available to me:
[JDK-8023431](https://bugs.openjdk.org/browse/JDK-8023431)
- Soon after this, GZIPInZip was again updated to fix test failure, this time
removing the use of piped streams and threads. The JBS issue is not available
to me: [JDK-8026756](https://bugs.openjdk.org/browse/JDK-8026756)
The current behavior of ignoring trailing malformed data does not seem to be
specified in the API. On the contrary, the read methods are specified to throw
ZipException for corrupt input data:
* @throwsZipException if the compressed input data is corrupt.
* @throwsIOException if an I/O error has occurred.
*
*/
public int read(byte[] buf, int off, int len) throws IOException {
Not sure whether it is worthwhile to change this long-standing behavior of
GZIpInputStream. But it could perhaps be noted somehow in the API
documentation? (To be clear, that would be for a different PR/issue/CSR)
-
PR Comment: https://git.openjdk.org/jdk/pull/17113#issuecomment-1859177655
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v3]
On Fri, 15 Dec 2023 16:30:01 GMT, Archie Cobbs wrote: >> `GZIPInputStream`, when looking for a concatenated stream, relies on what >> the underlying `InputStream` says is how many bytes are `available()`. But >> this is inappropriate because `InputStream.available()` is just an estimate >> and is allowed (for example) to always return zero. >> >> The fix is to ignore what's `available()` and just proceed and see what >> happens. If fewer bytes are available than required, the attempt to extend >> to another stream is canceled just as it was before, e.g., when the next >> stream header couldn't be read. > > Archie Cobbs has updated the pull request incrementally with one additional > commit since the last revision: > > Address second round of review comments. The test is shaping up nicely. Since it's a new test it should use JUnit 5. Also included a couple suggestions, I'll stop now, promise! :) test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 30: > 28: */ > 29: > 30: import org.junit.Test; Let's use JUnit 5: Suggestion: import org.junit.jupiter.api.Test; test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 36: > 34: import java.util.zip.*; > 35: > 36: import static org.junit.Assert.assertArrayEquals; Let's use JUnit 5: Suggestion: import static org.junit.jupiter.api.Assertions.assertArrayEquals; test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 53: > 51: byte[] compressedN = repeat(compressed1, NUM_COPIES); > 52: > 53: // (a) Read back copied compressed data from a stream where > available() is accurate and verify Suggestion: // (a) Read back inflated data from a stream where available() is accurate and verify test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 54: > 52: > 53: // (a) Read back copied compressed data from a stream where > available() is accurate and verify > 54: byte[] readback1 = new GZIPInputStream(new > ByteArrayInputStream(compressedN)).readAllBytes(); These readback lines got rather long. Perhaps consider extracting the GZIP reading into a method taking the source InputStream as a parameter? Suggestion: byte[] readback1 = inflateFrom(new ByteArrayInputStream(compressedN)); test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 57: > 55: assertArrayEquals(uncompressedN, readback1); > 56: > 57: // (b) Read back copied compressed data from a stream where > available() always returns zero and verify Suggestion: // (b) Read back inflated data from a stream where available() always returns zero and verify test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 58: > 56: > 57: // (b) Read back copied compressed data from a stream where > available() always returns zero and verify > 58: byte[] readback2 = new GZIPInputStream(new > ZeroAvailableStream(new ByteArrayInputStream(compressedN))).readAllBytes(); Suggestion: byte[] readback2 = inflateFrom(new ZeroAvailableStream(new ByteArrayInputStream(compressedN))); - PR Review: https://git.openjdk.org/jdk/pull/17113#pullrequestreview-1784857282 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428434843 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428435461 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428452384 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428455030 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428452815 PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428456971
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v2]
On Fri, 15 Dec 2023 03:20:01 GMT, Archie Cobbs wrote:
>> `GZIPInputStream`, when looking for a concatenated stream, relies on what
>> the underlying `InputStream` says is how many bytes are `available()`. But
>> this is inappropriate because `InputStream.available()` is just an estimate
>> and is allowed (for example) to always return zero.
>>
>> The fix is to ignore what's `available()` and just proceed and see what
>> happens. If fewer bytes are available than required, the attempt to extend
>> to another stream is canceled just as it was before, e.g., when the next
>> stream header couldn't be read.
>
> Archie Cobbs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments.
A few minor suggestions.
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 52:
> 50: buf.reset();
> 51: for(int i = 0; i < 100; i++)
> 52: buf.write(gz);
Whitespace after `for`, braces are recommended even when optional in the
language:
Suggestion:
for (int i = 0; i < 100; i++) {
buf.write(gz);
}
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 53:
> 51: for(int i = 0; i < 100; i++)
> 52: buf.write(gz);
> 53: final byte[] gz32 = buf.toByteArray();
Drop final, consider finding a more expressive name:
Suggestion:
byte[] concatenatedGz = buf.toByteArray();
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 56:
> 54:
> 55: // (a) Read it back from a stream where available() is accurate
> 56: long count1 = countBytes(new GZIPInputStream(new
> ByteArrayInputStream(gz32)));
This is the expected number of bytes to be read. This could be calculated
directly from the uncompressed data. At least the name should express that this
is the expected number of bytes:
Suggestion:
long expectedBytes = countBytes(new GZIPInputStream(new
ByteArrayInputStream(gz32)));
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 62:
> 60:
> 61: // They should be the same
> 62: Assert.assertEquals(count2, count1);
This could be a static import. And for the assertion failure message to be
correct, the expected value must come first:
Suggestion:
assertEquals(expectedBytes, actualBytes);
An alternative here could be to store the original uncompressed data and
compare that to the full inflated data read from GZIPInputStream using
assertArrayEquals. The length alone is a bit of a weak proxy for equality.
-
PR Review: https://git.openjdk.org/jdk/pull/17113#pullrequestreview-1784136773
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428015571
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428017078
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428019017
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428020568
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
On Thu, 14 Dec 2023 20:15:39 GMT, Archie Cobbs wrote:
> `GZIPInputStream`, when looking for a concatenated stream, relies on what the
> underlying `InputStream` says is how many bytes are `available()`. But this
> is inappropriate because `InputStream.available()` is just an estimate and is
> allowed (for example) to always return zero.
>
> The fix is to ignore what's `available()` and just proceed and see what
> happens. If fewer bytes are available than required, the attempt to extend to
> another stream is canceled just as it was before, e.g., when the next stream
> header couldn't be read.
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 37:
> 35: public static void main(String [] args) throws IOException {
> 36:
> 37: // Create gz data
Perhaps expand the comment to explain that you are creating a concatenated
stream?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427291799
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
On Thu, 14 Dec 2023 20:15:39 GMT, Archie Cobbs wrote:
> `GZIPInputStream`, when looking for a concatenated stream, relies on what the
> underlying `InputStream` says is how many bytes are `available()`. But this
> is inappropriate because `InputStream.available()` is just an estimate and is
> allowed (for example) to always return zero.
>
> The fix is to ignore what's `available()` and just proceed and see what
> happens. If fewer bytes are available than required, the attempt to extend to
> another stream is canceled just as it was before, e.g., when the next stream
> header couldn't be read.
The test could benefit from a conversion to JUnit. Not sure I love final local
variables, I see the split assignment inside the try/catch makes it useful, but
perhaps if you rewrite countBytes as suggested, final will be less useful.
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 54:
> 52: try (GZIPInputStream in = new GZIPInputStream(new
> ByteArrayInputStream(gz32))) {
> 53: count1 = countBytes(in);
> 54: }
Consider rewriting countBytes to take the
ByteArrayInputStream/ZeroAvailableInputStream as a parameter, so you could just
do:
```suggestion
long count1 = countBytes(new ByteArrayInputStream(gz32));
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 56:
> 54: }
> 55:
> 56: // (a) Read it from a stream where available() always returns zero
Suggestion:
// (b) Read it from a stream where available() always returns zero
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 64:
> 62: // They should be the same
> 63: if (count2 != count1)
> 64: throw new AssertionError(count2 + " != " + count1);
Consider converting the test to JUnit, this would allow:
Suggestion:
asssertEquals(count1, count2);
-
PR Review: https://git.openjdk.org/jdk/pull/17113#pullrequestreview-1782746089
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427283057
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427283554
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427285307
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v7]
On Thu, 14 Dec 2023 14:55:08 GMT, Eirik Bjorsnos wrote: >> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and >> `ZipFile/input.jar`. >> >> Binary test vectors are harder to analyze, and sharing test vectors across >> unrelated tests increases maintenance burden. It would be better to have >> each test produce its own test vectors independently. >> >> While visiting these dusty tests, we should take the opportunity to convert >> them to JUnit, add more comments and perform some mild modernization and >> cleanups where appropriate. We should also consider whether any test are >> duplicated and can be retired, see comments below. >> >> To help reviewers, here are some comments on the updated tests: >> >> `Available.java` >> This test currently has no jtreg `@test` header, so isn't currently active. >> After discussion, we decided to merge this test into `ReadZip.java`. I added >> some checks to verify that reading from the stream reduces the number of >> available bytes accordingly, also checking the behavior when the stream is >> closed. >> >> `CopyJar.java` >> The concern of copying entries seems to now have better coverage in the test >> `zip/CopyZipFile`. We decided to retire this test rather than convert it and >> instead convert `CopyZipFile` to JUnit. >> >> `EnumAfterClose.java` >> To prevent confusion with Java Enums, I suggest we rename this test to >> `EnumerateAfterClose`. >> >> `FinalizeInflater.java` >> The code verified by this test has been updated to use cleaners instead of >> finalizers. Still, this test code relies on finalizers. Not sure if this is >> an issue, but this test will need to be updated when finalizers are finally >> removed. >> >> `GetDirEntry.java` >> We decided to merge this test into `ReadZip.readDirectoryEntries` rather >> than keeping it as a separate test. >> >> `ReadZip.java` >> Nothing exciting here, the single main method was split into multiple JUnit >> methods, each focusing on a separate concern. >> >> `ReleaseInflater.java` >> Nothing exciting, tried to add some comment to help understanding of what is >> tested. >> >> `StreamZipEntriesTest.java` >> This test was using TestNG so was converted to JUnit for consistency. Added >> some comments to help understanding. >> >> `ReadAfterClose.java` >> This uses the binary test vector `crash.jar`. The test is converted to JUnit >> and moved to `ReadZip.readAfterClose´. The test is expanded to exercise more >> read methods. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Merge the ReadAfterClose test into ReadZip, converting it to JUnit. Seeing that `ReadAfterClose.java` uses a binary test vector `crash.jar`, I think it makes sense to include it in this PR, convert it to JUnit and move it into `ReadZip.readAfterClose`. This removes the last remaining binary test vector ZIP in the `ZipFile/` directory. - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1856008970
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v7]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. > After discussion, we decided to merge this test into `ReadZip.java`. I added > some checks to verify that reading from the stream reduces the number of > available bytes accordingly, also checking the behavior when the stream is > closed. > > `CopyJar.java` > The concern of copying entries seems to now have better coverage in the test > `zip/CopyZipFile`. We decided to retire this test rather than convert it and > instead convert `CopyZipFile` to JUnit. > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > We decided to merge this test into `ReadZip.readDirectoryEntries` rather than > keeping it as a separate test. > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was using TestNG so was converted to JUnit for consistency. Added > some comments to help understanding. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Merge the ReadAfterClose test into ReadZip, converting it to JUnit. - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/8623effd..27fe1131 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17038=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=05-06 Stats: 84 lines in 3 files changed: 33 ins; 51 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v6]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. > After discussion, we decided to merge this test into `ReadZip.java`. I added > some checks to verify that reading from the stream reduces the number of > available bytes accordingly, also checking the behavior when the stream is > closed. > > `CopyJar.java` > The concern of copying entries seems to now have better coverage in the test > `zip/CopyZipFile`. We decided to retire this test rather than convert it. > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > We decided to merge this test into `ReadZip.readDirectoryEntries` rather than > keeping it as a separate test. > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was using TestNG so was converted to JUnit for consistency. Added > some comments to help understanding. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Improve comment explaining what happens when ZipEntry.setCompressedSize is calledExplicitly - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/4e9c97d4..8623effd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17038=05 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=04-05 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v5]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Convert CopyZipFile.java to JUnit - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/593a76cd..4e9c97d4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17038=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=03-04 Stats: 195 lines in 1 file changed: 82 ins; 19 del; 94 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
On Thu, 14 Dec 2023 11:39:22 GMT, Lance Andersen wrote: > (though CopyZipFile could use a junit conversion ;-) Agreed, I have included that conversion in this PR :-) This test can make good use of `assertThrows` and splitting different concerns into separate test methods. - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855914377
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v4]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Remove trailing whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/03cb8354..593a76cd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17038=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
On Thu, 14 Dec 2023 11:39:22 GMT, Lance Andersen wrote: >> The scope of this PR has now expanded to removing uses of the `input.zip` >> and `input.jar` files, updating any test using them to produce their own >> test vectors, and convert affected tests to JUnit. >> >> I'm marking the PR ready for review again. Before looking too closely at the >> code, it would be useful to discuss the following tests: >> >> - `Available.java`: This test has no jtreg header. I've added one and >> converted the test. Is this worthwhile, or should we rather remove it? >> - `CopyJar.java`: The concern tested seems to have superior coverage in the >> test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of >> coverting it? >> - `DirEntry.java`: There is duplication between this test and >> `ReadZip.readDirectoryEntry()`. Should we retire one of these? > >> The scope of this PR has now expanded to removing uses of the `input.zip` >> and `input.jar` files, updating any test using them to produce their own >> test vectors, and convert affected tests to JUnit. >> >> I'm marking the PR ready for review again. Before looking too closely at the >> code, it would be useful to discuss the following tests: >> >> * `Available.java`: This test has no jtreg header. I've added one and >> converted the test. Is this worthwhile, or should we rather remove it? > > This could be moved into ReadZip. I do not believe we have a specific test > and it is trivial > >> * `CopyJar.java`: The concern tested seems to have superior coverage in the >> test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of >> coverting it? > > Yes CopyZipFile already exercises Zipfile.ZipInputStream so it is safe to > retire CopyJar (though CopyZipFile could use a junit conversion ;-) >> * `DirEntry.java`: There is duplication between this test and >> `ReadZip.readDirectoryEntry()`. Should we retire one of these? > > I believe you meant GetDirEntry.java not DirEntry.java? > > Having a test that specifically validates we can read META-INF is not a bad > thing, but I suspect we have a test that already does that if not in the > java/util/zip tests or java/util/jar tests. If not we should keep it but > merge it as you suggest @LanceAndersen Thanks for your guidance! I moved `Available` into `ReadZip`, deleted `CopyJar` and merged `GetDirEntry` into ReadZip.readDirectoryEntries (adding a 'META-INF/' directory just in case) - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855753641
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v3]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... Eirik Bjorsnos has updated the pull request incrementally with three additional commits since the last revision: - Merge GetDirEntry.java into ReadZip.readDirectoryEntries() - Retire redundant test ZipFile/CopyJar - Merge Available.java into ReadZip.java - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/56f55d89..03cb8354 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17038=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=01-02 Stats: 361 lines in 4 files changed: 59 ins; 296 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v2]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Add @bug reference 4401122 on the Available.java test - Merge branch 'master' into readzip-junit - The input.jar test vector included a META-INF/ directory before the manifest, lets keep it that way - Remove the use of binary test vector ZIP/JAR files input.zip and input.jar, converting affected tests to JUnit - Update copyright year - Convert ReadZip to JUnit - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/f6a00f0c..56f55d89 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17038=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=00-01 Stats: 5608 lines in 92 files changed: 3340 ins; 1807 del; 461 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
On Fri, 8 Dec 2023 20:28:20 GMT, Eirik Bjorsnos wrote: > This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... The scope of this PR has now expanded to removing uses of the `input.zip` and `input.jar` files, updating any test using them to produce their own test vectors, and convert affected tests to JUnit. I'm marking the PR ready for review again. Before looking too closely at the code, it would be useful to discuss the following tests: - `Available.java`: This test has no jtreg header. I've added one and converted the test. Is this worthwhile, or should we rather remove it? - `CopyJar.java`: The concern tested seems to have superior coverage in the test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of coverting it? - `DirEntry.java`: There is duplication between this test and `ReadZip. readDirectoryEntry`. Should we retire one of these? - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855560373
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v11]
On Mon, 23 Oct 2023 16:12:45 GMT, Sean Coffey wrote: >> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source >> objects aren't created for the same zip file. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Update jmh test comments I noticed the ZipSourceCache test fail on GHA on windows-x64: FAILED ZipSourceCache::testKeySourceMapping 'testKeySourceMapping()' java.nio.file.FileSystemException: 1702471080605-bug8317678.zip: The process cannot access the file because it is being used by another process at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:92) at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103) at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108) at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:273) at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:109) at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191) at ZipSourceCache.cleanup(ZipSourceCache.java:65) On a re-run of the jobs, the test ran green again on the same source. So this seems timing related. - PR Comment: https://git.openjdk.org/jdk/pull/16115#issuecomment-1854702162
Re: RFR: JDK-8319626: Override toString() for ZipFile [v8]
On Wed, 13 Dec 2023 20:53:17 GMT, Justin Lu wrote: > > I'm seeing the `ZipSourceCache` fail on GHA on `windows-x64`: > > Hi Eirik, > > Please let me know if I'm misunderstanding, I claim `TooManyOpenTabsException` :-) Sorry, this comment was for #16115 - PR Comment: https://git.openjdk.org/jdk/pull/16643#issuecomment-1854696711
Re: RFR: JDK-8319626: Override toString() for ZipFile [v8]
On Mon, 11 Dec 2023 05:47:33 GMT, Justin Lu wrote: >> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319982) >> which overrides and provides an implementation of `toString()` in >> _java.util.zip.ZipFile_ (and by extension, _java.util.jar.JarFile_). > > Justin Lu has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 10 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8319626 > - break line > - reflect review: change impl to store field name > - drop additional specification > - drop 2nd paragraph > - return base name, not full path > - shorten wording > - reflect review: use separator, clarify spec > - reflect review: change string value and drop spec > - init I'm seeing the `ZipSourceCache` fail on GHA on `windows-x64`: FAILED ZipSourceCache::testKeySourceMapping 'testKeySourceMapping()' java.nio.file.FileSystemException: 1702471080605-bug8317678.zip: The process cannot access the file because it is being used by another process at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:92) at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103) at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108) at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:273) at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:109) at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191) at ZipSourceCache.cleanup(ZipSourceCache.java:65) Build summary: https://github.com/eirbjo/jdk/actions/runs/7194395508 - PR Comment: https://git.openjdk.org/jdk/pull/16643#issuecomment-1854508435
Re: RFR: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem [v2]
On Tue, 12 Dec 2023 11:47:35 GMT, Lance Andersen wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use uppercase LOC in ZipException messages
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 2579:
>
>> 2577: }
>> 2578: if (LOCSIG(buf) != LOCSIG) {
>> 2579: throw new ZipException("invalid loc header (bad
>> signature)");
>
> Please change loc -> LOC
Fixed, including the ZipException for the preceding read check.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17059#discussion_r1423888116
Re: RFR: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem [v2]
> Please review this PR which adds validation of incorrect LOC signatures in > `ZipFileSystem`. > > `ZipFile` already rejects the case where the offset pointed to from the CEN > header does not start with the expected LOC signature. It makes sense to add > this check to `ZipFileSystem` as well. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Use uppercase LOC in ZipException messages - Changes: - all: https://git.openjdk.org/jdk/pull/17059/files - new: https://git.openjdk.org/jdk/pull/17059/files/bd18ce34..c7ca450e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17059=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17059=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17059.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17059/head:pull/17059 PR: https://git.openjdk.org/jdk/pull/17059
RFR: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem
Please review this PR which adds validation of incorrect LOC signatures in `ZipFileSystem`. `ZipFile` already rejects the case where the offset pointed to from the CEN header does not start with the expected LOC signature. It makes sense to add this check to `ZipFileSystem` as well. - Commit messages: - Vaidate that LOC offset pointed to from the CEN actually starts with the expected LOC signature Changes: https://git.openjdk.org/jdk/pull/17059/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17059=00 Issue: https://bugs.openjdk.org/browse/JDK-8321802 Stats: 14 lines in 2 files changed: 13 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17059.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17059/head:pull/17059 PR: https://git.openjdk.org/jdk/pull/17059
Re: RFR: 8321616: Convert the ReadZip test to JUnit
On Mon, 11 Dec 2023 17:40:24 GMT, Lance Andersen wrote: > I don't have a preference whether we deal with input.jar separately, but > these tests are not that complex so I do not see a risk if they are all > converted at once, or done piece meal Thanks, I'll extend the goal of this PR to remove `input.jar`, `input.zip` and convert affected tests to JUnit along the way. - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1850570227
Re: RFR: 8321616: Convert the ReadZip test to JUnit
On Mon, 11 Dec 2023 16:51:28 GMT, Lance Andersen wrote: > One quick comment, if we are updating this test, we should look to get rid of > input.zip I started going down that road, but felt uneasy about the amount of unrelated changes in a single PR. I'd like to make efficient use of reviewer time, so my preference was to focus on the JUnit conversion, without too many other changes. If you prefer that we get rid of input.zip and also convert the affected tests to JUnit in the same PR, then I'm happy to switch strategy. Note that `StreamZipEntriesTest` also uses `input.jar`, this is also read by `Available`, `CopyJar`, `GetDirEntry`, `ReleaseInflater`. Should we get rid of `input.jar` as well, or perhaps defer that to a follow-up? - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1850524996
RFR: 8321616: Convert the ReadZip test to JUnit
Please review this PR which suggests we rewrite the `../zip/ZipFile/ReadZip.java` test to JUnit. The current test is a single main method with a sequence of fairly unrelated scenarios. It would benefit from a rewrite to multiple JUnit test methods and some general modernization and simplification of the code. - Commit messages: - Update copyright year - Convert ReadZip to JUnit Changes: https://git.openjdk.org/jdk/pull/17038/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17038=00 Issue: https://bugs.openjdk.org/browse/JDK-8321616 Stats: 222 lines in 1 file changed: 80 ins; 48 del; 94 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321396: Retire test/jdk/java/util/zip/NoExtensionSignature.java
On Tue, 5 Dec 2023 15:58:14 GMT, Eirik Bjorsnos wrote: > Please review this PR which suggests we retire the ZIP test > `NoExtensionSignature` along with its `test.jar` test vector. > > The concern of a missing data descriptor signature is covered by the recently > updated `DataDescriptorSignatureMissing` test, see #12959. That test is more > complete, includes more documentation and uses a programmatically generated > test vector. > > One might argue that 'more tests are always better', but in this case I think > the 21 year old `NoExtensionSignature` test with its binary test vector is > nebulous and requires extensive analysis to understand, more so to update. I > think it does not carry its weight and should be retired. > > Careful analysis of the deleted `test.jar` test vector revealed that it > contains a local header with non-zero `compressed size` and `uncompressed > size` fields for a streaming-mode entry. `APPNOTE.TXT` mandates that when bit > 3 of the general purpose bit flag is set, then these fields and the `crc` > field should all be set to zero. > > By injecting assertions into `ZipInputStream.readLOC` I was able to determine > that `NoExtensionSignature` is the only test currently parsing a ZIP file > with such non-zero fields in streaming mode. > > Because of this, and out of caution, this PR introduces a new test > `DataDescriptorIgnoreCrcAndSizeFields` which explicitly verifies that > `ZipInputStream` does not read any non-zero `crc`, `compressed size` or > `uncompressed size` field values from a local header when in streaming mode. > `ZipInputStream` should ignore these values and it would be good to have a > test which asserts that this invariant holds even when the fields are > non-zero. Here's the relevant section from `APPNOTE.TXT`: 4.4.4 general purpose bit flag: (2 bytes) [..] Bit 3: If this bit is set, the fields crc-32, compressed size and uncompressed size are set to zero in the local header. The correct values are put in the data descriptor immediately following the compressed data. One could argue we should validate and throw a `ZipException` for such entries, but that is perhaps a question for another PR. - PR Comment: https://git.openjdk.org/jdk/pull/16975#issuecomment-1847156258
Re: RFR: 8321396: Retire test/jdk/java/util/zip/NoExtensionSignature.java
On Thu, 7 Dec 2023 18:14:59 GMT, Lance Andersen wrote: > Eirik, Could you add a reference to [PR > 12959](https://github.com/openjdk/jdk/pull/12959/) or to > [JDK-8303920](https://bugs.openjdk.org/browse/JDK-8303920) in the above Thanks, that makes sense! - PR Comment: https://git.openjdk.org/jdk/pull/16975#issuecomment-1845880582
Re: RFR: 8321396: Retire test/jdk/java/util/zip/NoExtensionSignature.java
On Tue, 5 Dec 2023 15:58:14 GMT, Eirik Bjorsnos wrote: > Please review this PR which suggests we retire the ZIP test > `NoExtensionSignature` along with its `test.jar` test vector. > > The concern of a missing data descriptor signature is covered by the recently > updated `DataDescriptorSignatureMissing` test. That test is more complete, > includes more documentation and uses a programmatically generated test vector. > > One might argue that 'more tests are always better', but in this case I think > the 21 year old `NoExtensionSignature` test with its binary test vector is > nebulous and requires extensive analysis to understand, more so to update. I > think it does not carry its weight and should be retired. > > Careful analysis of the deleted `test.jar` test vector revealed that it > contains a local header with non-zero `compressed size` and `uncompressed > size` fields for a streaming-mode entry. `APPNOTE.TXT` mandates that when bit > 3 of the general purpose bit flag is set, then these fields and the `crc` > field should all be set to zero. > > By injecting assertions into `ZipInputStream.readLOC` I was able to determine > that `NoExtensionSignature` is the only test currently parsing a ZIP file > with such non-zero fields in streaming mode. > > Because of this, and out of caution, this PR introduces a new test > `DataDescriptorIgnoreCrcAndSizeFields` which explicitly verifies that > `ZipInputStream` does not read any non-zero `crc`, `compressed size` and > `uncompressed size` field values from a local header when in streaming mode. > `ZipInputStream` should ignore these values and it would be good to have a > test which asserts that this invariant holds even when the fields are > non-zero. For completeness, here is a trace of the deleted `test.zip`: -- Local File Header -- 00 signature 0x04034b50 04 version20 06 flags 0x0008 08 method 8 Deflated 10 time 0x9a45 19:18:10 12 date 0x2c47 2002-02-07 14 crc0x 18 csize 2 22 size 2 26 nlen 8 28 elen 0 30 name 8 bytes'test.txt' -- File Data -- 38 data 4 bytes -- Data Descriptor -- 42 crc0xd8932aac 46 csize 4 50 size 2 -- Central Directory File Header -- 54 signature 0x02014b50 58 made by version788 60 extract version20 62 flags 0x0008 64 method 8 Deflated 66 time 0x9a45 19:18:10 68 date 0x2c47 2002-02-07 70 crc0xd8932aac 74 csize 4 78 size 2 82 diskstart 0 84 nlen 8 86 elen 0 88 clen 0 90 iattr 0x00 92 eattr 0x81b6 96 loc offset 0 000100 name 8 bytes'test.txt' -- End of Central Directory -- 000108 signature 0x06054b50 000112 this disk 0 000114 cen disk 0 000116 entries disk 1 000118 entries total 1 000120 cen size 54 000124 cen offset 54 000128 clen 0 - PR Comment: https://git.openjdk.org/jdk/pull/16975#issuecomment-1845780545
RFR: 8321396: Retire test/jdk/java/util/zip/NoExtensionSignature.java
Please review this PR which suggests we retire the ZIP test `NoExtensionSignature` along with its `test.jar` test vector. The concern of a missing data descriptor signature is covered by the recently updated `DataDescriptorSignatureMissing` test. That test is more complete, includes more documentation and uses a programmatically generated test vector. One might argue that 'more tests are always better', but in this case I think the 21 year old `NoExtensionSignature` test with its binary test vector is nebulous and requires extensive analysis to understand, more so to update. I think it does not carry its weight and should be retired. Careful analysis of the deleted `test.jar` test vector revealed that it contains a local header with non-zero `compressed size` and `uncompressed size` fields for a streaming-mode entry. `APPNOTE.TXT` mandates that when bit 3 of the general purpose bit flag is set, then these fields and the `crc` field should all be set to zero. By injecting assertions into `ZipInputStream.readLOC` I was able to determine that `NoExtensionSignature` is the only test currently parsing a ZIP file with such non-zero fields in streaming mode. Because of this, and out of caution, this PR introduces a new test `DataDescriptorIgnoreCrcAndSizeFields` which explicitly verifies that `ZipInputStream` does not read any non-zero `crc`, `compressed size` and `uncompressed size` field values from a local header when in streaming mode. `ZipInputStream` should ignore these values and it would be good to have a test which asserts that this invariant holds even when the fields are non-zero. - Commit messages: - Rename 'nameAndContent' parameter to 'expected' - Retire the test NoExtensionSignature in favor of DataDescriptorSignatureMissing. Introduce the new test DataDescriptorIgnoreCrcAndSizeFields covering unrelated aspects implicitly tested by NoExtensionSignature. Changes: https://git.openjdk.org/jdk/pull/16975/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16975=00 Issue: https://bugs.openjdk.org/browse/JDK-8321396 Stats: 222 lines in 3 files changed: 180 ins; 42 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16975.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16975/head:pull/16975 PR: https://git.openjdk.org/jdk/pull/16975
RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes
Please consider this PR which suggests we rename `ZipEntry.extraAttributes` to `ZipEntry.externalAttributes`. This field was introduced in [JDK-8218021](https://bugs.openjdk.org/browse/JDK-8218021), originally under the name `ZipEntry.posixPerms`. [JDK-8250968](https://bugs.openjdk.org/browse/JDK-8250968) later renamed the field to `ZipEntry.extraAttributes` and extended its semantics to hold the full two-byte value of the `external file attributes` field, as defined by `APPNOTE.TXT` The name `extraAttributes` is misleading. It has nothing to do with the `extra field` (an unrelated structure defined in `APPNOTE.TXT`), although the name indicates it does. To prevent confusion and make life easier for future maintainers, I suggest we rename this field to `ZipEntry.externalAttributes` and update related methods, parameters and comments accordingly. While this change is a straightforward renaming, reviewers should consider whether it carries its weight, especially considering it might complicate future backports. As a note to reviewers, this PR includes the following intended updates: - Rename `ZipEntry.extraAttributes` and any references to this field to `ZipEntry.externalAttributes` - Update `JavaUtilZipFileAccess` to similarly rename methods to `setExternalAttributes` and `getExternalAttributes` - Rename the field `JarSigner.extraAttrsDetected` to `JarSigner.externalAttrsDetected` - Rename a local variable in `JarSigner.writeEntry` to `externalAttrs` - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to `externalAttrsDetected` - Rename resource string key names in `s.s.t.jarsigner.Resources` from `extra.attributes.detected` to `external.attributes.detected` - Rename method `SymlinkTest.verifyExtraAttrs` to `verifyExternalAttrs`, also updated two references to 'extra attributes' in this method - Updated copyright in all affected files If the resource file changes should be dropped and instead handled via `msgdop` updates, let me know and I can revert the non-default files. I did a search across the code base to find 'extraAttrs', 'extra.attr' after these updates, and found nothing related to zip/jar. The `zip` and `jar` tests run clean: make test TEST="test/jdk/java/util/jar" make test TEST="test/jdk/java/util/zip" - Commit messages: - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes Changes: https://git.openjdk.org/jdk/pull/16952/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16952=00 Issue: https://bugs.openjdk.org/browse/JDK-8321274 Stats: 34 lines in 11 files changed: 0 ins; 0 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/16952.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16952/head:pull/16952 PR: https://git.openjdk.org/jdk/pull/16952
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v8]
On Wed, 15 Nov 2023 20:10:53 GMT, Eirik Bjorsnos wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field. This brings >> ZipInputStream into alignment with the APPNOTE format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Extract ZIP64_BLOCK_SIZE_OFFSET as a constant Thanks for your patient and thorough review of this long-lived PR, Lance! No worries, we can hold off the integration until after 22 is forked off mainline. - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1830464397
Re: RFR: 8316141: Improve CEN header validation checking
On Wed, 8 Nov 2023 19:59:34 GMT, Lance Andersen wrote: > Please review this PR which enhances the existing CEN header validation > checking to ensure that the > size of the CEN Header + name length + comment length + extra length do not > exceed 65,535 bytes per the PKWare APP.NOTE 4.4.10, 4.4.11, & 4.4.12. Also > check that current CEN header will not exceed the length of the CEN array. > > Mach 5 tiers 1-3 are clean with this change. While investigating an unrelated issue, I noticed that Android's `zipalign` tool processes zip files and injects data into the extra field to make the beginning of the file data be word-aligned or page-aligned: - PR Comment: https://git.openjdk.org/jdk/pull/16570#issuecomment-1835864018
Re: RFR: 8316141: Improve CEN header validation checking
On Tue, 28 Nov 2023 20:41:21 GMT, Alan Bateman wrote: > Doing it early in JDK 23 to allow time for course correction if needed seems > a good plan. Another benefit is that if we should decide to validate LOC headers similarly in `ZipInputStream`, delaying until 23 will allow us to introduce these very similar changes in the same release. - PR Comment: https://git.openjdk.org/jdk/pull/16570#issuecomment-1830715882
RFR: 8320712: Rewrite BadFactoryTest in pure Java
Please review this PR which rewrites the BadFactoryTest to pure Java/JUnit. The test is currently implemented using a mix of shell script and a Java main method. Reviewers may notice the following changes: - The shell script file `BadFactoryTest.sh` has been retired and jtreg tags moved over to `BadFactoryTest.java` - The main method has been replaced with a JUnit `@Test` method - The service definition file used to be packaged into a jar file, now the source directory is instead directly added to the classpath using `@library /javax/script/JDK_8196959` - The `@summary` tag was updated since the existing summary was slightly confusing - To make jtreg happy, the SecurityManager-enabled invocation now runs with `-Djava.security.manager=allow` instead of just `-Djava.security.manager` - A sanity check was added to verify that `ScriptEngineManager` can actually find and load `BadFactory`. Such a check would have detected the issue which inspired this rewrite, see #16585. Verified by running: % make test TEST="test/jdk/javax/script/JDK_8196959" [..] TEST TOTAL PASS FAIL ERROR jtreg:test/jdk/javax/script/JDK_8196959 1 1 0 0 Note that the original issue JDK-8196959 is not available in JBS, so my understanding of what the original test does is based on code inspection. - Commit messages: - Add the Java rewrite issue to the @bug list - Merge branch 'master' into badfactory-java-rewritte - Move jtreg tags from before to after imports - Merge branch 'master' into badfactory-java-rewritte - Update the @summary to describe the purpose of the test, not the BadFactory - Add comment describing the initialization of ScriptEngineManager - Implement BadFactoryTest in pure Java Changes: https://git.openjdk.org/jdk/pull/16830/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16830=00 Issue: https://bugs.openjdk.org/browse/JDK-8320712 Stats: 87 lines in 2 files changed: 25 ins; 60 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16830.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16830/head:pull/16830 PR: https://git.openjdk.org/jdk/pull/16830
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Mon, 27 Nov 2023 16:45:12 GMT, Gaurav Chaudhari wrote: > You can go ahead and contribute the PR in that case. Thanks! You'll need a sponsor to integrate this PR, then I'll go ahead and follow up with #16830 - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1829551620
Integrated: 8320781: Fix whitespace in j.l.Double and j.u.z.ZipInputStream @snippets
On Mon, 27 Nov 2023 16:17:14 GMT, Eirik Bjorsnos wrote: > Please review this trivial, formatting and documentation-only change which > adds missing whitespace around a few `if` statements, `while` statements and > assigments in `@snippet` code in `j.l.Double` and `j.u.z.ZipInputStream`. > > While there are many similar issues in the OpenJDK code base, these were the > only instances found in `@snippet` code in the API of `java.base`, so it > could be worthwhile fixing. > > While updating the snippet in `ZipInputStream`, I took the liberty to also > add `lang = "java"` since this seems more commonly included around the code > base. This pull request has now been integrated. Changeset: 99f870c4 Author:Eirik Bjorsnos Committer: Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/99f870c43fea4e31a63240733ab9a471469f282b Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod 8320781: Fix whitespace in j.l.Double and j.u.z.ZipInputStream @snippets Reviewed-by: lancea, bpb, darcy, jpai - PR: https://git.openjdk.org/jdk/pull/16827
Re: RFR: 8320781: Fix whitespace in j.l.Double and j.u.z.ZipInputStream @snippets
On Tue, 28 Nov 2023 10:00:14 GMT, Jaikiran Pai wrote: > Looks good to me. Please add a "noreg-doc" label to the JDK-8320781 issue. Thanks, label added! I'll need a sponsor for the integration of this change. - PR Comment: https://git.openjdk.org/jdk/pull/16827#issuecomment-1829499864
RFR: 8320781: Fix whitespace in j.l.Double and j.u.z.ZipInputStream @snippets
Please review this trivial, formatting and documentation-only change which adds missing whitespace around a few `if` statements, `while` statements and assigments in `@snippet` code in `j.l.Double` and `j.u.z.ZipInputStream`. While there are many similar issues in the OpenJDK code base, these were the only instances found in `@snippet` code in the API of `java.base`, so it could be worthwhile fixing. While updating the snippet in `ZipInputStream`, I took the liberty to also add `lang = "java"` since this seems more commonly included around the code base. - Commit messages: - Correct whitespace around if, while and assignments in @snippet code in java.base Changes: https://git.openjdk.org/jdk/pull/16827/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16827=00 Issue: https://bugs.openjdk.org/browse/JDK-8320781 Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16827.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16827/head:pull/16827 PR: https://git.openjdk.org/jdk/pull/16827
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Fri, 24 Nov 2023 16:46:05 GMT, Alan Bateman wrote: > The change here is trivial, it's okay to integrate and use a separate > issue/PR to replace the shell test. Fair point, I filed https://bugs.openjdk.org/browse/JDK-8320712 to track the rewrite. @Deigue, would you like to contribute a PR for the rewrite as well? If not, I'll be happy to take on this task. - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1826374488
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Wed, 22 Nov 2023 18:01:21 GMT, Eirik Bjorsnos wrote: >> @eirbjo Yes, as you noticed, the jar file does matter. And the reason I >> suspected it wasn't noticed was because it was in the scenario of running >> without security manager, So may that part of the code wasn't being actively >> executed. >> >> As for the rewrite, it does look good. But would it make more sense to bring >> this change as a separate PR having a own openjdk bug issue # designated to >> reworking of BadFactoryTest.sh for tracking purposes? > >> As for the rewrite, it does look good. But would it make more sense to bring >> this change as a separate PR having a own openjdk bug issue # designated to >> reworking of BadFactoryTest.sh for tracking purposes? > > We have two options: > > - Withdraw this PR, submit a new PR for the rewrite > - Repurpose this PR for the rewrite, updating JBS and PR titles accordingly > > In either case, I can help updating/creating JBS isssues as required. > > I have a slight preference for repurposing, but it's up to you. What do you > prefer? > @eirbjo If it makes sense from perspective of commiters/reviewers, I'll be > happy to integrate the changes here and tweak so that it looks good. Reviewer time is a scarce resource. It would be wasteful to spend review cycles on getting a fix of this `.sh` test integrated now and then immediately follow up with a delete in the rewrite PR. I think we should handle this change in one PR, not two. If you prefer to keep the history of your `.sh` fix documented, we can repurpose this PR, otherwise we start fresh with a new PR for the rewrite. > I also wonder if we have BadFactoryTest.java instead of BadFactoryTest.sh , > will certain references elsewhere need to be adjusted accordingly, in case > these jtreg tests are being referenced in a certain way somewhere. jtreg will pick this up automatically. I did a search of `BadFactoryTest` across the code base, and it is not referenced outside the file itself. - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1823271462
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh [v2]
On Wed, 22 Nov 2023 17:53:33 GMT, Gaurav Chaudhari wrote: > As for the rewrite, it does look good. But would it make more sense to bring > this change as a separate PR having a own openjdk bug issue # designated to > reworking of BadFactoryTest.sh for tracking purposes? We have two options: - Withdraw this PR, submit a new PR for the rewrite - Repurpose this PR for the rewrite, updating JBS and PR titles accordingly In either case, I can help updating/creating JBS isssues as required. I have a slight preference for repurposing, but it's up to you. What do you prefer? - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1823238308
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Tue, 21 Nov 2023 19:59:16 GMT, Gaurav Chaudhari wrote:
>> Looks okay. This test is begging to be re-written in Java, maybe some day.
>>
>> I assume the copyright header will be updated before this change is
>> integrated.
>
>> Looks okay. This test is begging to be re-written in Java, maybe some day.
>>
>> I assume the copyright header will be updated before this change is
>> integrated.
>
> Hi @AlanBateman, do I have to update the copyright year to 2023 manually and
> amend the commit before `/integrate` ?
@Deigue
I was able to update `BadFactoryTest.java` to work as a pure Java test by
adding the following jtreg header before the imports:
/*
* @test
* @bug 8196959
* @summary BadFactory that results in NPE being thrown from ScriptEngineManager
* @library /javax/script/JDK_8196959
* @build BadFactory BadFactoryTest
* @run main/othervm BadFactoryTest
* @run main/othervm -Djava.security.manager=allow BadFactoryTest
*/
I think we should add some code in the main method which validates that the
BadFactory ScriptEngineFactory is loaded. (This validation would have caught
the jar file typo):
ScriptEngineManager m = new ScriptEngineManager();
m.getEngineFactories().stream()
.filter(c -> c.getClass() == BadFactory.class)
.findAny()
.orElseThrow(() -> new IllegalStateException("Expected to find
BadFactory"));
Would you like to include these changes in your PR? If not, I'm happy to create
a separate PR to convert this test to Java.
-
PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1822599642
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Tue, 21 Nov 2023 21:16:41 GMT, Eirik Bjorsnos wrote:
> So it's not clear to me why this test uses a jar file at all, it does not
> seem necessary.
On a closer look, `${TESTCLASSES}` contains the compiled java classes, but not
the service definition file
`META-INF/services/javax.script.ScriptEngineFactory`. That probably explains
the jar file.
Adding `${TESTSRC}` to the class paths would incude the service definition file:
-classpath "${TESTCLASSES}${PS}${TESTSRC}"
But perhaps not a big improvement. Might be better to spend efforts re-writing
this in Java, as suggested by Alan.
-
PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1822463159
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Thu, 9 Nov 2023 16:49:41 GMT, Gaurav Chaudhari wrote:
> The file test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh contains a typo.
> When running without security manager, the test references 'badfactoty.jar'
> instead of 'badfactory.jar'. This change addresses this by correcting the jar
> name.
The typo you found here reveals that the test runs fine without having the jar
file on the classpath. All the necessary class files already seem to be in
`${TESTCLASSES}`
So it's not clear to me why this test uses a jar file at all, it does not seem
necessary.
Perhaps you could clean this up in this PR? That would include:
- Removing the code which creates the JAR
- Updating the two `-classpath` options to just `"${TESTCLASSES}"`
If you feel this is out of scope for this PR, I'm fine with that as well.
-
PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1821701137
Re: RFR: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
On Tue, 21 Nov 2023 16:36:26 GMT, Alan Bateman wrote: >> The file test/jdk/javax/script/JDK_8196959/BadFactoryTest.sh contains a >> typo. When running without security manager, the test references >> 'badfactoty.jar' instead of 'badfactory.jar'. This change addresses this by >> correcting the jar name. > > Looks okay. This test is begging to be re-written in Java, maybe some day. > > I assume the copyright header will be updated before this change is > integrated. > Hi @AlanBateman, do I have to update the copyright year to 2023 manually and > amend the commit before `/integrate` ? @Deigue Not sure if this was a question on the formatting of the copyright header, but the year should not simply be updated to `2023`, but instead to the range `2018, 2023`. So in your case, the first line should read: # Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved. Once you push that commit, a reviewer can approve the PR and you can `/integrate` - PR Comment: https://git.openjdk.org/jdk/pull/16585#issuecomment-1821612336
Re: RFR: 8316141: Improve CEN header validation checking
On Thu, 9 Nov 2023 17:22:39 GMT, Lance Andersen wrote: > Regarding you comment about checking whether or not to check if the combined > length of the CEN header + name length + comment length + extra length > 65K > bytes, I chose to add this given the strong wording given this is a really > old spec. That being said, I do not object to removing the validation if that > is the overall preference. I can't claim to have a particularly strong opinion on this, the following is mostly me thinking aloud: - Given Hyrum's Law, it is conceivable that someone is currently using the extra or comment fields to attach up to 65535+65535 bytes of metadata for entires. The proposed validation will break such schemes. Does Oracle have a ZIP file corpus which could be used to identify files currently exceeding the combined length clause, just to get a sense of how rare or common this is? - The actual benefits of adding this validation after all these years is not quite clear to me. I don't see how this improves security, robustness, compatibility, maintainability or other ilities (apart from strictly-following-the-spec-ility :-) - I created a ZIP file with an entry with an extra field of the maximal expressable length of 0x. Info-ZIP's `unzip` command on MacOS did not output any warning or error when processing this file. - PR Comment: https://git.openjdk.org/jdk/pull/16570#issuecomment-1815293978
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v6]
On Tue, 14 Nov 2023 11:49:11 GMT, Lance Andersen wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add a @bug reference in the test
>
> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 581:
>
>> 579: if ((flag & 8) == 8) {
>> 580: /* "Data Descriptor" present */
>> 581: if (hasZip64Extra(e) ||
>
> You probably want to consider updating `readLOC` to make sure the extralen is
> != 0 if the appropriate fields are set to either 0x or 0x or
> update `hasZip64Extra` to do the validation
I think I prefer keeping this PR maintaining a strict focus on expanding the
set of readable files to include those that use Zip64 extra fields for < 2GB
entries with data descriptors.
Would you be ok with that?
Adding validation to `readLOC` is a fair effort, but I would prefer this to be
done in a separate PR, similar to your work on adding Zip64 validation to
ZipFile.
I wouldn't mind looking into that, but perhaps you would like to handle it,
given your comment above about spending some time on `ZipInputStream` in the
following days?
> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 689:
>
>> 687: return switch (blockSize) {
>> 688: case 8, 16 -> true;
>> 689: default -> false;
>
> Also from 4.5.3:
>
>> This entry in the Local header MUST include BOTH original and compressed
>> file size fields
>
> So I believe the minimum value is 16 given both fields must be present
So it seems 16 is the only valid size for a Zip64 block in a LOC header.
Updated the code to reflect that and also added a comment referencing that
original and uncompressed sizes are both required in a LOC header.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1394742755
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1394743998
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v6]
On Mon, 13 Nov 2023 19:02:28 GMT, Lance Andersen wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add a @bug reference in the test
>
> test/jdk/java/util/zip/ZipInputStream/Zip64DataDescriptor.java line 57:
>
>> 55: public void setup() {
>> 56: /*
>> 57: * Structure of the ZIP64 file used below . Note the precense
>
> typo: **precense**
Thanks, fixed.
> test/jdk/java/util/zip/ZipInputStream/Zip64DataDescriptor.java line 63:
>
>> 61: * The file was produced using the following command:
>> 62: * echo hello | zip -fd > hello.zip
>> 63: *
>
> Please document which zip command(and options) is being used by the above
Documented the zip command used (zip 3.0, by Info-ZIP), the use of
stdin/streaming to enable Zip64, and the use of -fd to force the use of data
descriptors.
> test/jdk/java/util/zip/ZipInputStream/Zip64DataDescriptor.java line 149:
>
>> 147: */
>> 148: private void setExtraSize(short size) {
>> 149: int extSizeOffset = 33;
>
> I would suggest making this a constant. Either way I would like to have a
> comment added indicating that the value represents of offset of the extra
> length size in the LOC Header for `zip64File` used by the test
Extracted the constant `ZIP64_BLOCK_SIZE_OFFSET` with a comment
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1394736922
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1394738124
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1394737247
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v8]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field. This brings ZipInputStream > into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Extract ZIP64_BLOCK_SIZE_OFFSET as a constant - Changes: - all: https://git.openjdk.org/jdk/pull/12524/files - new: https://git.openjdk.org/jdk/pull/12524/files/a77147bc..b6d0a0ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12524=07 - incr: https://webrevs.openjdk.org/?repo=jdk=12524=06-07 Stats: 5 lines in 1 file changed: 3 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v7]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field. This brings ZipInputStream > into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjorsnos has updated the pull request incrementally with three additional commits since the last revision: - A Zip64 extra field used in a LOC header must include both the uncompressed and compressed size fields, and does not include local header offset or disk start number fields. Conequently, a valid LOC Zip64 block must always be 16 bytes long. - Document better the zip command and options used to generate the test vector ZIP - Fix spelling of "presence" - Changes: - all: https://git.openjdk.org/jdk/pull/12524/files - new: https://git.openjdk.org/jdk/pull/12524/files/7601b8dd..a77147bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12524=06 - incr: https://webrevs.openjdk.org/?repo=jdk=12524=05-06 Stats: 15 lines in 2 files changed: 8 ins; 2 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v11]
> Please review this PR which speeds up TestTooManyEntries and clarifies its > purpose: > > - The name 'TestTooManyEntries' does not clearly convey the purpose of the > test. What is tested is the validation that the total CEN size fits in a Java > byte array. Suggested rename: CenSizeTooLarge > - The test creates DEFLATED entries which incurs zlib costs and File Data / > Data Descriptors for no additional benefit. We can use STORED instead. > - By creating a single LocalDateTime and setting it with > `ZipEntry.setTimeLocal`, we can avoid repeated time zone calculations. > - The name of entries is generated by calling UUID.randomUUID, we could use > simple counter instead. > - The produced file is unnecessarily large. We know how large a CEN entry is, > let's take advantage of that to create a file with the minimal size. > - By adding a maximally large extra field to the CEN entries, we get away > with fewer CEN records and save memory > - The summary and comments of the test can be improved to help explain the > purpose of the test and how we reach the limit being tested. > - By writing sparse 'holes' until the last CEN entry, we can reduce required > disk space. > > These speedups reduced the runtime from 4 min 17 sec to 3 seconds on my > Macbook Pro. The produced ZIP size was reduced from 5.7 GB to ~4K. Memory > consumption is down from 8GB to something like 12MB. Eirik Bjorsnos has updated the pull request incrementally with two additional commits since the last revision: - Fix spelling of LocalDateTime - Add comment to the SparseOutputStream class noting that instances must be passed directly to the ZipOutputStream constructor, without buffering. - Changes: - all: https://git.openjdk.org/jdk/pull/12991/files - new: https://git.openjdk.org/jdk/pull/12991/files/c3d0069f..16a3c733 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12991=10 - incr: https://webrevs.openjdk.org/?repo=jdk=12991=09-10 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12991.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12991/head:pull/12991 PR: https://git.openjdk.org/jdk/pull/12991
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v10]
On Wed, 15 Nov 2023 07:41:32 GMT, Jaikiran Pai wrote:
> Overall, this is a very good improvement to the test and looks good to me. I
> just a have a trivial comment about a typo in a code comment, which I've
> added inline.
Thanks for your review, Jaikiran! With these latest, comment-only changes, this
PR is now looking for a sponsor.
> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 78:
>
>> 76: public static final int NAME_LENGTH = 10;
>> 77:
>> 78: // Use a shared LocalDataTime on all entries to save processing time
>
> Hello Eirik, there appears to be a typo in the comment here. It should have
> been `LocalDateTime`.
Fixed
> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 204:
>
>> 202: channel.position(position);
>> 203: // Check for last CEN record
>> 204: if (Arrays.equals(LAST_CEN_COMMENT_BYTES, 0,
>> LAST_CEN_COMMENT_BYTES.length, b, off, len)) {
>
> The way the instance of a `SparseOutputStream` is used in this test, it gets
> passed to the constructor of `ZipOutputStream`. That then means that there is
> no buffering involved when bytes are written out to the output stream
> internally by `ZipOutputStream`. The implementation of `ZipOutputStream`
> writes out the `ZipEntry`'s comment (if any) in one single `write(byte[])`
> call on the outputstream, so it's guaranteed that the `byte[] b` passed in
> here will actually have the entire comment (from `off` to `len`). So this
> `Arrays.equals(...)` check is then guaranteed to pass (when that specific
> entry does get written out). So this check looks good to me.
Thanks, this was a nice observation. I added a short comment to the
SparseOutputStream noting the instances should be passed directly, without any
buffering.
-
PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1813123375
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1394673748
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1394674564
Integrated: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test
On Thu, 9 Mar 2023 19:53:44 GMT, Eirik Bjorsnos wrote: > Please review this PR which brings the DataDescriptorSignatureMissing test > back to life. > > This test currently calls out to Python to create a test vector ZIP with a > Data Descriptor without the recommended but optional signature. The Python > dependency has turned out to be very brittle, so the test is currently marked > with `@ignore` > > The PR replaces Python callouts with directly creating the test vector ZIP in > the test itself. We can then remove the `@ignore`tag and run this useful test > automatically. This pull request has now been integrated. Changeset: 07eaea8c Author:Eirik Bjorsnos Committer: Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/07eaea8c25bae6ed852685f082f8b50c5b20c1a9 Stats: 158 lines in 1 file changed: 52 ins; 38 del; 68 mod 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test Co-authored-by: Jaikiran Pai Reviewed-by: jpai, lancea, iris - PR: https://git.openjdk.org/jdk/pull/12959
Re: RFR: 8316141: Improve CEN header validation checking
On Wed, 8 Nov 2023 19:59:34 GMT, Lance Andersen wrote:
> Please review this PR which enhances the existing CEN header validation
> checking to ensure that the
> size of the CEN Header + name length + comment length + extra length do not
> exceed 65,535 bytes per the PKWare APP.NOTE 4.4.10, 4.4.11, & 4.4.12. Also
> check that current CEN header will not exceed the length of the CEN array.
>
> Mach 5 tiers 1-3 are clean with this change.
LGTM, although I'm not sure I follow the underlying motivation of this stricter
validation.
What problem is being solved here? APPNOTE.TXT uses the phrase `SHOULD NOT`.
Even if the spec is not an RFC, RFC2119 defines `SHOULD NOT` as:
```This phrase, or the phrase "NOT RECOMMENDED" mean that
there may exist valid reasons in particular circumstances when the
particular behavior is acceptable or even useful, but the full
implications should be understood and the case carefully weighed
before implementing any behavior described with this label.
I would expect our producer `ZipOutputStream` to be stricter than our consumers
in this case, honoring Postel's law. From a implementation robustness
perspective, the individual lengths are already validated, it's just the
combined clause that is now enforced in this PR.
That said, here are some comments inline:
src/java.base/share/classes/java/util/zip/ZipFile.java line 1225:
> 1223: int elen = CENEXT(cen, pos);
> 1224: int clen = CENCOM(cen, pos);
> 1225: long headerSize = (long)CENHDR + nlen + clen + elen;
Since CENHDR is 46 and nlen, clen, elen are all unsigned shorts, this sum
cannot possibly overflow an int. Is the long conversion necessary?
The specification uses the term "combined length", would it be better to name
the local variable `combinedLength` instead?
src/java.base/share/classes/java/util/zip/ZipFile.java line 1235:
> 1233:
> 1234: if (elen > 0 && !DISABLE_ZIP64_EXTRA_VALIDATION) {
> 1235: checkExtraFields(pos, entryPos + nlen, elen);
The naming of `entryPos` confused me here. The name indicates it is the
position where the CEN header starts, but we already have `pos` for that. (It
actually contains the position where the encoded name starts)
So perhaps it should be renamed to `namePos` or `npos` to make future
maintenance less confusing?
Also, I actually liked the `extraStartingOffset` local variable, having a name
makes the code easier to follow than just `entryPos + nlen`. But perhaps
`extraPos` is shorter and more consistent with other uses of `pos`?
So perhaps: `long extraPos = pos + CENHDR + nlen` ?
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1596:
> 1594: throw new ZipException("invalid CEN header (unsupported
> compression method: " + method + ")");
> 1595: }
> 1596: long headerSize = (long)CENHDR + nlen + clen + elen;
If the corresponding ZipFile local variable is renamed, this should also be
updated.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1668:
> 1666: int tagBlockSize = SH(cen, currentOffset);
> 1667: currentOffset += Short.BYTES;
> 1668: long tagBlockEndingOffset = (long)currentOffset +
> tagBlockSize;
I think my ZipFile comment also applies here.
-
PR Review: https://git.openjdk.org/jdk/pull/16570#pullrequestreview-1721212606
PR Review Comment: https://git.openjdk.org/jdk/pull/16570#discussion_r1387163396
PR Review Comment: https://git.openjdk.org/jdk/pull/16570#discussion_r1387177874
PR Review Comment: https://git.openjdk.org/jdk/pull/16570#discussion_r1387194439
PR Review Comment: https://git.openjdk.org/jdk/pull/16570#discussion_r1387195085
Re: RFR: 8316141: Improve CEN header validation checking
On Wed, 8 Nov 2023 19:59:34 GMT, Lance Andersen wrote: > Please review this PR which enhances the existing CEN header validation > checking to ensure that the > size of the CEN Header + name length + comment length + extra length do not > exceed 65,535 bytes per the PKWare APP.NOTE 4.4.10, 4.4.11, & 4.4.12. Also > check that current CEN header will not exceed the length of the CEN array. > > Mach 5 tiers 1-3 are clean with this change. Perhaps the PR/issue title could be more specific in describing what is being validated? Something like "Validate the combined length of CEN header fields"? - PR Comment: https://git.openjdk.org/jdk/pull/16570#issuecomment-1802596701
Re: RFR: JDK-8295391: Add discussion of binary <-> decimal conversion issues [v3]
On Wed, 8 Nov 2023 19:24:24 GMT, Joe Darcy wrote:
>> Add discussion of some of the common pitfalls related to decimal <-> binary
>> conversion.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Switch to parseFloat from valueOf, add links.
Left some minor proofreading comments. (Not a Reviewer)
src/java.base/share/classes/java/lang/Double.java line 209:
> 207: * decimal conversion. While integer values can be exactly represented
> 208: * in any base, which fractional values can be exactly represented is
> 209: * a function of the base. For example, in base 10, 1/3 is a repeating
Could the first sentence be simplified and "de-aspectized"? Suggestion:
"..trace back to conversions between binary and decimal representations".
The second sentence has a suspicious while/which pairing and "represented is"
should probably be "represented as". Suggestion: "While integer values can be
exactly represented in any base, fractional values can only be exactly
represented as a function of the base."
src/java.base/share/classes/java/lang/Double.java line 243:
> 241: * double}. Alternatives include using an integer type and storing
> 242: * cents or mills or using {@link java.math.BigDecimal BigDecimal} to
> 243: * store decimal fraction values exactly.
The use of and/or here makes this sentence somewhat hard to parse. Perhaps a
bit of punctuation could help readability?
src/java.base/share/classes/java/lang/Double.java line 246:
> 244: *
> 245: * For each finite floating-point value and a given floating-point
> 246: * type, there is a contiguous region of the real number line which
Neither English nor floating-point are native to me, but would it be better to
use "region on the real number line" here?
A quick Google search of the alternatives gives 4 results for "region of.." and
22.000 for "region on.."
src/java.base/share/classes/java/lang/Double.java line 275:
> 273: * }
> 274: *
> 275: * An analogous range can be constructed similarly for the {@code
Suggestion:
* Similarly, an analogous range can be constructed for the {@code
src/java.base/share/classes/java/lang/Double.java line 289:
> 287: *
> 288: *
> 289: * A floating-point value doesn't "know" if it was the result of
Would it be better to use "whether" in this listing of alternatives? (As
recommended by Merriam-Webster:
https://www.merriam-webster.com/grammar/if-vs-whether-difference-usage)
src/java.base/share/classes/java/lang/Double.java line 305:
> 303: *
> 304: * should not be expected to be exactly equal to 1.0, but
> 305: * only close to 1.0. Consequently the following code is an infinite
> loop:
I think "Consequently" is a conjunctive adverb here, so should be followed by a
comma. Or perhaps just drop it, since the previous sentence also starts with
"Consequently, "
src/java.base/share/classes/java/lang/Double.java line 314:
> 312: * }
> 313: *
> 314: * Instead, for counted loops, use an integer loop count:
Suggestion:
* Instead, use integer loop counters for counted loops:
-
PR Review: https://git.openjdk.org/jdk/pull/16566#pullrequestreview-1721041731
PR Review Comment: https://git.openjdk.org/jdk/pull/16566#discussion_r1387055262
PR Review Comment: https://git.openjdk.org/jdk/pull/16566#discussion_r1387060390
PR Review Comment: https://git.openjdk.org/jdk/pull/16566#discussion_r1387063710
PR Review Comment: https://git.openjdk.org/jdk/pull/16566#discussion_r1387092563
PR Review Comment: https://git.openjdk.org/jdk/pull/16566#discussion_r1387077414
PR Review Comment: https://git.openjdk.org/jdk/pull/16566#discussion_r1387086007
PR Review Comment: https://git.openjdk.org/jdk/pull/16566#discussion_r1387097101
Re: RFR: 8314891: Additional Zip64 extra header validation [v8]
On Mon, 23 Oct 2023 19:12:57 GMT, Lance Andersen wrote: >> Please review this PR which improves the Zip64 extra header validation: >> >> - Throw a ZipException If the extra len field is 0 and : >> -- size, csize, or loc offset are set to 0x >> -- disk starting number is set to 0x >> >> - We have a valid size for the Zip64 extra header but we are missing the >> csize or loc fields if they are expected to be part of the header >> >> Mach5 tiers 1-3 are clean > > Lance Andersen has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains ten additional > commits since the last revision: > > - Merge branch 'master' into JDK-8314891 > - Add missing space > - Revamp isZip64ExtBlockSizeValid > - Merge branch 'master' into JDK-8314891 > - Merge branch 'master' into JDK-8314891 > - Remove tab(s) from comment > - Added additional tests, along with additional cleanup and refactoring > - Clean up some minor formatting issues > - Additional Zip64 extra header validation @LanceAndersen I noticed that this PR did not update `ZipInputStream.readLOC` to perform consistency validation between expected and actual extra field size and values. Any particular reason why processing of LOC headers was not made consistent with CEN? - PR Comment: https://git.openjdk.org/jdk/pull/15650#issuecomment-1802241544
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v3]
On Mon, 30 Oct 2023 14:29:41 GMT, Lance Andersen wrote: > > > I think the changes look good overall. Thank you for this. I am not sure > > > that the `@requires` is needed at this point. > > > > > > Was the `@requires (sun.arch.data.model == "64")` added to satisfy the 8GB > > memory requirement? If so, I guess we can safely remove the requires tag, > > even though the test still creates a ~2GB sparse file? > > We would have to run across all of the mach5 platforms to verify. You could > remove it for now and we go from here FWIW, this test ran successfully on `linux-x86` on Github Actions (which is 32 bit?): TEST: java/util/zip/ZipFile/CenSizeTooLarge.java build: 0.059 seconds compile: 0.059 seconds junit: 2.437 seconds TEST RESULT: Passed. Execution successful - PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1801937743
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v9]
On Mon, 30 Oct 2023 17:48:48 GMT, Eirik Bjorsnos wrote: >> Please review this PR which brings the DataDescriptorSignatureMissing test >> back to life. >> >> This test currently calls out to Python to create a test vector ZIP with a >> Data Descriptor without the recommended but optional signature. The Python >> dependency has turned out to be very brittle, so the test is currently >> marked with `@ignore` >> >> The PR replaces Python callouts with directly creating the test vector ZIP >> in the test itself. We can then remove the `@ignore`tag and run this useful >> test automatically. > > Eirik Bjorsnos has updated the pull request incrementally with three > additional commits since the last revision: > > - Describe the structure of Data Descriptors using an example > - Extend the comment of `makeZipWithSignaturelessDescriptor` with some > background info on (signature-less) data descriptors, for the benefit of > future maintainers. > - Convert test from testng to junit > > I can kick of a test run internally next week or perhaps Sunday > > > FWIW, the test ran fine on Github Actions, including on `linux-x86` (which is > 32-bit, right?): Disregard this comment, since it confuses this PR with #12991 :-) - PR Comment: https://git.openjdk.org/jdk/pull/12959#issuecomment-1801925790
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v6]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field. This brings ZipInputStream > into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Add a @bug reference in the test - Changes: - all: https://git.openjdk.org/jdk/pull/12524/files - new: https://git.openjdk.org/jdk/pull/12524/files/01e26a4f..7601b8dd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12524=05 - incr: https://webrevs.openjdk.org/?repo=jdk=12524=04-05 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v5]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field. This brings ZipInputStream > into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjorsnos has updated the pull request incrementally with two additional commits since the last revision: - Use the term "block size" when referring to the size of a Zip64 extra field data block - Update comment reflect that a Zip64 extended field in a LOC header has only two valid block sizes - Changes: - all: https://git.openjdk.org/jdk/pull/12524/files - new: https://git.openjdk.org/jdk/pull/12524/files/657f961e..01e26a4f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12524=04 - incr: https://webrevs.openjdk.org/?repo=jdk=12524=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v4]
On Wed, 8 Nov 2023 13:20:33 GMT, Eirik Bjorsnos wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field. This brings >> ZipInputStream into alignment with the APPNOTE format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > Eirik Bjorsnos has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 25 commits: > > - Convert test from testNG to JUnit > - Fix the check that the size of an extra field block size must not grow > past the total extra field length > - Move isZip64ExtBlockSizeValid back into ZipInputStream, since it is > different from the ZipFile implementation which reads the LOC > - Merge branch 'master' into data-descriptor > ># Conflicts: ># src/java.base/share/classes/java/util/zip/ZipFile.java > - Remove excessive comment > - Move isZip64ExtBlockSizeValid to ZipUtils, use it from ZipInputStream and > ZipFile.Source > - Merge branch 'master' into data-descriptor > - Use block comments instead of javadoc comments to avoid doclint warnings > - Merge branch 'master' into data-descriptor > - Zip64 extra field of a LOC header has 1-3 long components > - ... and 15 more: https://git.openjdk.org/jdk/compare/1e687b45...657f961e Observation: Unfortunately, `readLOC` skips reading the LOC's `size`, `csize` and `crc` values when in streaming mode. The fields are also overwritten by `ZipEntry.setExtra0`. This means we cannot use the original values and compare them to ZIP64_MAGICVAL when determining whether a data descriptor uses 4 or 8 byte values. - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1801900801
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v3]
On Sat, 28 Oct 2023 17:34:35 GMT, Eirik Bjorsnos wrote: > We need to hold off this PR until #15650 has been integrated as it impacts > some of the changes proposed here Marking this PR ready for review, now that #15650 has been integrated. A summary of updates after #15650: - `ZipFile.isZip64ExtBlockSizeValid` cannot be reused from `ZipInputStream` because values from the CEN are not available in streaming mode. My understanding of ZIP64 fields in a LOC is that they should contain csize and/or size, since LOC Offset and Disk Start Number are CEN-only - Converted the test to junit - Fixed an incorrect check that a Zip64 block size does not exceed the size of the total extra field - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1801887748
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v4]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field. This brings ZipInputStream > into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits: - Convert test from testNG to JUnit - Fix the check that the size of an extra field block size must not grow past the total extra field length - Move isZip64ExtBlockSizeValid back into ZipInputStream, since it is different from the ZipFile implementation which reads the LOC - Merge branch 'master' into data-descriptor # Conflicts: #src/java.base/share/classes/java/util/zip/ZipFile.java - Remove excessive comment - Move isZip64ExtBlockSizeValid to ZipUtils, use it from ZipInputStream and ZipFile.Source - Merge branch 'master' into data-descriptor - Use block comments instead of javadoc comments to avoid doclint warnings - Merge branch 'master' into data-descriptor - Zip64 extra field of a LOC header has 1-3 long components - ... and 15 more: https://git.openjdk.org/jdk/compare/1e687b45...657f961e - Changes: https://git.openjdk.org/jdk/pull/12524/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12524=03 Stats: 229 lines in 2 files changed: 228 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v9]
On Fri, 3 Nov 2023 18:43:31 GMT, Lance Andersen wrote: > I can kick of a test run internally next week or perhaps Sunday Thanks for your reviews, Lance and Iris! FWIW, the test ran fine on Github Actions, including on `linux-x86` (which is 32-bit, right?): TEST: java/util/zip/DataDescriptorSignatureMissing.java build: 0.081 seconds compile: 0.081 seconds junit: 0.055 seconds TEST RESULT: Passed. Execution successful https://github.com/eirbjo/jdk/actions/runs/6696663322/job/18196803138 - PR Comment: https://git.openjdk.org/jdk/pull/12959#issuecomment-1793024736
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v9]
On Mon, 30 Oct 2023 19:54:12 GMT, Lance Andersen wrote: > Thinking some more about this, I would like to see us keep the Zip generated > by python, store it in a byte array (or equivalent) as it also validate that > we can still process the zip given this was the original test and the zip is > being generated by a 3rd party tool. Lance, I was finally able to reproduce the original issue using Python 3.4.4. (Which was a challenge to install given its archaic dependencies!) I was able to verfy that the missing signature is the ONLY difference between the input and output files. (Except updated LOC and CEN offsets accounting for the missing bytes). Additionally, I independently removed the signature files from the input file, this produced an output file binary identical to Python's. Given that the one and only difference introduced by the Python script is covered by the test in this PR, I'm not sure I see any additional value in adding a test with the binary test vector produced by Python. I think it will just increase our maintenance costs, without adding any real value or coverage. If you see this differently, that's of course ok. Just let me know and I'll create the test with the encoded binary ZIP (which I have easily available now). Waiting for your guidance, thanks :-) - PR Comment: https://git.openjdk.org/jdk/pull/12959#issuecomment-1791298079
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v8]
On Mon, 30 Oct 2023 17:45:28 GMT, Eirik Bjorsnos wrote: >> test/jdk/java/util/zip/DataDescriptorSignatureMissing.java line 3: >> >>> 1: /* >>> 2: * Copyright 2012 Google, Inc. All Rights Reserved. >>> 3: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. >> >> I am not sure the copyright can be updated this way @irisclark, could you >> provide guidance > > This way of updating the copyright was suggested by @jaikiran in the March > 10th comment above. Would be nice to get this clarified, yes. There is actually very little left of Martin's code after my rewrite, besides whitespace, some curly braces and a couple of imports. Hardly defensible IP, but then I Am Not a Lawyer :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/12959#discussion_r1376616333
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v8]
On Mon, 30 Oct 2023 15:02:57 GMT, Lance Andersen wrote: > Another question, is the zip that is generated by this test readable by other > zip tools such as info-zip, Apache Common-compress, winzip? - info-zip: Does not support unzipping from a zip, so uses the CEN instead of the data descriptor. - Apache commons-compress, reads the signature-less ZIP just fine. - winzip: I do not currently have easy access to Windows, so can't test this. But I would assume it also uses the CEN when unzipping > test/jdk/java/util/zip/DataDescriptorSignatureMissing.java line 3: > >> 1: /* >> 2: * Copyright 2012 Google, Inc. All Rights Reserved. >> 3: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. > > I am not sure the copyright can be updated this way @irisclark, could you > provide guidance This way of updating the copyright was suggested by @jaikiran in the March 10th comment above. Would be nice to get this clarified, yes. > test/jdk/java/util/zip/DataDescriptorSignatureMissing.java line 34: > >> 32: * without data descriptors was found. >> 33: * @run testng DataDescriptorSignatureMissing >> 34: */ > > Can we convert this please to use junit Converted to junit as suggested. > test/jdk/java/util/zip/DataDescriptorSignatureMissing.java line 76: > >> 74: /** >> 75: * Produce a ZIP file where the first entry has a signature-less >> data descriptor >> 76: */ > > I think it would be useful to show the what the internal zip representation > of the LOC and CEN looks like to make it clear what a signature-less data > descriptor is meant to be for future maintainers Added a comment including some structural examples. (I personally feel it maybe ended up a bit excessive) - PR Comment: https://git.openjdk.org/jdk/pull/12959#issuecomment-1785744562 PR Review Comment: https://git.openjdk.org/jdk/pull/12959#discussion_r1376602288 PR Review Comment: https://git.openjdk.org/jdk/pull/12959#discussion_r1376602596 PR Review Comment: https://git.openjdk.org/jdk/pull/12959#discussion_r1376603460
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v9]
> Please review this PR which brings the DataDescriptorSignatureMissing test > back to life. > > This test currently calls out to Python to create a test vector ZIP with a > Data Descriptor without the recommended but optional signature. The Python > dependency has turned out to be very brittle, so the test is currently marked > with `@ignore` > > The PR replaces Python callouts with directly creating the test vector ZIP in > the test itself. We can then remove the `@ignore`tag and run this useful test > automatically. Eirik Bjorsnos has updated the pull request incrementally with three additional commits since the last revision: - Describe the structure of Data Descriptors using an example - Extend the comment of `makeZipWithSignaturelessDescriptor` with some background info on (signature-less) data descriptors, for the benefit of future maintainers. - Convert test from testng to junit - Changes: - all: https://git.openjdk.org/jdk/pull/12959/files - new: https://git.openjdk.org/jdk/pull/12959/files/de80380f..4a541ffb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12959=08 - incr: https://webrevs.openjdk.org/?repo=jdk=12959=07-08 Stats: 43 lines in 1 file changed: 34 ins; 1 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/12959.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12959/head:pull/12959 PR: https://git.openjdk.org/jdk/pull/12959
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v7]
On Mon, 30 Oct 2023 14:28:28 GMT, Lance Andersen wrote: > Please see comments below Big thanks for your review Lance! I think I have addressed your concerns now. > I would suggest a comment to help future developers. I realize there are some > earlier bread crumbs but I think this would be beneficial I added a code comment explaining here, and also replaced the `afterLastCEN` variable with a `sparse` variable, flipping the if statements accordingly. Sparse is the 'normal' in this context, so better to have actual writes in the `else` branch. - PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1785506785 PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376441175
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v10]
> Please review this PR which speeds up TestTooManyEntries and clarifies its > purpose: > > - The name 'TestTooManyEntries' does not clearly convey the purpose of the > test. What is tested is the validation that the total CEN size fits in a Java > byte array. Suggested rename: CenSizeTooLarge > - The test creates DEFLATED entries which incurs zlib costs and File Data / > Data Descriptors for no additional benefit. We can use STORED instead. > - By creating a single LocalDateTime and setting it with > `ZipEntry.setTimeLocal`, we can avoid repeated time zone calculations. > - The name of entries is generated by calling UUID.randomUUID, we could use > simple counter instead. > - The produced file is unnecessarily large. We know how large a CEN entry is, > let's take advantage of that to create a file with the minimal size. > - By adding a maximally large extra field to the CEN entries, we get away > with fewer CEN records and save memory > - The summary and comments of the test can be improved to help explain the > purpose of the test and how we reach the limit being tested. > - By writing sparse 'holes' until the last CEN entry, we can reduce required > disk space. > > These speedups reduced the runtime from 4 min 17 sec to 3 seconds on my > Macbook Pro. The produced ZIP size was reduced from 5.7 GB to ~4K. Memory > consumption is down from 8GB to something like 12MB. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Replace the 'afterLastCEN' boolean with a 'sparse' state variable - Changes: - all: https://git.openjdk.org/jdk/pull/12991/files - new: https://git.openjdk.org/jdk/pull/12991/files/8f1b0186..c3d0069f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12991=09 - incr: https://webrevs.openjdk.org/?repo=jdk=12991=08-09 Stats: 15 lines in 1 file changed: 6 ins; 4 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/12991.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12991/head:pull/12991 PR: https://git.openjdk.org/jdk/pull/12991
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v9]
> Please review this PR which speeds up TestTooManyEntries and clarifies its > purpose: > > - The name 'TestTooManyEntries' does not clearly convey the purpose of the > test. What is tested is the validation that the total CEN size fits in a Java > byte array. Suggested rename: CenSizeTooLarge > - The test creates DEFLATED entries which incurs zlib costs and File Data / > Data Descriptors for no additional benefit. We can use STORED instead. > - By creating a single LocalDateTime and setting it with > `ZipEntry.setTimeLocal`, we can avoid repeated time zone calculations. > - The name of entries is generated by calling UUID.randomUUID, we could use > simple counter instead. > - The produced file is unnecessarily large. We know how large a CEN entry is, > let's take advantage of that to create a file with the minimal size. > - By adding a maximally large extra field to the CEN entries, we get away > with fewer CEN records and save memory > - The summary and comments of the test can be improved to help explain the > purpose of the test and how we reach the limit being tested. > - By writing sparse 'holes' until the last CEN entry, we can reduce required > disk space. > > These speedups reduced the runtime from 4 min 17 sec to 3 seconds on my > Macbook Pro. The produced ZIP size was reduced from 5.7 GB to ~4K. Memory > consumption is down from 8GB to something like 12MB. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Add a code comment explaining the writing of a sparse file until the last CEN - Changes: - all: https://git.openjdk.org/jdk/pull/12991/files - new: https://git.openjdk.org/jdk/pull/12991/files/c21789b0..8f1b0186 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12991=08 - incr: https://webrevs.openjdk.org/?repo=jdk=12991=07-08 Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12991.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12991/head:pull/12991 PR: https://git.openjdk.org/jdk/pull/12991
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v7]
On Mon, 30 Oct 2023 14:10:25 GMT, Lance Andersen wrote:
>> Eirik Bjorsnos has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 14 additional
>> commits since the last revision:
>>
>> - Run CenSizeTooLarge automatically, since it no longer requires excessive
>> memory or disk space
>> - Use a SparseOutputStream to reduce required diskspace from 2GB to 4K.
>> SparseOutputStream writes 'holes' instead of actual bytes until the final
>> CEN is written.
>> - TestTooManyEntries is renamed to CenSizeTooLarge
>> - Merge branch 'master' into cen-size-too-large
>> - MAX_EXTRA_FIELD_SIZE can be better expressed as 0x
>> - Bring back '@requires sun.arch.data.model == 64' for now
>> - Spell 'specified' correctly
>> - Give test method a long, meaningful name
>> - Remove blank line
>> - Make CEN headers maximally large such that we need fewer of them and save
>> memory
>> - ... and 4 more: https://git.openjdk.org/jdk/compare/f5cb971b...9629b8d2
>
> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 33:
>
>> 31: import org.testng.annotations.BeforeTest;
>> 32: import org.testng.annotations.Test;
>> 33:
>
> As this will be a new test, could you please consider converting to junit.
- Added a comment explaining the chosen value for the constant, referencing
APPNOTE.TXT
- Converted the test to JUnit (even remembered to swap argument order of
assertEquals!)
> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 139:
>
>> 137: public void centralDirectoryTooLargeToFitInByteArray() {
>> 138: ZipException ex = expectThrows(ZipException.class, () -> new
>> ZipFile(hugeZipFile));
>> 139: assertEquals(ex.getMessage(), "invalid END header (central
>> directory size too large)");
>
> Could be have the expected message a class constant so that if this message
> ever changes it is easier to find/update
Extracted the expected ZipException message to a class constant.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376407892
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376408869
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v3]
On Mon, 30 Oct 2023 14:29:41 GMT, Lance Andersen wrote: > > Was the `@requires (sun.arch.data.model == "64")` added to satisfy the 8GB > > memory requirement? If so, I guess we can safely remove the requires tag, > > even though the test still creates a ~2GB sparse file? > > We would have to run across all of the mach5 platforms to verify. You could > remove it for now and we go from here Removed it for now, let's see what the bots say. - PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1785461403
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v8]
> Please review this PR which speeds up TestTooManyEntries and clarifies its > purpose: > > - The name 'TestTooManyEntries' does not clearly convey the purpose of the > test. What is tested is the validation that the total CEN size fits in a Java > byte array. Suggested rename: CenSizeTooLarge > - The test creates DEFLATED entries which incurs zlib costs and File Data / > Data Descriptors for no additional benefit. We can use STORED instead. > - By creating a single LocalDateTime and setting it with > `ZipEntry.setTimeLocal`, we can avoid repeated time zone calculations. > - The name of entries is generated by calling UUID.randomUUID, we could use > simple counter instead. > - The produced file is unnecessarily large. We know how large a CEN entry is, > let's take advantage of that to create a file with the minimal size. > - By adding a maximally large extra field to the CEN entries, we get away > with fewer CEN records and save memory > - The summary and comments of the test can be improved to help explain the > purpose of the test and how we reach the limit being tested. > - By writing sparse 'holes' until the last CEN entry, we can reduce required > disk space. > > These speedups reduced the runtime from 4 min 17 sec to 3 seconds on my > Macbook Pro. The produced ZIP size was reduced from 5.7 GB to ~4K. Memory > consumption is down from 8GB to something like 12MB. Eirik Bjorsnos has updated the pull request incrementally with four additional commits since the last revision: - Extract the expected ZipException message into a class constant - Add comment explaining the MAX_EXTRA_FIELD_SIZE constant - Convert test to junit - Remove @requires for sun.arch.data.model == "64", assuming the updated test doesn't need it. - Changes: - all: https://git.openjdk.org/jdk/pull/12991/files - new: https://git.openjdk.org/jdk/pull/12991/files/9629b8d2..c21789b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12991=07 - incr: https://webrevs.openjdk.org/?repo=jdk=12991=06-07 Stats: 27 lines in 1 file changed: 17 ins; 1 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/12991.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12991/head:pull/12991 PR: https://git.openjdk.org/jdk/pull/12991
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v3]
On Sun, 12 Mar 2023 11:21:18 GMT, Eirik Bjorsnos wrote: > I think the changes look good overall. Thank you for this. I am not sure that > the `@requires` is needed at this point. Was the `@requires (sun.arch.data.model == "64")` added to satisfy the 8GB memory requirement? If so, I guess we can safely remove the requires tag, even though the test still creates a ~2GB sparse file? - PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1785057458
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v3]
On Sun, 12 Mar 2023 14:12:08 GMT, Lance Andersen wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use Integer.toString instead of Long.toString
>
>> Here's a wild idea:
>>
>> We could inject large extra fields on the CEN entries to inflate the size of
>> each CEN entry. This means we get away with much fewer CEN entries which
>> again means much less memory. Also, writing mostly empty extra data seems to
>> be really fast, at least on my Macbook Pro.
>>
>> For me, the test now run without the `@requires' and completes in less than
>> 5 seconds.
>>
>> The cost is slightly more complicated code. But perhaps still worth the
>> added complexity?
>>
>> ```java
>> @BeforeTest
>> public void setup() throws IOException {
>> hugeZipFile = new File("cen-too-large.zip");
>> hugeZipFile.deleteOnExit();
>>
>> // Length of generated entry names
>> int nameLength = 10;
>>
>> // We use a large extra field to get away with fewer CEN headers
>> byte[] extra = makeLargeExtra();
>>
>> // The size of a single CEN header, including name and extra field
>> int cenHeaderSize = ZipFile.CENHDR + nameLength + extra.length;
>>
>> // Determine the number of entries needed to exceed the MAX_CEN_SIZE
>> int numEntries = (MAX_CEN_SIZE / cenHeaderSize) + 1;
>>
>> ZipEntry[] entries = new ZipEntry[numEntries];
>> try (ZipOutputStream zip = new ZipOutputStream(new
>> BufferedOutputStream(new FileOutputStream(hugeZipFile {
>> // Creating the LocalDataTime once allows faster processing
>> LocalDateTime time = LocalDateTime.now();
>> // Add entries until MAX_CEN_SIZE is reached
>> for (int i = 0; i < numEntries; i++) {
>> String name = Integer.toString(i);
>> name = "0".repeat(nameLength -name.length()) + name;
>> ZipEntry entry = entries[i] = new ZipEntry(name);
>> // Use STORED for faster processing
>> entry.setMethod(ZipEntry.STORED);
>> entry.setSize(0);
>> entry.setCrc(0);
>> entry.setTimeLocal(time);
>> zip.putNextEntry(entry);
>> }
>> zip.closeEntry();
>> for (ZipEntry entry : entries) {
>> entry.setExtra(extra);
>> }
>> }
>> }
>>
>> /**
>> * Make an extra field with an 'unknown' tag and the maximum possible data
>> size
>> * @return
>> */
>> private static byte[] makeLargeExtra() {
>> byte[] extra = new byte[Short.MAX_VALUE];
>> ByteBuffer buffer =
>> ByteBuffer.wrap(extra).order(ByteOrder.LITTLE_ENDIAN);
>> buffer.putShort((short) 0x9902); // 'unknown' tag
>> buffer.putShort((short) (extra.length - 2 * Short.BYTES)); // Data size
>> return e...
This PR was reviewed by @LanceAndersen and @Martin-Buchholz back in May, but
unfortunately did not get integrated before it was closed for inactivity.
I'm reopening it now, with a few changes after the last reviews:
- To save disk space required by the test, a SparseOutputStream writes 'holes'
until it detects that the last CEN is written, after which it starts writing
actual bytes for the END header. This reduces the required disk space from ~2GB
to ~4K.
- Since the test no longer requires excessive memory or disk space, the
'manual' tag is removed and the test is removed from the
`jdk_core_manual_no_input` group in `TEST.groups`
With a blessing of these last changes on top of what's already reviewed, I'm
hoping to integrate this PR soonish.
And sorry for the long-lived PR, I can only blame it on the unusually great
summer in the north of Norway this year :-)
-
PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1785034602
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v7]
> Please review this PR which speeds up TestTooManyEntries and clarifies its > purpose: > > - The name 'TestTooManyEntries' does not clearly convey the purpose of the > test. What is tested is the validation that the total CEN size fits in a Java > byte array. Suggested rename: CenSizeTooLarge > - The test creates DEFLATED entries which incurs zlib costs and File Data / > Data Descriptors for no additional benefit. We can use STORED instead. > - By creating a single LocalDateTime and setting it with > `ZipEntry.setTimeLocal`, we can avoid repeated time zone calculations. > - The name of entries is generated by calling UUID.randomUUID, we could use > simple counter instead. > - The produced file is unnecessarily large. We know how large a CEN entry is, > let's take advantage of that to create a file with the minimal size. > - By adding a maximally large extra field to the CEN entries, we get away > with fewer CEN records and save memory > - The summary and comments of the test can be improved to help explain the > purpose of the test and how we reach the limit being tested. > > These speedups reduced the runtime from 4 min 17 sec to 4 seconds on my > Macbook Pro. The produced ZIP size was reduced from 5.7 GB to 2 GB. Memory > consumption is down from 8GB to something like 12MB. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 14 additional commits since the last revision: - Run CenSizeTooLarge automatically, since it no longer requires excessive memory or disk space - Use a SparseOutputStream to reduce required diskspace from 2GB to 4K. SparseOutputStream writes 'holes' instead of actual bytes until the final CEN is written. - TestTooManyEntries is renamed to CenSizeTooLarge - Merge branch 'master' into cen-size-too-large - MAX_EXTRA_FIELD_SIZE can be better expressed as 0x - Bring back '@requires sun.arch.data.model == 64' for now - Spell 'specified' correctly - Give test method a long, meaningful name - Remove blank line - Make CEN headers maximally large such that we need fewer of them and save memory - ... and 4 more: https://git.openjdk.org/jdk/compare/33a18950...9629b8d2 - Changes: - all: https://git.openjdk.org/jdk/pull/12991/files - new: https://git.openjdk.org/jdk/pull/12991/files/a1a77192..9629b8d2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12991=06 - incr: https://webrevs.openjdk.org/?repo=jdk=12991=05-06 Stats: 1001917 lines in 12941 files changed: 671061 ins; 209198 del; 121658 mod Patch: https://git.openjdk.org/jdk/pull/12991.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12991/head:pull/12991 PR: https://git.openjdk.org/jdk/pull/12991
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v8]
On Sat, 28 Oct 2023 20:01:56 GMT, Eirik Bjorsnos wrote: >> Please review this PR which brings the DataDescriptorSignatureMissing test >> back to life. >> >> This test currently calls out to Python to create a test vector ZIP with a >> Data Descriptor without the recommended but optional signature. The Python >> dependency has turned out to be very brittle, so the test is currently >> marked with `@ignore` >> >> The PR replaces Python callouts with directly creating the test vector ZIP >> in the test itself. We can then remove the `@ignore`tag and run this useful >> test automatically. > > Eirik Bjorsnos has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into signature-less-data-descriptor > ># Conflicts: ># test/jdk/java/util/zip/DataDescriptorSignatureMissing.java > - The END header LOC offset field and the second entry's CEN LOC offset > fields need to be update to account for the four missing signature bytes. > - Merge branch 'master' into signature-less-data-descriptor > - Add assertNotNulls to catch unexpectedly missing entries > - Revert change to Google copyright line, add an Oracle copyright line > instead. > - Use "Signatureless" consistently > - Remove reference to python in the @summary of > DataDescriptorSignatureMissing > - Update copyright years > - Add method comments > - Instead of calling out to python, create a ZIP file and remove the data > descriptor signature. Reopening this PR. Before being closed for inactivity, this PR was reviewed by @jaikiran, who requested that another reviewer with experience in this area also take a look at it before integration. I think it would be good to have this regression test run automatic again. - PR Comment: https://git.openjdk.org/jdk/pull/12959#issuecomment-1783910651
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v8]
> Please review this PR which brings the DataDescriptorSignatureMissing test > back to life. > > This test currently calls out to Python to create a test vector ZIP with a > Data Descriptor without the recommended but optional signature. The Python > dependency has turned out to be very brittle, so the test is currently marked > with `@ignore` > > The PR replaces Python callouts with directly creating the test vector ZIP in > the test itself. We can then remove the `@ignore`tag and run this useful test > automatically. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - Merge branch 'master' into signature-less-data-descriptor # Conflicts: #test/jdk/java/util/zip/DataDescriptorSignatureMissing.java - The END header LOC offset field and the second entry's CEN LOC offset fields need to be update to account for the four missing signature bytes. - Merge branch 'master' into signature-less-data-descriptor - Add assertNotNulls to catch unexpectedly missing entries - Revert change to Google copyright line, add an Oracle copyright line instead. - Use "Signatureless" consistently - Remove reference to python in the @summary of DataDescriptorSignatureMissing - Update copyright years - Add method comments - Instead of calling out to python, create a ZIP file and remove the data descriptor signature. - Changes: https://git.openjdk.org/jdk/pull/12959/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12959=07 Stats: 134 lines in 1 file changed: 28 ins; 47 del; 59 mod Patch: https://git.openjdk.org/jdk/pull/12959.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12959/head:pull/12959 PR: https://git.openjdk.org/jdk/pull/12959
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v3]
On Sat, 28 Oct 2023 17:08:07 GMT, Eirik Bjorsnos wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field. This brings >> ZipInputStream into alignment with the APPNOTE format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > Eirik Bjorsnos has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 21 additional > commits since the last revision: > > - Remove excessive comment > - Move isZip64ExtBlockSizeValid to ZipUtils, use it from ZipInputStream and > ZipFile.Source > - Merge branch 'master' into data-descriptor > - Use block comments instead of javadoc comments to avoid doclint warnings > - Merge branch 'master' into data-descriptor > - Zip64 extra field of a LOC header has 1-3 long components > - Clarify comment for shouldIgnoreExcessiveExtraSize > - Update test to use a Zip64 file produced using the zip command with the > -fd flag > - Add comment to explaining the setExtraSize and readZipInputStream methods > and the zip64File field. > - Add comment to the call site of hasZip64 extra > - ... and 11 more: https://git.openjdk.org/jdk/compare/fb1a2716...fad0da2e > We need to hold off this PR until #15650 has been integrated as it impacts > some of the changes proposed here Thanks Lance, I updated the issue to add [JDK-8314891](https://bugs.openjdk.org/browse/JDK-8314891) as a blocker. - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1783879504
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v3]
On Sat, 28 Oct 2023 17:08:07 GMT, Eirik Bjorsnos wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field. This brings >> ZipInputStream into alignment with the APPNOTE format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > Eirik Bjorsnos has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 21 additional > commits since the last revision: > > - Remove excessive comment > - Move isZip64ExtBlockSizeValid to ZipUtils, use it from ZipInputStream and > ZipFile.Source > - Merge branch 'master' into data-descriptor > - Use block comments instead of javadoc comments to avoid doclint warnings > - Merge branch 'master' into data-descriptor > - Zip64 extra field of a LOC header has 1-3 long components > - Clarify comment for shouldIgnoreExcessiveExtraSize > - Update test to use a Zip64 file produced using the zip command with the > -fd flag > - Add comment to explaining the setExtraSize and readZipInputStream methods > and the zip64File field. > - Add comment to the call site of hasZip64 extra > - ... and 11 more: https://git.openjdk.org/jdk/compare/473ada95...fad0da2e Resuscitating this PR since the issue does seem to be causing real problems (see Tomcat Migration tooling discussion in the comments). Updates: - `ZipFile` now has `checkZip64ExtraFieldValues`. This method can be reused by `ZipInputStream` when checking the ZIP64 field lengths. To enable reuse, the method is moved to `ZipUtils` and made package protected. - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1783873510
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v3]
> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the > number of compressed or uncompressed bytes read from the inflater is larger > than the Zip64 magic value. > > While the ZIP format mandates that the data descriptor `SHOULD be stored in > ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it > also states that `ZIP64 format MAY be used regardless of the size of a file`. > For such small entries, the above assumption does not hold. > > This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the > ZipEntry includes a Zip64 extra information field. This brings ZipInputStream > into alignment with the APPNOTE format spec: > > > When extracting, if the zip64 extended information extra > field is present for the file the compressed and > uncompressed sizes will be 8 byte values. > > > While small Zip64 files with 8-byte data descriptors are not commonly found > in the wild, it is possible to create one using the Info-ZIP command line > `-fd` flag: > > `echo hello | zip -fd > hello.zip` > > The PR also adds a test verifying that such a small Zip64 file can be parsed > by ZipInputStream. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 21 additional commits since the last revision: - Remove excessive comment - Move isZip64ExtBlockSizeValid to ZipUtils, use it from ZipInputStream and ZipFile.Source - Merge branch 'master' into data-descriptor - Use block comments instead of javadoc comments to avoid doclint warnings - Merge branch 'master' into data-descriptor - Zip64 extra field of a LOC header has 1-3 long components - Clarify comment for shouldIgnoreExcessiveExtraSize - Update test to use a Zip64 file produced using the zip command with the -fd flag - Add comment to explaining the setExtraSize and readZipInputStream methods and the zip64File field. - Add comment to the call site of hasZip64 extra - ... and 11 more: https://git.openjdk.org/jdk/compare/ac50234c...fad0da2e - Changes: - all: https://git.openjdk.org/jdk/pull/12524/files - new: https://git.openjdk.org/jdk/pull/12524/files/01216ef7..fad0da2e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12524=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12524=01-02 Stats: 1001872 lines in 12942 files changed: 671014 ins; 209199 del; 121659 mod Patch: https://git.openjdk.org/jdk/pull/12524.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12524/head:pull/12524 PR: https://git.openjdk.org/jdk/pull/12524
Re: RFR: 8317742: ISO Starndard Date Format implementation consistency on DateTimeFormatter and String.format [v2]
On Thu, 12 Oct 2023 06:01:41 GMT, Shaojin Wen wrote:
>> Shaojin Wen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> No longer localize printed numbers
>
> # Summary
> Keep the behavior of String.format('%tF') when printing LocalDate consistent
> with the behavior of DateTimeFormatter.ISO_LOCAL_DATE.
>
>
> # Problem
> 1. When using String.format('%tF', localDate), use
> get(ChronoField.YEAR_OF_ERA). When the value range is not [0,], the
> behavior is inconsistent with DateTimeFormatter.ISO_LOCAL_DATE.format.
> 2. When String.format('%tF', localDate), Locale will be processed, which is
> inconsistent with the standard.
>
> * the year value range [-∞, -1] changes:
> ``` java
> String.format("%tF", LocalDate.of(-1, 1, 1))
> // String.format 10001-01-01
> // DateTimeFormatter -9-01-01
>
>
> * the year value range [1, +∞] changes:
>
> String.format("%tF", LocalDate.of(1, 1, 1)):
> // String.format 1-01-01
> // DateTimeFormatter.format +1-01-01
> And the "%tF" format no longer handles Locale when printing numbers.
>
>
>
> Locale.setDefault(
> Locale.forLanguageTag("th-TH-u-nu-thai"));
> String.format("%tF", LocalDate.of(2001, 1, 1));
> // String.format ๒๐๐๑-๐๑-๐๑
> // DateTimeFormatter.ISO_LOCAL_DATE.format 2001-01-01
>
>
> # Solution
> 1. When String.format('%tF', localDate) uses ChronoField.YEAR instead of
> ChronoField.YEAR_OF_ERA, when year > , the prefix is added with '+'
> 2. And don't use Locale to print numbers.
>
>
> # Specification
> * [-∞, -1000]
>
> String.format("%tF", LocalDate.of(-1, 1, 1))
> // -1-01-01
>
>
> The year value range [-999, -1], padded to 4 digits
>
> String.format("%tF", LocalDate.of(-1, 1, 1))
> // -0001-01-01
>
>
>
> The year value range [1, ], padded to 4 digits
>
> String.format("%tF", LocalDate.of(1, 1, 1))
> // 0001-01-01
>
>
> * The year value range [1, -∞], prefix prints '+'
>
> String.format("%tF", LocalDate.of(1, 1, 1))
> // +1-01-01
>
>
> # Risk
> 1. When String.format('%tF') processes
> LocalDate/ZonedDateTime/OffsetDateTime, if the value range of year is not in
> [0-], the output will be different from before.
> 2. When the Locale of String.format('%tF') is "th-TH-u-nu-thai", the printed
> result is different from before.
@wenshao I fixed the spelling of "standard" in the JBS issue and CSR. Can you
update the PR title as well? Thanks!
-
PR Comment: https://git.openjdk.org/jdk/pull/16033#issuecomment-1764222545
Re: RFR: 8318072: DonwcallLinker does not acquire/release segments in interpreter
On Fri, 13 Oct 2023 08:00:07 GMT, Jorn Vernee wrote: > Implement missing by-reference argument acquire/release functionality in > DowncallLinker::invokeInterpBindings. > > I've also simplified the related code a bit: > - `retIndexMap` was not used. I've removed it > - `BindingInterpreter.StoreFunc::store`'s type argument was not used. Removed > - UpcallLinker was redundantly collecting the move bindings for return > values. Removed > > I've added runs without specialization to the failing tests as well, so that > we keep testing this. > > Testing: `jdk_foreign` suite. DowncallLinker is not spelled correctly in the PR and JBS issue. Fixing this could improve searchability. - PR Comment: https://git.openjdk.org/jdk/pull/16177#issuecomment-1762026584
Re: RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v2]
On Tue, 15 Aug 2023 15:39:37 GMT, Lance Andersen wrote: >> I think I agree with Volker that it would be better if >> isZip64ExtBlockSizeValid continued to return false for block size 0. > > OK, I have made the suggest change that you both prefer. > > Thank you for your input I'm also happy to see `isZip64ExtBlockSizeValid` rejecting 0. This logic could be useful when implementing support for valid Zip64 fields for small entries in ZipInputStream, like #12524 attempted to do. (The PR was closed by the bots in the end). I guess this method could be moved to ZipUtils if JDK-8303866 is ever implemented. - PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1296113648
Re: RFR: 8309632: JDK 21 RDP1 L10n resource files update
On Mon, 12 Jun 2023 21:21:05 GMT, Justin Lu wrote: > Please review this PR which updates the JDK's localized resources since the > previous L10n translation drop (1/26). > > To help with reviewing the changes, @jonathan-gibbons created a tool which > displays the localized changes next to the original file's changes in UTF-8 > native. Those files can be viewed [here](https://cr.openjdk.org/~jlu/output/) > > For example, > > src="https://github.com/openjdk/jdk21/assets/67398801/e68a48a8-916e-4860-afb0-526924c6a3d3;> > > > Please note that the HTML files only apply to .properties, and not .java > resources. src/jdk.jartool/share/classes/sun/tools/jar/resources/jar_de.properties line 115: > 113: main.help.opt.main.validate=\ --validate Validiert den > Inhalt des JAR-Archivs. Diese Option\n validiert, > dass die von einem Multi-Release-JAR-Archiv\n > exportierte API \u00FCber die verschiedenen Releaseversionen\n > hinweg konsistent ist. > 114: main.help.opt.any=\ In jedem Modus g\u00FCltige > Vorgangsmodifikatoren:\n\n -C DIR Zum angegebenen > Verzeichnis wechseln und die folgende\n Datei > aufnehmen > 115: main.help.opt.any.file=\ -f, --file=FILEDer Name der > Archivdatei. Wenn Sie dies lauslassen, wird entweder stdin oder\n > stdout verwendet, je nach Vorgang\n --release VERSION > Speichert alle der folgenden Dateien in einem versionierten Verzeichnis\n > der JAR-Datei (d.h. META-INF/versions/VERSION/) (Review comment moved to mainline PR) - PR Review Comment: https://git.openjdk.org/jdk21/pull/11#discussion_r1227287139
Re: RFR: 8309632: JDK 21 RDP1 L10n resource files update
On Mon, 12 Jun 2023 22:00:01 GMT, Justin Lu wrote: > Please review this PR which updates the JDK's localized resources since the > previous L10n translation drop (1/26). > > To help with reviewing the changes, @jonathan-gibbons created a tool which > displays the localized changes next to the original file's changes in UTF-8 > native. Those files can be viewed [here](https://cr.openjdk.org/~jlu/output/) > > For example, > > src="https://github.com/openjdk/jdk21/assets/67398801/e68a48a8-916e-4860-afb0-526924c6a3d3;> > > > Please note that the HTML files only apply to .properties, and not .java > resources. src/jdk.jartool/share/classes/sun/tools/jar/resources/jar_de.properties line 115: > 113: main.help.opt.main.validate=\ --validate Validiert den > Inhalt des JAR-Archivs. Diese Option\n validiert, > dass die von einem Multi-Release-JAR-Archiv\n > exportierte API \u00FCber die verschiedenen Releaseversionen\n > hinweg konsistent ist. > 114: main.help.opt.any=\ In jedem Modus g\u00FCltige > Vorgangsmodifikatoren:\n\n -C DIR Zum angegebenen > Verzeichnis wechseln und die folgende\n Datei > aufnehmen > 115: main.help.opt.any.file=\ -f, --file=FILEDer Name der > Archivdatei. Wenn Sie dies lauslassen, wird entweder stdin oder\n > stdout verwendet, je nach Vorgang\n --release VERSION > Speichert alle der folgenden Dateien in einem versionierten Verzeichnis\n > der JAR-Datei (d.h. META-INF/versions/VERSION/) This change looks suspicious to me. The English word used is “omitted”. “Auslassen” is to “Leave out”, while “lauslassen” is to “let go”. A native German speaker should confirm, I have only high school level German.. - PR Review Comment: https://git.openjdk.org/jdk/pull/14430#discussion_r1227296089
Re: RFR: 8307088: Allow the jdbc.drivers system property to be searchable
On Fri, 28 Apr 2023 17:28:47 GMT, Lance Andersen wrote: > Hi all, > > Please review this trivial change which allows the` jdbc.drivers` system > property to be searchable. > > Best. > Lance Somewhat unrelated perhaps, but I found the example value given for `jdbc.drivers` in `DriverManager` javadocs to be a bit funky. `jdbc.drivers=foo.bah.Driver:wombat.sql.Driver:bad.taste.ourDriver` I know, I know, coming up with examples is hard, but perhaps we could find something more boring, realistic, package- and database-looking example drivers? Something like: `jdbc.drivers=com.company.jdbc.Driver:org.somedb.Driver:org.otherdb.JbdcDriver` I know this is very subjective, and not really related to the issue. But I thought I'd mention it now that this code is updated. - PR Comment: https://git.openjdk.org/jdk/pull/13724#issuecomment-1527952909
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v2]
On Tue, 18 Apr 2023 18:19:20 GMT, Lance Andersen wrote: > It would be useful to obtain the zip/jar in question to validate that your > proposed patch addresses the issue as well as verifying if ZipfFile can be > used to process the zip/jar as reading the long thread appears that > ZipFile:getInputStream is fine. The original input file to the Tomcat Migration tool is found here: https://repo1.maven.org/maven2/org/apache/tomcat/embed/tomcat-embed-core/10.1.7/tomcat-embed-core-10.1.7.jar I have tested reading this file using `ZipInputStream` in this PR branch and it processes the 8 byte data descriptors successfully. > Do you know how the Zip in question is being created, is it via ApacheCommons > and could there be an issue there? The file is produced in the Tomcat Ant release targets. First, the `jar` target produces the file, followed by postprocessing by `bnd` for OSGi purposes. At this point the file is still not in ZIP64 mode. Finally, the `zip` task is used to update the jar file with some Graal releated files , and this is where the ZIP64 seems to be introduced. I might want to do some more investigation on the Ant side to see if this behavior could be improved. - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1513687801
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v2]
On Tue, 18 Apr 2023 18:19:20 GMT, Lance Andersen wrote: > Do you know how the Zip in question is being created, is it via ApacheCommons > and could there be an issue there? Currently investigating. The Tomcat build is using the Ant `jar` task to create the `tomcat-embed-core.jar`, but then calls `zip` in update mode to update the jar with some Graal related files. This is where the ZIP64 extra fields get introduced. I'll take a look at testing this PR with this Tomcat jar once I have a better understanding why the `zip` task introduces ZIP64. - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1513615536
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v2]
On Wed, 29 Mar 2023 10:48:57 GMT, Eirik Bjorsnos wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field. This brings >> ZipInputStream into alignment with the APPNOTE format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Use block comments instead of javadoc comments to avoid doclint warnings Mark Thomas of Apache chimed in on the JBS to say that this limitation of ZipInputStream is affecting Tomcat migration tooling: >This bug is affecting the library Apache Tomcat uses to automatically migrate >web applications from Java EE to Jakarta EE. https://github.com/apache/tomcat-jakartaee-migration/issues/46 - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1513497317
Re: RFR: 8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message [v5]
On Mon, 17 Apr 2023 17:04:32 GMT, Alan Bateman wrote: > What you have is fine, I'm just making the point that the exception message > is somewhat secondary I guess this is also a question of testing the interface vs. testing the implementation. To get good coverage of validation scenarios, I often find it quite useful to test on the actual message. This makes the test overspecific to the interface, but it helps make sure that each error condition is covered independently. Without asserting on the actual message, it can be hard to know if you are being shadowed by another higher level error condition. I've seen this a lot when adding test coverage for various ZIP processing code. - PR Review Comment: https://git.openjdk.org/jdk/pull/13482#discussion_r1169058586
Integrated: 8306036: Use @apiNote in String.toLowerCase, String.toUpperCase
On Sat, 15 Apr 2023 07:56:17 GMT, Eirik Bjorsnos wrote: > Please review this PR which suggests to use `@apiNote` in the > `String.toLowerCase()` and `String.toUpperCase()` API docs. > > These methods include important usage notes which today are encoded using > `Note:`. This pull request has now been integrated. Changeset: 7f56de8f Author: Eirik Bjorsnos Committer: Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/7f56de8f78c0b54e5cf313f53213102a3495234f Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod 8306036: Use @apiNote in String.toLowerCase, String.toUpperCase Reviewed-by: alanb, jpai - PR: https://git.openjdk.org/jdk/pull/13487
Re: RFR: 8306036: Use @apiNote in String.toLowerCase, String.toUpperCase [v2]
On Sat, 15 Apr 2023 11:17:34 GMT, Eirik Bjorsnos wrote: >> Please review this PR which suggests to use `@apiNote` in the >> `String.toLowerCase()` and `String.toUpperCase()` API docs. >> >> These methods include important usage notes which today are encoded using >> `Note:`. > > Eirik Bjorsnos has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'master' into to-ul-case-apinote > - Use @apiNote in String.toLowerCase, String.toUpperCase Thanks for your review Alan, I have integrated as-is. Perhaps you could sponsor? - PR Comment: https://git.openjdk.org/jdk/pull/13487#issuecomment-1510197569
Re: RFR: 8306036: Use @apiNote in String.toLowerCase, String.toUpperCase [v2]
On Sat, 15 Apr 2023 19:09:34 GMT, Alan Bateman wrote: > There may be a few like this around the JDK that pre-date the apiNote tag. There are 44 matches in 12 files in `src/java.base/share/classes/java`, including classes such as `HttpURLConnection`, `Character`, `SimpleDateFormat`, `Locale` and `ResourceBundle`. Would you prefer that I expand this PR to take care of at least a few of those? Happy to do so, but it will require more reviewer cycles. (Although changes should be fairly straightforward). - PR Comment: https://git.openjdk.org/jdk/pull/13487#issuecomment-1509944762
Re: RFR: 8305461: [vectorapi] Add VectorMask::xor
On Thu, 6 Apr 2023 03:43:26 GMT, Quan Anh Mai wrote: >> Thank you for doing this. I am guessing it was always intended to be public >> but was not spotted in previous reviews. > > @PaulSandoz Thanks for your timely review, do I need a second review for this > patch? Hello @merykitty, I'm observing GHA test failure in `jdk/incubator/vector/ShortMaxVectorTests` on `linux-x86` (jdk master): test ShortMaxVectorTests.sliceShortMaxVectorTestsMasked(short[-i * 5], short[i + 1], mask[true]): failure java.lang.AssertionError: arrays differ firstly at element [16]; expected value is <12> but was <1>. (ref: [-135, -140, -145, -150, -155, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27], res: [-135, -140, -145, -150, -155, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]), at index #0, at origin #27 at org.testng.Assert.fail(Assert.java:99) at org.testng.Assert.assertEquals(Assert.java:275) at ShortMaxVectorTests.assertArraysEquals(ShortMaxVectorTests.java:854) at ShortMaxVectorTests.sliceShortMaxVectorTestsMasked(ShortMaxVectorTests.java:4812) Could this be caused by changes in this PR? See https://github.com/eirbjo/jdk/actions/runs/4707493730#user-content-jdk_incubator_vector_shortmaxvectortests - PR Comment: https://git.openjdk.org/jdk/pull/13277#issuecomment-1509840795
