Re: RFR: 8322565 (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits [v4]
On Wed, 10 Jan 2024 18:29:40 GMT, Eirik Bjørsnøs wrote: >> 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 instead of just the >> trailing 12 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`. >> This test also verifies that operations not related to POSIX, such as >> Files.setLastModifiedTime does not affect the 'external file attributes' >> value. >> >> 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) > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Convert TestPosix.java from testng to Junit 5 Thank you for the updates. Looks good to go - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17170#pullrequestreview-1814209477
Re: RFR: 8322565 (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits [v2]
On Tue, 9 Jan 2024 10:22:40 GMT, Eirik Bjørsnøs wrote:
>> 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 instead of just the
>> trailing 12 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`.
>> This test also verifies that operations not related to POSIX, such as
>> Files.setLastModifiedTime does not affect the 'external file attributes'
>> value.
>>
>> 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)
>
> Eirik Bjørsnøs 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 three additional
> commits since the last revision:
>
> - Verify that ZipFileSystem preserves 'external file attribute' bits when
> performing operations unrelated to POSIX, such as Files.setLastModifiedTime.
> - Merge branch 'master' into zipfs-preserve-external-file-attrs
> - Preserve non-permission 'external file attributes' bits when setting posix
> permissions
Thank you for the PR. Overall it looks good a few couple nit comments.
Extra credit to convert this from testng to a junit test but not a must
test/jdk/jdk/nio/zipfs/TestPosix.java line 761:
> 759: Files.write(zipFile, zip);
> 760:
> 761: // Verify that a read/synch roundtrip preserves the external
> file attributes
typo 'synch'
test/jdk/jdk/nio/zipfs/TestPosix.java line 770:
> 768:
> 769: // Verify calling Files.setPosixFilePermissions with current
> permission set
> 770: try (FileSystem fs = FileSystems.newFileSystem(zipFile,
> ENV_POSIX)) {
This should be a separate test IMHO
-
Marked as reviewed by lancea (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17170#pullrequestreview-1813747144
PR Review Comment: https://git.openjdk.org/jdk/pull/17170#discussion_r1447701846
PR Review Comment: https://git.openjdk.org/jdk/pull/17170#discussion_r1447718468
Re: RFR: 8322565 (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits
On Wed, 20 Dec 2023 18:51:08 GMT, Eirik Bjørsnøs wrote: > 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) @RealCLanger, can you add this to your list to review given your efforts to add support for Posix permission support? Eirik, I will try and make some time this week or early next for this one - PR Comment: https://git.openjdk.org/jdk/pull/17170#issuecomment-1881541762
Re: RFR: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile [v9]
On Tue, 2 Jan 2024 13:31:01 GMT, Eirik Bjørsnøs wrote: >> 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. A new test case `noentries()` >> was added, resolving >> [JDK-8322830](https://bugs.openjdk.org/browse/JDK-8322830) >> >> `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 >> ... > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Add test verifying that ZipFile can open a ZIP file with zero entries Changes look fine. Probably need to change the copyright from 2023 -> 2024 - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17038#pullrequestreview-1787585369
Re: RFR: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile [v7]
On Thu, 14 Dec 2023 14:55:08 GMT, Eirik Bjørsnøs wrote:
>> 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. A new test case `noentries()`
>> was added, resolving
>> [JDK-8322830](https://bugs.openjdk.org/browse/JDK-8322830)
>>
>> `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
>> ...
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Merge the ReadAfterClose test into ReadZip, converting it to JUnit.
test/jdk/java/util/zip/ZipFile/ReadZip.java line 233:
> 231: URI uri = URI.create("jar:" + zip.toUri());
> 232: Map env = Map.of("create", "true",
> "forceZIP64End", "true");
> 233: try (FileSystem fs = FileSystems.newFileSystem(uri, env)) {
No Need to use a URI here
test/jdk/java/util/zip/ZipFile/ReadZip.java line 243:
> 241: }
> 242: // Read using ZipFileSystem
> 243: try (FileSystem fs = FileSystems.newFileSystem(uri, Map.of())) {
Same comment as above regarding URI
test/jdk/java/util/zip/ZipFile/ReadZip.java line 249:
> 247:
> 248: /**
> 249: * Read a zip file created via "echo hello | zip dst.zip -",
This was created using info-zip so I would clarify in the comments
test/jdk/java/util/zip/ZipFile/ReadZip.java line 257:
> 255: @Test
> 256: public void readZip64EndZipProcess() throws IOException,
> InterruptedException {
> 257: if (Files.notExists(Paths.get("/usr/bin/zip"))) {
We should address this as the test won't run on Windows. It would be better to
store the zip as a byte array so that it can be processed on all platforms and
by removing ProcessBuilder, the test run will speed up a bit
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17038#discussion_r1430574058
PR Review Comment: https://git.openjdk.org/jdk/pull/17038#discussion_r1430574583
PR Review Comment: https://git.openjdk.org/jdk/pull/17038#discussion_r1430575196
PR Review Comment: https://git.openjdk.org/jdk/pull/17038#discussion_r1430578476
Re: RFR: 8322214: Return value of XMLInputFactory.getProperty() changed from boolean to String in JDK 22 early access builds
On Wed, 3 Jan 2024 19:31:16 GMT, Joe Wang wrote: > Fix the get (return) operation by using the same method as that used for > checking the values of the DTD features during the resolution process. > > Note: this patch also addresses the issue reported in > https://bugs.openjdk.org/browse/JDK-8322216 > > Test: new test added covers DTD properties for SAX/DOM/StAX. > Existing tests passed. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17252#pullrequestreview-1803102547
Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v6]
On Tue, 2 Jan 2024 17:50:51 GMT, Eirik Bjørsnøs wrote: >> Please review this test-only PR which adds test coverage for >> `ZipFile.getEntry` under certain charset conditions. >> >> When `ZipFile.getEntry` is called for an entry which has the `Language >> encoding flag` general purpose bit flag set, then `ZipCoder.UTF8` is used >> unconditionally, even when a different charset was supplied to the `ZipFile` >> constructor. >> >> It turns out we do not have any testing for this particular case, as can be >> verified by commenting out the following line of code in >> `ZipFile.Source.getEntryPos`: >> >> >> //ZipCoder zc = zipCoderForPos(pos); >> ``` >> >> and then running `make test TEST="test/jdk/java/util/zip"` >> >> The current test verifies that the correct ZipCoder is used by >> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way. >> >> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was >> (accidentally?) fixed by >> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is >> worthwhile to add explicit testing for this case to avoid regressions. >> >> While visiting `ZipCoding.java`, I took the opportunity to convert it to >> JUnit 5. The conversion and modernization of the code is done in the first >> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second >> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code >> required to verify the `Language encoding flag` condition for >> `ZipFile.getEntry`. >> >> Testing: Verified that the test indeed fails when >> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as >> suggested above. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Fine tuning assertion messages for consistency between tests Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17207#pullrequestreview-1800965421
Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v4]
On Tue, 2 Jan 2024 09:31:16 GMT, Eirik Bjørsnøs wrote: >> Please review this test-only PR which adds test coverage for >> `ZipFile.getEntry` under certain charset conditions. >> >> When `ZipFile.getEntry` is called for an entry which has the `Language >> encoding flag` general purpose bit flag set, then `ZipCoder.UTF8` is used >> unconditionally, even when a different charset was supplied to the `ZipFile` >> constructor. >> >> It turns out we do not have any testing for this particular case, as can be >> verified by commenting out the following line of code in >> `ZipFile.Source.getEntryPos`: >> >> >> //ZipCoder zc = zipCoderForPos(pos); >> ``` >> >> and then running `make test TEST="test/jdk/java/util/zip"` >> >> The current test verifies that the correct ZipCoder is used by >> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way. >> >> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was >> (accidentally?) fixed by >> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is >> worthwhile to add explicit testing for this case to avoid regressions. >> >> While visiting `ZipCoding.java`, I took the opportunity to convert it to >> JUnit 5. The conversion and modernization of the code is done in the first >> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second >> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code >> required to verify the `Language encoding flag` condition for >> `ZipFile.getEntry`. >> >> Testing: Verified that the test indeed fails when >> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as >> suggested above. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Add more cases for 'language encoding' bit set, opened with a different > encoding Looks good over all. One minor comment to consider before pushing test/jdk/java/util/zip/ZipCoding.java line 130: > 128: assertEquals(name, e.getName()); > 129: assertNull(e.getComment()); // No comment in the LOC header > 130: assertArrayEquals(ENTRY_DATA, zis.readAllBytes(), "ZipIS > content doesn't match!"); Minor Nit, but perhaps changes the "ZipIS content..." message to make it clearer as it is not what is clear what ZipIS is... - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17207#pullrequestreview-1800709897 PR Review Comment: https://git.openjdk.org/jdk/pull/17207#discussion_r1439626614
Re: RFR: 7009069: ZipFile.getEntry(String name) does NOT respect the "language encoding flag"
On Sun, 31 Dec 2023 18:07:33 GMT, Eirik Bjørsnøs wrote: > Please review this test-only PR which adds test coverage for > `ZipFile.getEntry` under certain charset conditions. > > When `ZipFile.getEntry` is called for an entry which has the `Language > encoding flag` general purpose bit flag set, then `ZipCoder.UTF8` is used > unconditionally, even when a different charset was supplied to the `ZipFile` > constructor. > > It turns out we do not have any testing for this particular case, as can be > verified by commenting out the following line of code in > `ZipFile.Source.getEntryPos`: > > > //ZipCoder zc = zipCoderForPos(pos); > ``` > > and then running `make test TEST="test/jdk/java/util/zip"` > > The current test verifies that the correct ZipCoder is used by > `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way. > > Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was > (accidentally?) fixed by > [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is > worthwhile to add explicit testing for this case to avoid regressions. > > While visiting `ZipCoding.java`, I took the opportunity to convert it to > JUnit 5. The conversion and modernization of the code is done in the first > commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second > commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code > required to verify the `Language encoding flag` condition for > `ZipFile.getEntry`. > > Testing: Verified that the test indeed fails when > `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as > suggested above. Thanks for taking this on. With the conversion, I probably would would at separating out the test runs for ZipFile and ZipInputStream and introduce a DataProvider. This will just make it a bit clearer to identify an error that might be specific to ZipFile and/or ZipInputStream.. - PR Review: https://git.openjdk.org/jdk/pull/17207#pullrequestreview-1799692059
Re: RFR: 8322078: ZipSourceCache.testKeySourceMapping() test fails with The process cannot access the file because it is being used by another process
On Wed, 20 Dec 2023 15:37:37 GMT, Sean Coffey wrote: > trivial test edit to use FileUtils.deleteFileIfExistsWithRetry test library > API > > tested on CI system with test-repeat = 20 - no issues seen. Hi Sean, Thank you for tackling this. Would it be beneficial to use a try-with-resources for the zip file opened on lines 88 and 98 just to rule out any other weirdness on windows? - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17169#pullrequestreview-1791382251
Re: RFR: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile [v7]
On Thu, 14 Dec 2023 15:00:15 GMT, Eirik Bjorsnos wrote: > 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. These are on my list to review, thank you for the update. One comment, for the tests that we are removing/retiring, we probably want to include the bug numbers and/or the name of the original test class in the new home for the tests to make it easier for future maintainers to chase back the history in the unlikely event that an issue arises... - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1857706931
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
On Thu, 14 Dec 2023 10:13:31 GMT, Eirik Bjorsnos 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? 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 - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855690222
Re: RFR: 8322027: One XMLStreamException constructor fails to initialize cause
On Wed, 13 Dec 2023 21:19:34 GMT, Joe Wang wrote: >> One of the three `XMLStreamException` constructors that takes a `Throwable` >> fails to pass it to the superclass constructor. >> >> This simple patch fixes that omission. >> >> It's worth considering if there is any code out there that is working around >> this problem already by invoking `initCause()` manually. If so, that code >> would start throwing an `IllegalStateException` after this change. >> >> So a more conservative fix would be to just add another constructor taking >> the same arguments in a different order. But then again that's not much >> better than just saying "always use initCause() with the broken >> constructor", i.e., don't change anything. >> >> Hmm. > > test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamExceptionTest/ExceptionCauseTest.java > line 38: > >> 36: * @test >> 37: * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest >> 38: * @run testng/othervm -DrunSecMngr=true -Djava.security.manager=allow >> stream.XMLStreamExceptionTest.ExceptionCauseTest > > We no longer add this run in new tests as the Security Manager has been > deprecated (https://openjdk.org/jeps/411). Well, the SM is still technically supported so until we remove it, I would leave the run with SM in - PR Review Comment: https://git.openjdk.org/jdk/pull/17090#discussion_r1425898345
Re: RFR: 8320279: Link issues in java.xml module-info.java
On Wed, 13 Dec 2023 20:54:16 GMT, Joe Wang wrote: > Doc-only change: fix incorrect links in module-info.java and StAX factories. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17093#pullrequestreview-1780488559
Re: ZipEntry
Please refer to JDK-8250968<https://bugs.openjdk.org/browse/JDK-8250968> which why the field in question was added. If you are interested in additional support, you can open an enhancement request for consideration. Best Lance On Dec 12, 2023, at 4:15 PM, Alan Snyder mailto:javali...@cbfiddle.com>> wrote: On Dec 12, 2023, at 12:57 PM, Eirik Bjørsnøs mailto:eir...@gmail.com>> wrote: Alan, ZipOutputStream is a relatively high-level API for producing ZIP files. There are many shapes and corner cases allowed by APPNOTE.TXT which ZipOutputStream has no means to produce. So while exposing the "external file attributes" field (which was incorrectly named "extraAttributes" in ZipEntry) is perhaps not harmful in itself, the question is: Where do we stop? Right now, I’m more interested in reading zip files, but to answer your question: we can stop at any point. It's worth noting that the ZipFileSystem API has the "enablePosixFileAttributes" env option which lets you set POSIX permissions on ZIP file entries using Files.setPosixFilePermissions() I’m more interested in the symlink bit, and do not currently have any need to use ZipFileSystem. I’m wondering, though, why it is OK to access information via ZipFileSystem and not directly via ZipEntry? The following OpenJDK test can serve as an example: https://github.com/openjdk/jdk/blob/master/test/jdk/sun/security/tools/jarsigner/PosixPermissionsTest.java#L151 Cheers, Eirik, On Tue, Dec 12, 2023 at 9:17 PM Alan Snyder mailto:javali...@cbfiddle.com>> wrote: ZipEntry is a public class and I am aware that it is used outside the JDK. Presumably that is not a problem. I’m wondering why the class stores the external file attributes field but does not provide public accessors for it. I would find it useful to have access to this field. I would rather not have to switch to using a third party library just for this reason. I understand that the interpretation of the external file attributes is not standardized, but the field itself is defined in the standard cited by the package info page, so I don’t see the harm in making it accessible without interpretation. Alan [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen | Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem [v2]
On Tue, 12 Dec 2023 11:59:36 GMT, Eirik Bjorsnos wrote: >> 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 Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17059#pullrequestreview-1777315017
Re: RFR: 8321802: (zipfs) Add validation of incorrect LOC signature in ZipFileSystem
On Mon, 11 Dec 2023 15:38:28 GMT, Eirik Bjorsnos wrote:
> 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.
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
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17059#discussion_r1423879142
Re: RFR: 8321616: Convert the ReadZip test to JUnit
On Mon, 11 Dec 2023 17:22:14 GMT, Eirik Bjorsnos 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? input.zip and input.jar are pretty trivial: > jar tvf test/jdk/java/util/zip/ZipFile/input.zip >506 Fri May 28 16:32:31 EDT 1999 ReadZip.java > jar tvf test/jdk/java/util/zip/ZipFile/input.jar > 0 Tue Mar 23 13:09:08 EST 1999 META-INF/ > 86 Tue Mar 23 13:09:08 EST 1999 META-INF/MANIFEST.MF >728 Tue Mar 23 13:08:30 EST 1999 ReleaseInflater.java So you could have the tests create the zip files on the fly or store the contents in an array within the tests. If the tests create the zip file/jar on the fly, then we just need to make sure that we create it in the same fashion as the test needs. I just think that we should take this opportunity as part of re-writing the tests to remove the need for the binary files in the workspace. 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 - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1850559536
Re: RFR: 8321616: Convert the ReadZip test to JUnit
On Fri, 8 Dec 2023 20:28:20 GMT, Eirik Bjorsnos wrote: > 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. Thank you for tacking the conversion of this test to junit One quick comment, if we are updating this test, we should look to get rid of input.zip which means we should also address as part of the change: open/test/jdk/java/util/zip/ZipFile/EnumAfterClose.java open/test/jdk/java/util/zip/ZipFile/FinalizeInflater.java open/test/jdk/java/util/zip/ZipFile/StreamZipEntriesTest.java - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1850472569
Integrated: 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. This pull request has now been integrated. Changeset: 0eb299af Author:Lance Andersen URL: https://git.openjdk.org/jdk/commit/0eb299af792f95d66797e2274c359190bfd5560c Stats: 335 lines in 4 files changed: 322 ins; 4 del; 9 mod 8316141: Improve CEN header validation checking Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/16570
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. 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 - PR Comment: https://git.openjdk.org/jdk/pull/16975#issuecomment-1845874650
Re: RFR: 8321406: Null IDs should be resolved as before catalogs are added
On Tue, 5 Dec 2023 19:34:58 GMT, Joe Wang wrote: > Improve null handling. When both the public and system IDs are null, this > change lets the resolution process continue as usual, before the catalogs > were added. This is consistent with the process on the other part of the > program where a CatalogResolver is used only when the IDs are not null. > > Test: existing XML tests passed. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16983#pullrequestreview-1766249605
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes
On Mon, 4 Dec 2023 15:34:34 GMT, Eirik Bjorsnos wrote: > 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" Thank you for the contribution Eirik. I will take a peek at this once we are able to push changes for JDK 23 - PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-1839198963
Re: RFR: JDK-8319626: Override toString() for ZipFile [v5]
On Fri, 1 Dec 2023 08:49:34 GMT, Alan Bateman wrote: >> Thanks Jai, that makes sense. Replaced full path with just the base name in >> latest commit. > > I think the second paragraph of the method description is problematic. > Documenting the representation and then saying it is subject to change is > going to cause problems in the future? Can you just drop this paragraph? I agree we should probably remove the 2nd paragraph - PR Review Comment: https://git.openjdk.org/jdk/pull/16643#discussion_r1412442494
Re: RFR: JDK-8320940: Fix typo in java.lang.Double
On Wed, 29 Nov 2023 02:00:14 GMT, Joe Darcy wrote: > Typo fix to to the new text added in JDK-8295391. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16872#pullrequestreview-1755023783
Re: RFR: JDK-8210410: Refactor java.util.Currency:i18n shell tests to plain java tests [v3]
On Tue, 28 Nov 2023 20:49:38 GMT, Justin Lu wrote: >> Please review this PR which converts the shell test, >> _java/util/currency/PropertiesTest.sh_ to a normal java test. >> >> This test is a test runner that launches test methods from >> _PropertiesTest.java_. It tests both the ways to supersede the default >> currencies, that is, either using a custom properties file under the lib >> directory, or setting the path to a custom properties file via the system >> property. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > retain original copyright year Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16787#pullrequestreview-1753905849
Re: RFR: 8320918: Fix errors in the built-in Catalog implementation
On Tue, 28 Nov 2023 20:11:42 GMT, Joe Wang wrote: > Fix errors in the built-in Catalog implementation, specifically: > -- incorrect case in the property name as defined in the CSR > (https://bugs.openjdk.org/browse/JDK-8306056) > -- tests needed to be updated accordingly > -- jaxp.properties updated with documentation for the new properties > > Tests: passed after the update. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16862#pullrequestreview-1753859021
Re: RFR: 8316141: Improve CEN header validation checking
On Thu, 16 Nov 2023 20:52:08 GMT, Eirik Bjorsnos 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. Yes we have a corpus search available and have exercised this patch (along with your ZipInputStream patch) without any regressions. Given where we are in the JDK 22 cycle, going to hold off on finalizing the PR until we fork for JDK 23 and look to move this forward early on allowing for additional time to bake - PR Comment: https://git.openjdk.org/jdk/pull/16570#issuecomment-1830639162
Re: RFR: 8320798: Console read line with zero out should zero out underlying buffer [v2]
On Tue, 28 Nov 2023 19:30:34 GMT, Naoto Sato wrote: >> It is best practice to zero out the underlying buffer after use. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > corrected fromIndex Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16861#pullrequestreview-1753782100
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 Thank you for your efforts here Eirik. I am OK with the proposed changes. We have completed internally our Mach5 runs and thanks to Jai, we have been able to exercise a build against a large internal repository of jars I would like to suggest that we hold off until we fork for JDK 23 to push this (as we are going to do for my outstanding CEN validation checks) so that we are allowing for more time to catch the unexpected hiccup - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12524#pullrequestreview-1753604135
Re: Revisiting JDK-4512189: ZipConstants leaking into public APIs
I agree, I am not sure there is a large benefit to changing this at this time. Seems a lot of effort for minimal gain at best. On Nov 28, 2023, at 12:15 PM, Alan Bateman mailto:alan.bate...@oracle.com>> wrote: On 28/11/2023 15:50, Eirik Bjørsnøs wrote: : In light of this, I would like to revisit this issue, 22 years later: - Is my assessment that this change is actually not binary incompatible sound, or did I miss something? - Would it in any case make sense to mark ZipConstants as @Deprecated, maybe for removal to alert people we want to remove the constants? - Could we aim to make the mentioned classes *not* implement ZipConstants, following the regular deprecation process, CSR and release note etc? This is a JDK 1.1 era mistake. It would a source incompatible change to "remove" the constants. It would require corpus searches to gauge the impact. I think the question is whether it's worth the disruption, is your motivation to cleanup this area or something stronger? -Alan [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen | Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: JDK-8320714: java/util/Locale/LocaleProvidersRun.java and java/util/ResourceBundle/modules/visibility/VisibilityTest.java timeout after passing [v2]
On Mon, 27 Nov 2023 21:12:17 GMT, Justin Lu wrote: >> Please review this PR which fixes timeouts for the two tests: >> _java/util/Locale/LocaleProvidersRun.java_ and >> _java/util/ResourceBundle/modules/visibility/VisibilityTest.java_. >> >> These tests were updated to accept VM flags in >> [JDK-8319569](https://bugs.openjdk.org/browse/JDK-8319569), which is now >> causing timeouts in tiers 6 and 8. One set of VM flags causes >> _VisibilityTest.java_ to timeout with a duration of over 2 hours in tier6. >> >> Both have been updated to run without VM flags and marked as `vm.flagless`. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > clarify comments Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16831#pullrequestreview-1751388560
Re: RFR: 8320602: Lock contention in SchemaDVFactory.getInstance()
On Mon, 27 Nov 2023 17:24:35 GMT, Joe Wang wrote: > Applying an update from the upstream source, quote: "Removing synchronized > from the getInstance() methods". As the reporter noted, this change was made > 16 years ago. > > Tests: SQE & unit tests passed This looks OK. Please add a the appropriate label given there is no test included with this change so that you do not get asked later where is the test ;-) - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16829#pullrequestreview-1751080653
Re: RFR: 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. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16827#pullrequestreview-1750875671
Re: RFR: JDK-8319569: Several java/util tests should be updated to accept VM flags [v3]
On Mon, 20 Nov 2023 19:15:18 GMT, Justin Lu wrote: >> Please review this PR which allows these _j.util_ tests to launch new JVM >> processes with VM flags, >> >> This is primarily done using by switching to >> `ProcessTools::createTestJavaProcessBuilder`. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Pass test.java.opts to PropertiesTest.sh Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16705#pullrequestreview-1742959505
Re: RFR: 8306055: Add a built-in Catalog to JDK XML module [v3]
On Mon, 20 Nov 2023 17:46:53 GMT, Joe Wang wrote: >> Implement the built-in Catalog. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > add a note; fix alignment Thank you Joe. All good by me and Congrats on moving this across the goal line - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16719#pullrequestreview-1740399188
Re: RFR: 8306055: Add a built-in Catalog to JDK XML module [v2]
On Sun, 19 Nov 2023 23:36:16 GMT, Joe Wang wrote:
>> Implement the built-in Catalog.
>
> Joe Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove J2SE directory; add note explaining how DTDs are resolved
Hi Joe,
Thank you for all of your hard work and perseverance to drive this project as I
realize it was a lot of work and iterations based on feedback from the team.
Overall looks good. A couple of minor comments below
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/XMLEntityManager.java
line 1084:
> 1082: String publicId, String systemId) {
> 1083: InputSource is = resolveWithCatalog(cr, cFile, publicId,
> systemId);
> 1084: //if (is != null && !is.isEmpty()) {
Probably can delete this
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/XMLEntityManager.java
line 1101:
> 1099: return cr.resolveEntity(publicId, systemId);
> 1100: } catch (CatalogException e) {
> 1101:
> fErrorReporter.reportError(XMLMessageFormatter.XML_DOMAIN,"CatalogException",
Alignment looks like it needs to be sanity checked here
-
PR Review: https://git.openjdk.org/jdk/pull/16719#pullrequestreview-1739611943
PR Review Comment: https://git.openjdk.org/jdk/pull/16719#discussion_r1399080861
PR Review Comment: https://git.openjdk.org/jdk/pull/16719#discussion_r1399081930
Re: RFR: 8320199: Fix HTML 5 errors in java.math.BigInteger
On Wed, 15 Nov 2023 21:59:56 GMT, Brian Burkhalter wrote: > Clean up HTML error due to nested anchor (``) elements. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16679#pullrequestreview-1733121214
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v6]
On Wed, 15 Nov 2023 20:13:15 GMT, Eirik Bjorsnos wrote:
>> 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?
A follow on PR is fine. Why don't you take that on seeing you already have
your sleeves rolled up in this area :-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1394744061
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v11]
On Wed, 15 Nov 2023 19:26:06 GMT, Eirik Bjorsnos wrote: >> 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. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/12991#pullrequestreview-1732831434
Re: RFR: 8319986: Invalid/inconsistent description and example for DateFormat [v4]
On Tue, 14 Nov 2023 19:32:57 GMT, Naoto Sato wrote: >> Correcting the explanation of the `DateFormat.SHORT` constant. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > abbreviated/full clarification Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16645#pullrequestreview-1730811132
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v6]
On Wed, 8 Nov 2023 13:45:14 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:
>
> Add a @bug reference in the test
Thanks for tackling this Eirik, I think this is looking good overall.
A few comments below based on my pass through it today. I want to spend a bit
more time looking at ZipInputStream over the coming days.
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
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
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**
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
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
-
PR Review: https://git.openjdk.org/jdk/pull/12524#pullrequestreview-1728032033
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1392467451
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1393058445
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1391555674
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1391562081
PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1391559654
Re: RFR: 8316141: Improve CEN header validation checking
On Wed, 8 Nov 2023 20:26:32 GMT, Eirik Bjorsnos 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.
>
> 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?
I can remove the cast as that was a holdover. I chose to make this a long
knowing that it would not overflow but an overflow while unlikely could occur
depending on the value of `pos` in the statement below
if (headerSize > 0x || pos + headerSize > cen.length - ENDHDR) {
zerror("invalid CEN header (bad header size)");
}
I could keep headerSize an int and then move the cast to the if statement:
if (headerSize > 0x || (long)pos + headerSize > cen.length - ENDHDR) {
zerror("invalid CEN header (bad header size)");
}
I decided making headerSize a long might be clearer but do not have a strong
preference and will go with the consensus
As far as the name, I don't have a strong preference, but not sure
combinedLength is any better
> 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` ?
`entryPos` was the name of the field from a previous PR so I did not see a need
to change it and decided there was no need to keep extraStartingOffset given
the change in validation above.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16570#discussion_r1388318963
PR Review Comment: https://git.openjdk.org/jdk/pull/16570#discussion_r1388313503
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.
Thank you for the comments. See my replies below.
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 a really old
spec. That being said, I do not object to removing the validation if that is
the overall preference.
zerror("invalid CEN header (bad header size)");
}
-
PR Review: https://git.openjdk.org/jdk/pull/16570#pullrequestreview-1723102540
Re: RFR: 8306116: Update CLDR to Version 44.0
On Tue, 31 Oct 2023 21:06:13 GMT, Naoto Sato wrote: > Upgrading CLDR to v44 (https://cldr.unicode.org/index/downloads/cldr-44). > Besides the data upgrade, regression tests are modified to accommodate the > following CLDR fixes: > > CLDR-16534: Suggestion to rename the Islamic Calendar to Hijri Calendar > (https://unicode-org.atlassian.net/browse/CLDR-16534) > CLDR-17042: Use exception list for new languages > (https://unicode-org.atlassian.net/browse/CLDR-17042) > CLDR-16586: en_ID shows IDR rather than Rp for currency > (https://unicode-org.atlassian.net/browse/CLDR-16586) > CLDR-16358: Mexico and Latin America countries should be using 12 hours time > format (https://unicode-org.atlassian.net/browse/CLDR-16358) > CLDR-16974: en_GB and en_AU: request to remove comma from MMMd format > (https://unicode-org.atlassian.net/browse/CLDR-16974) > CLDR-16857: Need updated translations for new Sierra Leone currency - SLL > became SLE (https://unicode-org.atlassian.net/browse/CLDR-16857) Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16439#pullrequestreview-1722957033
Re: RFR: 8296250" Update ICU4J to Version 74.1
On Wed, 1 Nov 2023 17:40:09 GMT, Naoto Sato wrote: > Updating the ICU4J components to v74.1 (https://icu.unicode.org/download/74). > This change completes the Unicode 15.1 upgrade. The change is merely > replacing binary data files used for the Normalization and BiDi support Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16458#pullrequestreview-1722327026
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 Latest Mach 5 run looks fine - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12959#pullrequestreview-1722323767
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v10]
On Mon, 30 Oct 2023 15:50:49 GMT, Eirik Bjorsnos wrote: >> 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 Latest Mach5 run looks fine - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12991#pullrequestreview-1722319085
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v9]
On Fri, 3 Nov 2023 20:01:44 GMT, Eirik Bjorsnos 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 Have an internal mach5 run going and will let you know when it completes - PR Comment: https://git.openjdk.org/jdk/pull/12959#issuecomment-1802653611
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v3]
On Wed, 8 Nov 2023 13:54:23 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? > > > > > > 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 > ``` I have an internal mach5 run going. Once it completes, I will approve once again and you can push the PR - PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1802637172
RFR: 8316141: Improve CEN header validation checking
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. - Commit messages: - Initial changes for JDK-8316141 Changes: https://git.openjdk.org/jdk/pull/16570/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16570=00 Issue: https://bugs.openjdk.org/browse/JDK-8316141 Stats: 335 lines in 4 files changed: 322 ins; 4 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/16570.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16570/head:pull/16570 PR: https://git.openjdk.org/jdk/pull/16570
Re: RFR: 8314891: Additional Zip64 extra header validation [v8]
On Wed, 8 Nov 2023 16:27:56 GMT, Eirik Bjorsnos wrote: > @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? Intentional, as this was a follow on to the updates which were done previously to the CEN work in August, this is follow on cleanup. Updates to ZipInputStream would be done separately under a separate PR or could be done via your work on 8303866 - PR Comment: https://git.openjdk.org/jdk/pull/15650#issuecomment-1802340986
Integrated: 8314891: Additional Zip64 extra header validation
On Sat, 9 Sep 2023 14:33:53 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 This pull request has now been integrated. Changeset: 82747132 Author:Lance Andersen URL: https://git.openjdk.org/jdk/commit/82747132b07e288c1e6c2ba3392748c7718e994a Stats: 1122 lines in 3 files changed: 1037 ins; 51 del; 34 mod 8314891: Additional Zip64 extra header validation Reviewed-by: coffeys - PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8305814: Update Xalan Java to 2.7.3 [v2]
On Tue, 7 Nov 2023 05:43:06 GMT, Joe Wang wrote: >> Xalan 2.7.3: merge minor changes from the upstream project. >> >> Test: existing XML tests pass > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > remove commented out block in ExsltDatetime Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16532#pullrequestreview-1717450671
Re: RFR: 8305814: Update Xalan Java to 2.7.3
On Mon, 6 Nov 2023 22:52:47 GMT, Joe Wang wrote:
> Xalan 2.7.3: merge minor changes from the upstream project.
>
> Test: existing XML tests pass
src/java.xml/share/classes/com/sun/org/apache/xalan/internal/lib/ExsltDatetime.java
line 104:
> 102:
> buff.append(posneg).append(formatDigits(hrs)).append(':').append(formatDigits(min));
> 103: }
> 104: return buff.toString();*/
This commented out block is difficult to see given Can we clean this up or
better yet remove this code if not needed?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16532#discussion_r1384244649
Re: RFR: 8318971: jar v17 should either exit on error immediately or create archive as jar v1.8 did
On Mon, 6 Nov 2023 11:28:23 GMT, Ryan Wallace wrote: > I had a look and its been in since JDK 9 > (https://bugs.openjdk.org/browse/JDK-8158295) but haven’t found any mention > of this as a specific desired behaviour so I am going with just noticed now. > Its not a major blocker as the user can either make sure the missing file is > in place or exclude it from being added to the archive and rerun. > > I agree it will need some thought, I was of the opinion that we should fail > upfront and notify the user of why. The JDK 8 behaviour doesn’t make sense as > the state is recognised as an error and there are tests to validate this, but > they do not validate the archive has not been created, in my opinion we > shouldn’t continue on and create it. There are other areas in the jar tool in > 8 that will clean up (delete) the archive that has been created if there is > an error, this case just seems to fall through the cracks. As part of the decision process, we should consider the behavior of what simillar tools do in this area. We may also want to discuss the merits of whether there should be an option to toggle the behavior - PR Comment: https://git.openjdk.org/jdk/pull/16423#issuecomment-1795072524
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 think you are in a good place with the test clean up. Thanks for your efforts here I can kick of a test run internally next week or perhaps Sunday - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12959#pullrequestreview-1713371131
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v8]
On Mon, 30 Oct 2023 17:57:23 GMT, Eirik Bjorsnos wrote: >> 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 :-) I exchanged messages with Iris and she is comfortable with the updates to the copyright - PR Review Comment: https://git.openjdk.org/jdk/pull/12959#discussion_r1382103154
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v9]
On Thu, 2 Nov 2023 18:21:07 GMT, Eirik Bjorsnos 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 :-) If you verified that we are a complete match, I am good with that. Thank you for validating - PR Comment: https://git.openjdk.org/jdk/pull/12959#issuecomment-1791444261
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 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. This would be an additional test along with your proposed enhancements - PR Comment: https://git.openjdk.org/jdk/pull/12959#issuecomment-1785936716
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. Thanks for trying to move this forward. Please see my comments below. Another question, is the zip that is generated by this test readable by other zip tools such as info-zip, Apache Common-compress, winzip? 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 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 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 - PR Review: https://git.openjdk.org/jdk/pull/12959#pullrequestreview-1704351462 PR Review Comment: https://git.openjdk.org/jdk/pull/12959#discussion_r1376354980 PR Review Comment: https://git.openjdk.org/jdk/pull/12959#discussion_r1376355486 PR Review Comment: https://git.openjdk.org/jdk/pull/12959#discussion_r1376360218
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v6]
On Mon, 13 Mar 2023 03:18:26 GMT, Eirik Bjorsnos wrote:
>> test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 53:
>>
>>> 51:
>>> 52: // Maximum size (unsigned short) of an extra field allowed by the
>>> standard
>>> 53: static final int MAX_EXTRA_FIELD_SIZE = 0X;
>>
>> 4.4.10 file name length: (2 bytes)
>>4.4.11 extra field length: (2 bytes)
>>4.4.12 file comment length: (2 bytes)
>>
>>The length of the file name, extra field, and comment
>>fields respectively. The combined length of any
>>directory record and these three fields SHOULD NOT
>>generally exceed 65,535 bytes.
>
> `The combined length of any directory record and these three fields SHOULD
> NOT generally exceed 65,535 bytes.`
>
> I was not aware of this 'combined length' clause. Are you suggesting we take
> this into account in this test? I feel that would be somewhat strange, given
> that this constraint seems not be be enforced at all by ZipEntry or
> ZipOutputStream:
>
>
> ZipEntry(String name):
>
> if (name.length() > 0x) {
> throw new IllegalArgumentException("entry name too long");
> }
>
>
>
> ZipOutputStream.writeCEN:
>
> if (commentBytes != null) {
> writeBytes(commentBytes, 0, Math.min(commentBytes.length, 0x));
> }
I would clarify the comment a bit and perhaps point to the comments Martin
mentions in the review above
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376284800
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v3]
On Mon, 30 Oct 2023 12:13:59 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? We would have to run across all of the mach5 platforms to verify. You could remove it for now and we go from here - PR Comment: https://git.openjdk.org/jdk/pull/12991#issuecomment-1785341362
Re: RFR: 8304020: Speed up test/jdk/java/util/zip/ZipFile/TestTooManyEntries.java and clarify its purpose [v7]
On Mon, 30 Oct 2023 11:57:09 GMT, Eirik Bjorsnos wrote:
>> 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 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/f5d01b56...9629b8d2
Thanks for restarting the discussion on this test clean up.
Please see comments below
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.
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
test/jdk/java/util/zip/ZipFile/CenSizeTooLarge.java line 187:
> 185: } else {
> 186: channel.position(position);
> 187: if (Arrays.equals(LAST_COMMENT_BYTES, 0,
> LAST_COMMENT_BYTES.length, b, off, len)) {
I would suggest a comment to help future developers. I realize there are some
earlier bread crumbs but I think this would be beneficial
-
PR Review: https://git.openjdk.org/jdk/pull/12991#pullrequestreview-1704236482
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376286075
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376295495
PR Review Comment: https://git.openjdk.org/jdk/pull/12991#discussion_r1376302371
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/b0cd882b...fad0da2e We need to hold off this PR until https://github.com/openjdk/jdk/pull/15650 has been integrated as it impacts some of the changes proposed here - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1783874658
Re: RFR: 8316996: Catalog API Enhancement: add a factory method [v5]
On Fri, 27 Oct 2023 19:16:33 GMT, Joe Wang wrote: >> Add a new factory method so that a CatalogResolver can be created with a >> resolve property on top of the Catalog object. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Impl CSR Changes: change the parameter type from String to Enum Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16045#pullrequestreview-1702399673
Re: RFR: JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method [v4]
On Fri, 27 Oct 2023 18:40:56 GMT, Justin Lu wrote: >> Please review this PR which updates ChoiceFormat and MessageFormat to no >> longer call overridable methods in their constructors. >> >> The overridable methods called in the constructors are: >> _ChoiceFormat::applyPattern_, _ChoiceFormat::setChoices_, and >> _MessageFormat::applyPattern_. The code should be updated so that both the >> methods and constructors call a separate private method. Some other drive-by >> cleanup changes were included in the change as well. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > re-apply 'final' to the static methods in ChoiceFormat Looks reasonable. I assume you validated the links work in the generated docs as a sanity check - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16064#pullrequestreview-1702392861
Re: RFR: 8318487: Specification of the ListFormat.equals() method can be improved [v3]
On Tue, 24 Oct 2023 17:58:48 GMT, Naoto Sato wrote: >> Spec clarification of `ListFormat.equals()` method, utilizing newly >> introduced `getLocale()` and `getPatterns()`. A corresponding CSR has also >> been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review comments Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16321#pullrequestreview-1695659263
Re: RFR: 8274122: java/io/File/createTempFile/SpecialTempFile.java fails in Windows 11 [v2]
On Tue, 19 Sep 2023 21:55:24 GMT, Brian Burkhalter wrote: >> Windows 11 does not reserve as many names as prior versions of Windows so do >> not expect exceptions for COM7 and LPT1. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8274122: Make value of exceptionExpected more robust to Windows version Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15460#pullrequestreview-1695517205
Re: RFR: 8314891: Additional Zip64 extra header validation [v8]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/208a5ecc..255aec85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15650=07 - incr: https://webrevs.openjdk.org/?repo=jdk=15650=06-07 Stats: 75854 lines in 1953 files changed: 44906 ins; 19381 del; 11567 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v10]
On Mon, 23 Oct 2023 13:04:20 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: > > Incorporate test review comments from Jai I think the latest changes are fine Sean - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16115#pullrequestreview-1692683963
Re: RFR: JDK-8077371: Binary files in JAXP test should be removed [v3]
On Fri, 21 Apr 2023 13:33:37 GMT, Mahendra Chhipa wrote: >> Mahendra Chhipa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Implemented the review comment. > > Thanks for your review comments. I will implement them and push again. > @mahendrachhipa it is conventional and preferred to allow reviewers and > people who have commented on a PR a day to review/re-review before > integrating. Just to follow on to Roger's comment. There have been many changes to this PR since Joe approved this on April 28th so we definitely should have given more time to allow for the reviewers to go through the latest updates and provide feedback and/or approve - PR Comment: https://git.openjdk.org/jdk/pull/13537#issuecomment-1772941378
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v8]
On Wed, 18 Oct 2023 15:59:13 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 lastModifiedTime comment Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16115#pullrequestreview-1685516723
Re: RFR: 8318322: Update IANA Language Subtag Registry to Version 2023-10-16
On Tue, 17 Oct 2023 20:06:03 GMT, Justin Lu wrote: > This change updates the IANA subtag registry to the update released on > 2023-10-16. > > Announcement -> > https://mm.icann.org/pipermail/ietf-languages-announcements/2023-October/89.html Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16227#pullrequestreview-1685391866
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v7]
On Wed, 18 Oct 2023 10:09:29 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: > > Testcase feedback from Lance Thank you for the last set up updates Sean - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16115#pullrequestreview-1684700793
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v6]
On Tue, 17 Oct 2023 19:55:19 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:
>
> remove getCanonicalFile calls, update ZipFile jmh benchmark test, tidy up
> junit test
Latest updates look good.
test/jdk/java/util/zip/ZipFile/ZipSourceCache.java line 103:
> 101: // the old Source in use for old file, should no longer
> map correctly
> 102: IOException ioe = assertThrows(IOException.class, () ->
> readZipFileContents(absoluteZipFile));
> 103: assertEquals("ZipFile invalid LOC header (bad
> signature)", ioe.getMessage());
Perhaps put the expected message in a constant in the event we need to change
it at some point it is easier to find
-
Marked as reviewed by lancea (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16115#pullrequestreview-1683270909
PR Review Comment: https://git.openjdk.org/jdk/pull/16115#discussion_r1362698792
Re: RFR: 8317979: Use TZ database style abbreviations in the CLDR locale provider [v3]
On Tue, 17 Oct 2023 16:52:12 GMT, Naoto Sato wrote: >> CLDR provides very few short names for time zones, such as PST/PDT. This >> will typically end up substituting names from the COMPAT provider. Once the >> COMPAT is removed, they will be displayed in the GMT format, i.e., >> GMT+XX:YY. Although some of the short names in the COMPAT provider are >> somewhat questionable (less common ones are simply made up from the long >> names by taking the initials), it would not be desirable for them to fall >> back to the GMT format. >> To mitigate the situation, CLDR can use the abbreviated names from the TZ >> database, which contains legacy (major) short names as FORMAT. The CLDR >> provider can use them instead of the GMT offset style. This enhancement is a >> precursor to the future removal of the COMPAT provider. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Delay populating GMT format at runtime. Reflecting review comments. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16206#pullrequestreview-1683251272
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v4]
On Fri, 13 Oct 2023 12:18:41 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:
>
> minor test edits and comment updates
Thank you for driving this issue Sean.
Overall I think it looks good.
Please see my minor comment below which you can handle prior to pushing the PR
test/jdk/java/util/zip/ZipFile/ZipSourceCache.java line 63:
> 61:
> 62: @Test
> 63: public static void test() throws Exception {
Perhaps give the test a better name and add a brief comment block at least to
the test method as we have been trying to add more clarity to the new tests
that we add
-
Marked as reviewed by lancea (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16115#pullrequestreview-1667211269
PR Review Comment: https://git.openjdk.org/jdk/pull/16115#discussion_r1352165848
Re: RFR: JDK-8077371: Binary files in JAXP test should be removed [v8]
On Wed, 11 Oct 2023 19:27:46 GMT, Joe Wang wrote: > Thanks for the update. > > I'm not sure it's necessary to repeat tests with Base64 input. I think > Daniel's comment was that the data can be optionally base64-encoded into a > string, not that the tests needed to be duplicated. > > The generatePseudoCodeFor... methods in the Util class can be consolidated > too. Yes it does look like there is duplication where the tests are run using a byte array which represents the serialized object and a second time using a Base64 representation. We do not need both as the intent of the original test is to validate that the serialized object can be read across the JDK versions - PR Comment: https://git.openjdk.org/jdk/pull/13537#issuecomment-1760036070
Re: RFR: JDK-8317633: Modernize text.testlib.HexDumpReader [v2]
On Fri, 6 Oct 2023 21:07:51 GMT, Justin Lu wrote: >> Please review this PR which cleans up the static test utility class >> _HexDumpReader_. >> >> This cleans up the code by replacing the nested _ByteArrayBuilder_ class >> with _HexFormat_, and simplifies the File processing by using a stream. >> Changes were tested to ensure that the _text_ tests are still getting >> equivalent ByteArrayInputStreams as before. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > Reflect review comments Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16075#pullrequestreview-1665156750
Re: RFR: JDK-8317633: Modernize text.testlib.HexDumpReader
On Fri, 6 Oct 2023 17:19:29 GMT, Justin Lu wrote:
> Please review this PR which cleans up the static test utility class
> _HexDumpReader_.
>
> This cleans up the code by replacing the nested _ByteArrayBuilder_ class with
> _HexFormat_, and simplifies the File processing by using a stream. Changes
> were tested to ensure that the _text_ tests are still getting equivalent
> ByteArrayInputStreams as before.
looks OK overall...
A couple minor comments below
test/jdk/java/text/testlib/HexDumpReader.java line 54:
> 52:
> 53: // Converts a Hex dump file (File) into an InputStream containing
> bytes
> 54: public static InputStream getStreamFromHexDump(File hexFile) {
Maybe add a comment of the expected format of the file passed to this method.
-
Marked as reviewed by lancea (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16075#pullrequestreview-1662633249
PR Review Comment: https://git.openjdk.org/jdk/pull/16075#discussion_r1349209871
Re: RFR: 8317471: ListFormat::parseObject() spec can be improved on parsePosition valid values
On Thu, 5 Oct 2023 17:57:00 GMT, Naoto Sato wrote: > Adding IOOBE clause to clarify the behavior on an invalid `parsePos` argument > on calling `ListFormat::parseObject()`. A corresponding CSR has also been > drafted. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16063#pullrequestreview-1660626757
Re: RFR: 8317141: Remove unused validIndex method from URLClassPath$JarLoader
On Fri, 29 Sep 2023 05:41:11 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which removes unused (internal) > method from the `private` `URLClassPath$JarLoader`? > > The `validIndex` method which is being removed here was being used when JAR > index was supported. We removed support for JAR index in > https://bugs.openjdk.org/browse/JDK-8302819. This method is now a leftover > and no longer used. > > The commit in this PR also removes a couple of other member fields in > `URLClassPath$JarLoader`. These fields too were used previously when JAR > index was in use and are no longer used. > > tier1, tier2 and tier3 tests continue to pass with this change. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15976#pullrequestreview-1650499922
Re: RFR: 8317126: Redundant entries in Windows `tzmappings` file
On Thu, 28 Sep 2023 17:37:00 GMT, Naoto Sato wrote: > Removing redundant entries in `lib/tzmappings` file on Windows. The file maps > Windows time zones to Java time zones according to the region. Since `001` > means world, no region-specific entries are needed if those time zones are > the same. The diff of the generated tzmappings files, before and after the > fix, is attached. > [tzmappings.diff.txt](https://github.com/openjdk/jdk/files/12752377/tzmappings.diff.txt) Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15968#pullrequestreview-1649440371
Re: RFR: 8315721: CloseRace.java#id0 fails transiently on libgraal
On Tue, 26 Sep 2023 17:03:00 GMT, Roger Riggs wrote: > The timing of the test can be disturbed by -Xcomp so do not run with -Xcomp. > The problem appears with the Graal compiler but may also occur on other > platforms. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15930#pullrequestreview-1644854523
Re: RFR: JDK-8316559: Refactor some util/Calendar tests to JUnit [v2]
On Fri, 22 Sep 2023 19:50:49 GMT, Justin Lu wrote: >> Please review this PR which converts some tests under _Calendar_ to use >> JUnit. These tests either previously used the internal _IntlTest_, or used >> no framework at all. >> >> Any files named BugXXX.java will be renamed after review. > > Justin Lu has updated the pull request incrementally with two additional > commits since the last revision: > > - Review: revert removal of SupressWarnings annotation > - Reflect review comments Overall, this is fine. I would like to suggest comments to introduce all tests and DataProviders. Extra credit for helper methods. >From a future maintainers Point of view, having more info in the tests is >beneficial. - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15853#pullrequestreview-1640620487
Re: RFR: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind [v9]
On Thu, 21 Sep 2023 15:35:16 GMT, Brian Burkhalter wrote: >> Add a `finally` block to delete the created files. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8315960: Use assertEquals Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15757#pullrequestreview-1638338199
Re: RFR: 8316383: NullPointerException in AbstractSAXParser after JDK-8306632
On Tue, 19 Sep 2023 21:10:41 GMT, Joe Wang wrote: > Fix a NPE. The DTD patch (JDK-8306632) moved initialization to factories, for > example, for SAXParser, the SecurityManagers are created in the > SAXParserFactory impl and passed on to instances of SAXParsers. The > (deprecated) XMLReaderFactory however, instantiates SAXParsers directly, thus > without initializing the SecurityManagers. This patch checks the condition > and creates them if they have not already been constructed. > > Test: XML tests passed. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15828#pullrequestreview-1638204539
Re: RFR: 8314891: Additional Zip64 extra header validation [v7]
> 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 incrementally with one additional commit since the last revision: Add missing space - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/bbc5325d..208a5ecc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15650=06 - incr: https://webrevs.openjdk.org/?repo=jdk=15650=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v6]
> 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 incrementally with one additional commit since the last revision: Revamp isZip64ExtBlockSizeValid - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/f64a70c3..bbc5325d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15650=05 - incr: https://webrevs.openjdk.org/?repo=jdk=15650=04-05 Stats: 61 lines in 3 files changed: 14 ins; 13 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v5]
> 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 six additional commits since the last revision: - 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 - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/afc9d7d0..f64a70c3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15650=04 - incr: https://webrevs.openjdk.org/?repo=jdk=15650=03-04 Stats: 56 lines in 3 files changed: 18 ins; 1 del; 37 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v4]
> 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 five additional commits since the last revision: - 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 - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/24b159d8..afc9d7d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15650=03 - incr: https://webrevs.openjdk.org/?repo=jdk=15650=02-03 Stats: 89465 lines in 2912 files changed: 37847 ins; 14022 del; 37596 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8315960: test/jdk/java/io/File/TempDirDoesNotExist.java leaves test files behind
On Thu, 14 Sep 2023 21:53:30 GMT, Brian Burkhalter wrote: > Add a `finally` block to delete the created files. Given this is a small test, perhaps it would be worthwhile to convert to Junit as part of your cleanup - PR Review: https://git.openjdk.org/jdk/pull/15757#pullrequestreview-1627818889
Re: RFR: 8316144: Remove unused field jdk.internal.util.xml.impl.XMLStreamWriterImpl.Element._Depth
On Tue, 12 Sep 2023 20:04:54 GMT, Andrey Turbanov wrote: > A field `short _Depth` in the > `jdk.internal.util.xml.impl.XMLStreamWriterImpl.Element` class is unused and > can be removed. This makes sense to remove. Joe is on holiday so please wait for his input as there are some additional constants that may also be removed. - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15692#pullrequestreview-1627780880
Re: RFR: 8314891: Additional Zip64 extra header validation [v3]
> 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 incrementally with one additional commit since the last revision: Remove tab(s) from comment - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/40504b2c..24b159d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15650=02 - incr: https://webrevs.openjdk.org/?repo=jdk=15650=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8314891: Additional Zip64 extra header validation [v2]
On Mon, 11 Sep 2023 23:42:37 GMT, Sergey Bylokhov wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Added additional tests, along with additional cleanup and refactoring > > test/jdk/java/util/zip/ZipFile/MissingZIP64EntriesTest.java line 52: > >> 50: * starting number is set to 0x or when we have a valid Zip64 Extra >> header >> 51: * size but missing the corresponding field. >> 52: * @run junit MissingZIP64EntriesTest > > Is this comment accurate? I think we should check 3 cases when the header > extra len == 0, len == 8 and len ==16, but still do not contain all required > information. Clarified the comment to make it a bit clearer and also added additional tests - PR Review Comment: https://git.openjdk.org/jdk/pull/15650#discussion_r1326168515
Re: RFR: 8314891: Additional Zip64 extra header validation [v2]
On Tue, 12 Sep 2023 08:47:03 GMT, Andrey Turbanov wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Added additional tests, along with additional cleanup and refactoring > > test/jdk/java/util/zip/ZipFile/MissingZIP64EntriesTest.java line 198: > >> 196: * actual value is stored in the Zip64 Extra Header >> 197: */ >> 198: private static final int ZIP64_MAGICCOUNT = 0x; > > Suggestion: > > private static final int ZIP64_MAGICCOUNT = 0x; Removed the extra space. thank you for pointing it out - PR Review Comment: https://git.openjdk.org/jdk/pull/15650#discussion_r1326169219
Re: RFR: 8314891: Additional Zip64 extra header validation [v2]
> 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 incrementally with one additional commit since the last revision: Added additional tests, along with additional cleanup and refactoring - Changes: - all: https://git.openjdk.org/jdk/pull/15650/files - new: https://git.openjdk.org/jdk/pull/15650/files/bb962fcf..40504b2c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15650=01 - incr: https://webrevs.openjdk.org/?repo=jdk=15650=00-01 Stats: 737 lines in 3 files changed: 578 ins; 99 del; 60 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
RFR: 8314891: Additional Zip64 extra header validation
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 not set to 0x -- disk starting number is not 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 - Commit messages: - Clean up some minor formatting issues - Additional Zip64 extra header validation Changes: https://git.openjdk.org/jdk/pull/15650/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15650=00 Issue: https://bugs.openjdk.org/browse/JDK-8314891 Stats: 544 lines in 3 files changed: 513 ins; 7 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/15650.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15650/head:pull/15650 PR: https://git.openjdk.org/jdk/pull/15650
Re: RFR: 8306632: Add a JDK Property for specifying DTD support
On Fri, 28 Jul 2023 18:41:48 GMT, Joe Wang wrote: > Add a JDK Impl specific property 'jdk.xml.dtd.support' for applications to > specify how DTDs are handled. This property is uniformly supported across the > JDK XML libraries. It complements, rather than replaces, the existing > properties that are supportDTD for StAX and disallow-doctype-decl for DOM and > SAX processors, which means the later will continue to work as they were and > that if they are set, the new property will have no effect. Applications that > use the existing properties continue to work as expected. > >This patch continues the path made previously with Xalan and XPath in > which functions were moved into the jdk/xml classes. Similar changes are now > made with the Xerces and XML classes, consolidating functions into the > jdk/xml classes, reducing duplicate code for easier future maintenance. > > Tests: new tests are added to cover the various processors. >Existing tests pass. Only one was updated due to the change to the > property impl. Hi Joe, I have made a few passes through the code and overall I think it is OK. Nothing jumped out and was glaringly wrong given you have touched a large number of classes and added a fair amount of additional tests - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15075#pullrequestreview-1611789294
