Re: RFR: 8336021: Doccheck: valign not allowed for HTML5 in java.xml
On Wed, 10 Jul 2024 16:24:01 GMT, Joe Wang wrote: > Remove valign Attribute from the javadoc for org.w3c.dom.Attr. The table > looks good with the default setting. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20117#pullrequestreview-2170285535
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes [v5]
On Sat, 29 Jun 2024 18:24:46 GMT, Eirik Bjørsnøs wrote: >> Please consider this PR which suggests we rename `ZipEntry.extraAttributes` >> to `ZipEntry.externalFileAttributes`. >> >> 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.externalFileAttributes` 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.externalFileAttributes` >> - Update `JavaUtilZipFileAccess` to similarly rename methods to >> `setExternalFileAttributes` and `getExternalFileAttributes` >> - Rename the field `JarSigner.extraAttrsDetected` to >> `JarSigner.externalFileAttrsDetected` >> - Rename a local variable in `JarSigner.writeEntry` to >> `externalFileAttributes` >> - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to >> `externalFileAttributesDetected` >> - Rename resource string key names in `s.s.t.jarsigner.Resources` from >> `extra.attributes.detected` to `external.file.attributes.detected` >> - Rename method `SymlinkTest.verifyExtraAttrs` to >> `verifyExternalFileAttributes`, 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" > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge branch 'master' into zipentry-external-attributes > - Rename SymlinkTest.verifyExternalAttrs to verifyExternalFileAttributes > - Rename ZipFileSystem.Entry.posixPerms to externalFileAttributes > - For clarity, use "externalFileAttributes" instead of "externalAttributes" > - Merge branch 'master' into zipentry-external-attributes > - Update copyright years for 2024 > - Merge branch 'master' into zipentry-external-attributes > - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16952#pullrequestreview-2154579839
Re: java.util.zip.ZipError seems unused
The code you point to below is leveraging ZipFS, which provided as a demo in JDK 8 and was not a supported part of Java until JDK 9 at which point ZipFS did not use ZipError. So I understand your point but we also need to keep in mind that demo != supported as we consider the eventual removal of this class. On Jun 30, 2024, at 10:18 AM, Glavo wrote: I am the maintainer of HMCL and we need to catch ZipError in our program because our program needs to be compatible with Java 8: https://github.com/HMCL-dev/HMCL/blob/85b68ad135267bc33e03c3624b1bced9b7804c39/HMCLCore/src/main/java/org/jackhuang/hmcl/util/io/CompressingUtils.java#L218-L220 I'd love to see it deprecated, but I'm sure there are some use cases for it, so please evaluate the risks carefully. Glavo On Sun, Jun 30, 2024 at 5:42 PM Eirik Bjørsnøs mailto:eir...@gmail.com>> wrote: Hi! The java.util.zip.ZipError class seems unused in OpenJDK. I assume this is legacy from the native ZIP implementation in Java 8. This exception class extends InternalError and seems to have been added in Java 6 to help compatibility with existing code catching InternalError (JDK-4615343) This change also introduced the TestZipError test, which verified that ZipError was thrown while enumerating a ZIP file which was changed after being opened. The reimplementation of the ZIP implementation to Java (JDK-8145260) updated this test to expect a ZipException instead of the ZipError. Given that this class has now fallen out of use in OpenJDK, should we: 1: Deprecate it 2: Deprecate it for removal 3: Do nothing, keeping it around has a low cost 4: Something else It would also be useful if someone with access to a large code corpus could search for usages of this class so we could assess compatibility concerns of removing it. Thanks, Eirik. [oracle_sig_logo.gif] Lance Andersen | Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: java.util.zip.ZipError seems unused
Hi Eirik, The removal of ZipError from ZipFile/ZipFileSystem/ZipFileSystemProvider occurred via 8145260 and 8037394 for JDK 9 The test should also be re-written at this point Jai or I can make a pass to see if there are any external usages via a corpus search but I tend to doubt it On Jun 30, 2024, at 3:20 AM, Eirik Bjørsnøs wrote: Hi! The java.util.zip.ZipError class seems unused in OpenJDK. I assume this is legacy from the native ZIP implementation in Java 8. This exception class extends InternalError and seems to have been added in Java 6 to help compatibility with existing code catching InternalError (JDK-4615343) This change also introduced the TestZipError test, which verified that ZipError was thrown while enumerating a ZIP file which was changed after being opened. The reimplementation of the ZIP implementation to Java (JDK-8145260) updated this test to expect a ZipException instead of the ZipError. Given that this class has now fallen out of use in OpenJDK, should we: 1: Deprecate it 2: Deprecate it for removal 3: Do nothing, keeping it around has a low cost 4: Something else It would also be useful if someone with access to a large code corpus could search for usages of this class so we could assess compatibility concerns of removing it. Thanks, Eirik. [oracle_sig_logo.gif] Lance Andersen | Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: RFR: 8334490: Normalize string with locale invariant `toLowerCase()`
On Tue, 18 Jun 2024 18:23:12 GMT, Naoto Sato wrote: > The test library `jdk.test.lib.Platform` uses no-arg `toLowerCase()` for > string comparison, which should be avoided. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19775#pullrequestreview-2126291246
Re: RFR: 8334418: Update IANA Language Subtag Registry to Version 2024-06-14
On Mon, 17 Jun 2024 21:05:13 GMT, Justin Lu wrote: > Please review this PR, which is a trivial update of the IANA subtag registry > to version 2024-06-14. > Announcement: > https://mm.icann.org/pipermail/ietf-languages-announcements/2024-June/92.html Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19757#pullrequestreview-2125182766
Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v3]
On Thu, 6 Jun 2024 14:02:03 GMT, Jaikiran Pai wrote: >> Can I please get a review of this doc-only change which proposes to improve >> the code snippet that's in `java.util.zip.Deflater` and >> `java.util.zip.Inflater` to better explain the usage of those classes? This >> addresses https://bugs.openjdk.org/browse/JDK-8026127. >> >> The commit in the PR cleans up the snippet to correctly compress/decompress >> till the `Deflater` and `Inflater` are `finished()`. Additionally, the >> snippet also shows that the `Deflater` and `Inflater` are expected to be >> closed when their usage it done, to release the resources held by those >> instances. >> >> I've run `make docs-image` locally to verify that the generated snippet >> content as well as the link from `Inflater` work fine in the rendered >> javadoc HTML. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - minor change to the comment > - move the snippet to an external snippet thank you for converting the examples to take advantage of an external snippet Looks good Jai and thank you - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19507#pullrequestreview-2102777897
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]
On Wed, 5 Jun 2024 12:14:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which updates the API specification >> of `java.util.zip.InflaterInputStream.skip()` method to match its current >> implementation? >> >> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes >> a `long` value `n` representing the number of bytes to skip. When a value >> greater than `Integer.MAX_VALUE` is passed to this >> `InflaterInputStream.skip()` method, the current implementation in that >> method only skips at most `Integer.MAX_VALUE` bytes. >> `DeflaterInputStream.skip()` too has the same behaviour. However, in the >> case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that >> method's API documentation. `InflaterInputStream.skip()` currently doesn't >> specify this behaviour. >> >> The commit in this PR updates the `InflaterInputStream.skip()` to specify >> the current implementation. >> >> I'll open a CSR for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > clarify blocking semantic latest update looks good. Thank you and Alan for the additional wordsmithing - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19515#pullrequestreview-2099574770
Re: RFR: 8332750: Broken link in CatalogFeatures.html
On Tue, 4 Jun 2024 21:17:43 GMT, Joe Wang wrote: > Fix a broken link. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19551#pullrequestreview-2097429096
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v2]
On Mon, 3 Jun 2024 05:46:30 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which updates the API specification >> of `java.util.zip.InflaterInputStream.skip()` method to match its current >> implementation? >> >> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes >> a `long` value `n` representing the number of bytes to skip. When a value >> greater than `Integer.MAX_VALUE` is passed to this >> `InflaterInputStream.skip()` method, the current implementation in that >> method only skips at most `Integer.MAX_VALUE` bytes. >> `DeflaterInputStream.skip()` too has the same behaviour. However, in the >> case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that >> method's API documentation. `InflaterInputStream.skip()` currently doesn't >> specify this behaviour. >> >> The commit in this PR updates the `InflaterInputStream.skip()` to specify >> the current implementation. >> >> I'll open a CSR for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > also note that the method may block, just like DeflaterInputStream > specifies it Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19515#pullrequestreview-2093515783
Re: RFR: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java
On Mon, 3 Jun 2024 04:25:38 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which uncomments an > additional test that was commented out in > `test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`? > > As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue > due to which this line was commented out in the test, has been fixed several > releases back and thus this line can now be uncommented. > > I've verified that this test continues to pass with these changes. Change looks fine. We should look at at a later date to convert this to junit and when we do, look to rework isMultiReleaseJar() as it actually runs multiple tests within the test itself and if one fails, the rest will not be run - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19514#pullrequestreview-2093508099
Re: RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails [v2]
On Sat, 1 Jun 2024 05:18:17 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which updates a couple of >> places in the test to use `try-with-resource`? >> >> As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should >> prevent leaking of resources in case there's any failure in the test. The >> test continues to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > convert the test to junit thank you Jai, changes look good - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19492#pullrequestreview-2092169070
Re: RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails
On Fri, 31 May 2024 00:57:18 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which updates a couple of > places in the test to use `try-with-resource`? > > As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should > prevent leaking of resources in case there's any failure in the test. The > test continues to pass with this change. Hi Jai, I think the change is fine. I might suggest that we take the opportunity to convert this test to use junit as part of this PR - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19492#pullrequestreview-2091120545
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v4]
On Fri, 31 May 2024 11:03:18 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address the issue >> noted in https://bugs.openjdk.org/browse/JDK-8210471? >> >> `java.util.zip.Inflater` when instantiated will hold on the native resources >> which are freed only when `Inflater.end()` is called. The constructor of >> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal >> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor >> before returning from the constructor, can run into either a >> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` >> when trying to read a GZIP header from the underlying `InputStream`. In >> either of these cases, the `Inflater` that the `GZIPInputStream` created >> internally will end up leaking and the caller has no way to `end()` that >> `Inflater` or even knowledge of that `Inflater`. >> >> The commit in this PR catches the `IOException` when reading the GZIP header >> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an >> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` >> will now check the `InputStream` to be non-null and `size` to be `>0`, both >> of which were being done by the `super` constructor. >> >> Given the nature of the change, no new test has been added. Existing tests >> in `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 >> testing is in progress. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > rework the test to make it parameterized Thank you Jai for the update. I think this makes the test much cleaner - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19475#pullrequestreview-2090569778
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v3]
On Fri, 31 May 2024 10:09:20 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address the issue >> noted in https://bugs.openjdk.org/browse/JDK-8210471? >> >> `java.util.zip.Inflater` when instantiated will hold on the native resources >> which are freed only when `Inflater.end()` is called. The constructor of >> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal >> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor >> before returning from the constructor, can run into either a >> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` >> when trying to read a GZIP header from the underlying `InputStream`. In >> either of these cases, the `Inflater` that the `GZIPInputStream` created >> internally will end up leaking and the caller has no way to `end()` that >> `Inflater` or even knowledge of that `Inflater`. >> >> The commit in this PR catches the `IOException` when reading the GZIP header >> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an >> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` >> will now check the `InputStream` to be non-null and `size` to be `>0`, both >> of which were being done by the `super` constructor. >> >> Given the nature of the change, no new test has been added. Existing tests >> in `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 >> testing is in progress. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > enhance the new test to even verify IOException from the constructor Looks fine overall Jai. Personally, I would have had each assertThrows as its own test, as if one fails(which should not in this case) then the rest of the assertions are never executed until the first failure is addressed. - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19475#pullrequestreview-2090494111
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]
I was looking at jdk 22 and now see the npe was added to the constructor
specification earlier this year and I reviewed it
Sent from my iPad
> On May 30, 2024, at 8:56 PM, Jaikiran Pai wrote:
>
> On Thu, 30 May 2024 14:56:00 GMT, Lance Andersen wrote:
>
>>> Jaikiran Pai has updated the pull request incrementally with one additional
>>> commit since the last revision:
>>>
>>> introduce a basic test for GZIPInputStream
>>
>> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 97:
>>
>>> 95: */
>>> 96: private static Inflater createInflater(InputStream in, int size) {
>>> 97: Objects.requireNonNull(in);
>>
>> I don't believe we currently indicate at at NPE will be thrown if the
>> InputStream is null so this would require a CSR if we need to document it
>
> Hello Lance, both the constructors of `GZIPInputStream` already state that
> they throw a NullPointerException when the input stream is null:
>
>
> * @throwsNullPointerException if {@code in} is null
>
> It was already being thrown from within the super constructor. To prevent
> creation of a `Inflater` when `in` is null, I had to add this check here.
>
> -
>
> PR Review Comment:
> https://git.openjdk.org/jdk/pull/19475#discussion_r1621554479
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v2]
On Thu, 30 May 2024 12:32:22 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to address the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8210471?
>>
>> `java.util.zip.Inflater` when instantiated will hold on the native resources
>> which are freed only when `Inflater.end()` is called. The constructor of
>> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal
>> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor
>> before returning from the constructor, can run into either a
>> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException`
>> when trying to read a GZIP header from the underlying `InputStream`. In
>> either of these cases, the `Inflater` that the `GZIPInputStream` created
>> internally will end up leaking and the caller has no way to `end()` that
>> `Inflater` or even knowledge of that `Inflater`.
>>
>> The commit in this PR catches the `IOException` when reading the GZIP header
>> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an
>> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)`
>> will now check the `InputStream` to be non-null and `size` to be `>0`, both
>> of which were being done by the `super` constructor.
>>
>> Given the nature of the change, no new test has been added. Existing tests
>> in `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3
>> testing is in progress.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> introduce a basic test for GZIPInputStream
Looks good overall, but I believe we need to address the comment below unless I
missed something
src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 97:
> 95: */
> 96: private static Inflater createInflater(InputStream in, int size) {
> 97: Objects.requireNonNull(in);
I don't believe we currently indicate at at NPE will be thrown if the
InputStream is null so this would require a CSR if we need to document it
-
PR Review: https://git.openjdk.org/jdk/pull/19475#pullrequestreview-2088514194
PR Review Comment: https://git.openjdk.org/jdk/pull/19475#discussion_r1620877576
Re: RFR: 8330542: Template for Creating Strict JAXP Configuration File [v14]
On Fri, 24 May 2024 16:36:32 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >>> jaxp-compat.properties: used to regain compatibility from any more >>> restricted configuration than previous versions such as JDK 22 >> >> Updated on 5/16/2024 >> >> Design change: >> The design is changed to include in the JDK two configuration files that are >> the default jaxp.properties and jaxp-strict.properties, instead of three, >> dropping jaxp-compat.properties. >> >> Updated on 5/18/2024 >> >> Withdraw changes to jaxp.properties. The original idea was to match >> jaxp-strict.properties with regard to the properties. However, that change >> impact the configuration process, resulting in tests that verify the process >> to fail. >> >> Updated on 5/23/2024 >> >> Provide a template `jaxp-strict.template` instead of a properties file. This >> template can be used to create custom configuration files. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > rename the template to jaxp-strict.properties.template Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2077722644
Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v7]
On Thu, 16 May 2024 18:53:40 GMT, Joe Wang wrote:
>> Add two sample configuration files:
>>
>> jaxp-strict.properties: used to set strict configuration, stricter than
>> jaxp.properties in previous versions such as JDK 22
>>
>>> jaxp-compat.properties: used to regain compatibility from any more
>>> restricted configuration than previous versions such as JDK 22
>>
>> Updated 5/16/2024
>>
>> Design change:
>> The design is changed to include in the JDK two configuration files that are
>> the default jaxp.properties and jaxp-strict.properties, instead of three,
>> dropping jaxp-compat.properties.
>
> Joe Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> The JDK provides two configuration files instead of three. Updated
> jaxp-strict.properties to reflect the change. Removed jaxp-compat. Updated
> jaxp.properties with properties the same as in jaxp-strict, setting to
> default values.
src/java.xml/share/classes/module-info.java line 409:
> 407: * {@code jaxp.properties}
> 408: * {@code
> jaxp-strict.properties}
> 409: * {@code
> jaxp-compat.properties}
This line should be removed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1603907393
Re: RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory [v3]
On Wed, 1 May 2024 20:35:10 GMT, Justin Lu wrote: >> Please review this PR which converts _TestEncodingDecodingLength.java_ into >> a whitebox test which allows for testing to be done without memory usage >> issues. >> >> Currently, the test requires about ~2.75 `Integer.MAX_VALUE` sized byte >> arrays worth of memory. (2 for the initial array allocation, .75 for the >> target array in `decode()`). While the `-Xms6g -Xmx8g` options should >> address this, there have been intermittent memory issues, as the underlying >> machine machine may be running other tests simultaneously. >> >> By converting this test to a white-box test not only does it get rid of >> memory issues, but it also gets rid of the need to decode 2GB of data 3 >> times. The change is done using reflection to test the private visibility >> methods `encodedOutLength` and `decodedOutLength`, which the public `encode` >> and `decode` overloaded methods call respectively. > > Justin Lu has updated the pull request incrementally with one additional > commit since the last revision: > > reflect further review Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19036#pullrequestreview-2034414118
Re: RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory [v2]
On Wed, 1 May 2024 20:12:33 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflect review comments
>
> test/jdk/java/util/Base64/TestEncodingDecodingLength.java line 75:
>
>> 73: } catch (InvocationTargetException ex) {
>> 74: Throwable rootEx = ex.getCause();
>> 75: assertEquals(OutOfMemoryError.class, rootEx.getClass(),
>> "00ME should be thrown");
>
> Sorry if it is intentional, but I wonder if you meant "OOME" instead of
> "00ME" here?
Good catch, you sometimes see what you want to see
> test/jdk/java/util/Base64/TestEncodingDecodingLength.java line 100:
>
>> 98: m.invoke(DECODER, src, -LARGE_MEM_SIZE + 1, 1);
>> 99: } catch (InvocationTargetException ex) {
>> 100: fail("Decode should neither throw NASE or OOME: " +
>> ex.getCause());
>
> Should we check the cause is either NASE or OOME here?
The original test just validated there was no exception thrown, so I think we
can just update the message to indicate an unexpected Exception.
I had added this as a comment but must have missed hitting the comment button
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19036#discussion_r1586789013
PR Review Comment: https://git.openjdk.org/jdk/pull/19036#discussion_r1586788635
Re: RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory [v2]
On Wed, 1 May 2024 18:52:06 GMT, Justin Lu wrote: >> Please review this PR which converts _TestEncodingDecodingLength.java_ into >> a whitebox test which allows for testing to be done without memory usage >> issues. >> >> Currently, the test requires about ~2.75 `Integer.MAX_VALUE` sized byte >> arrays worth of memory. (2 for the initial array allocation, .75 for the >> target array in `decode()`). While the `-Xms6g -Xmx8g` options should >> address this, there have been intermittent memory issues, as the underlying >> machine machine may be running other tests simultaneously. >> >> By converting this test to a white-box test not only does it get rid of >> memory issues, but it also gets rid of the need to decode 2GB of data 3 >> times. The change is done using reflection to test the private visibility >> methods `encodedOutLength` and `decodedOutLength`, which the public `encode` >> and `decode` overloaded methods call respectively. > > 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/19036#pullrequestreview-2034254050
Re: RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory
On Wed, 1 May 2024 16:10:06 GMT, Justin Lu wrote:
> Please review this PR which converts _TestEncodingDecodingLength.java_ into a
> whitebox test which allows for testing to be done without memory usage issues.
>
> Currently, the test requires about ~2.75 `Integer.MAX_VALUE` sized byte
> arrays worth of memory. (2 for the initial array allocation, .75 for the
> target array in `decode()`). While the `-Xms6g -Xmx8g` options should address
> this, there have been intermittent memory issues, as the underlying machine
> machine may be running other tests simultaneously.
>
> By converting this test to a white-box test not only does it get rid of
> memory issues, but it also gets rid of the need to decode 2GB of data 3
> times. The change is done using reflection to test the private visibility
> methods `encodedOutLength` and `decodedOutLength`, which the public `encode`
> and `decode` overloaded methods call respectively.
Thank you for taking this on Justin and converting to a junit test as part of
your updates.
Overall it looks good on an initial pass.
Please see a few suggestions for your consideration
test/jdk/java/util/Base64/TestEncodingDecodingLength.java line 47:
> 45: public class TestEncodingDecodingLength {
> 46:
> 47: private static final int size = Integer.MAX_VALUE - 8;
Perhaps consider size-> SIZE and maybe tweak the name and add an additional
comment to its purpose
test/jdk/java/util/Base64/TestEncodingDecodingLength.java line 67:
> 65:
> 66: // OOME case
> 67: try {
Perhaps make this a separate test case so that the IAE test fails the OOME
still runs
-
PR Review: https://git.openjdk.org/jdk/pull/19036#pullrequestreview-2033944705
PR Review Comment: https://git.openjdk.org/jdk/pull/19036#discussion_r1586493998
PR Review Comment: https://git.openjdk.org/jdk/pull/19036#discussion_r1586499085
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v9]
On Fri, 26 Apr 2024 20:33:33 GMT, Archie Cobbs wrote: > > Is there any chance of backporting this to java 17 or 21? What's involved > > in doing that? > > It's a straightforward process but I'm not sure I'm one to judge whether it > would be appropriate. > > @jaikiran and/or @LanceAndersen - any opnions? I am not convinced this is a must have for a backport. Please see the[ OpenJDK developers guide regarding bacports.](https://openjdk.org/guide/#backporting). Also note this would require a CSR for each backport - PR Comment: https://git.openjdk.org/jdk/pull/17113#issuecomment-2080100566
Re: RFR: 8330686: Fix typos in the DatabaseMetaData javadoc [v2]
On Tue, 23 Apr 2024 09:32:40 GMT, Jin Kwon wrote: >> Fix typos within the `DatabaseMetaData.java`. > > Jin Kwon has updated the pull request incrementally with one additional > commit since the last revision: > > [JDK-8330686] Update copyright year Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18860#pullrequestreview-2020585117
Re: RFR: 8330542: Add two sample configuration files in preparation for a more secure by default configuration [v4]
On Tue, 23 Apr 2024 18:57:44 GMT, Sean Mullan wrote: > A few other comments/questions: > > Does this need a CSR since you are adding new property files? Not sure it does, but Joe will follow up with Joe Darcy > > Are there any tests to ensure the property files are working correctly? There are tests that Joe added as part of the JDK 22 work for custom config files > > Also, how does one try out these property files? Is there a system property > that needs to be set? Can you add more details to the RN on that? java -Djava.xml.config.file=$JAVA_HOME/conf/jaxp-compat.properties The property was added in JDK 22 see: https://docs.oracle.com/en/java/javase/22/docs/api/java.xml/module-summary.html#Conf_CF_SP - PR Comment: https://git.openjdk.org/jdk/pull/18831#issuecomment-2073248701
Re: RFR: 8330542: Add two sample configuration files in preparation for a more secure by default configuration [v4]
On Fri, 19 Apr 2024 21:55:09 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >> jaxp-compat.properties: used to regain compatibility from any more >> restricted configuration than previous versions such as JDK 22 > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > changes description of jaxp-compat and jaxp-strict after discussing with > Lance Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2018094529
Re: RFR: 8330542: Add two sample configuration files in preparation for a more secure by default configuration [v3]
On Fri, 19 Apr 2024 17:39:30 GMT, Joe Wang wrote: >> Add two sample configuration files: >> >> jaxp-strict.properties: used to set strict configuration, stricter than >> jaxp.properties in previous versions such as JDK 22 >> >> jaxp-compat.properties: used to regain compatibility from any more >> restricted configuration than previous versions such as JDK 22 > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > fix typo src/java.xml/share/conf/jaxp-compat.properties line 12: > 10: # > 11: # jaxp-strict.properties: this file resembles what will become the > Secure-By-Default > 12: # configuration where a strict restriction is the default. This file > allows strict restriction needs rewording. Perhaps something that indicates that this property file provides settings that will be equivalent to that will be the default JAXP settings in a future release to make the use of JAXP more secure out of the box src/java.xml/share/conf/jaxp-compat.properties line 20: > 18: # JDK has switched to a strict configuration as indicated in > jaxp-strict.properties. > 19: # This configuration contains the same properties as those in > jaxp-strict.properties > 20: # except it sets them back to the current status of the JDK. Note that, > although 'sets them back to the current status of the JDK' I think you are trying to indicate that this property file specifies the JAXP property values that were in place prior to being More Secure? - PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1572927195 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1572940722
Re: RFR: 8330542: Add two sample configuration files
On Wed, 17 Apr 2024 23:24:06 GMT, Joe Wang wrote: > Add two sample configuration files: > > jaxp-strict.properties: used to set strict configuration, stricter than > jaxp.properties in previous versions such as JDK 22 > > jaxp-compat.properties: used to regain compatibility from any more > restricted configuration than previous versions such as JDK 22 Hi Joe, Overall this looks fine though we need to clarify more as to the differences between jaxp-compat.properties vs jaxp.properties and also make it clearer why anyone would use jaxp.strict.properties As part of the review, I would suggest that a Release Note is created which will hopefully clarify when to use which file. Also when we actually change the defaults in a JDK release to be the equivalent of jaxp-strict.properties, that this file can be removed? If so this should be documented in the Release Note and perhaps a comment in the properties file itself src/java.xml/share/conf/jaxp-compat.properties line 10: > 8: # configuration, properties that have more restrictive settings as in the > 9: # strict configuration (jaxp-strict.properties) are reversed back to their > 10: # defaults. In particular: I think the above needs some more word smithing as we have not articulated what a strict configuration is or how this differs from jaxp.properties src/java.xml/share/conf/jaxp-compat.properties line 16: > 14: # > 15: # This configuration file can be used to reverse back to a working > environment > 16: # prior to any more restrictive configuration that may have been applied. How does this differ from jaxp.properties for JDK 23? I can understand for when we move to secure by default, we just need to be clear on the purpose of each jaxp properties files - PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2009160577 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1570932404 PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1570934511
Re: RFR: 8329787: Fix typo in CLDRConverter
On Fri, 5 Apr 2024 17:22:36 GMT, Naoto Sato wrote: > Fix a file/class name in the `CLDRConverter` build tool, with some clean-up > for a switch statement. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18662#pullrequestreview-1983860994
Re: CFV: New Core Libraries Group Member: Per-Ake Minborg
Vote: yes On Mar 19, 2024, at 11:19 AM, Daniel Fuchs wrote: Hi, I hereby nominate Per-Ake Minborg (pminborg) [1] to Membership in the Core Libraries Group [4]. Per-Ake is an OpenJDK Reviewer, a committer in the Leyden and Panama projects, and a member of Oracle’s Java Core Libraries team. Per-Ake has been actively participating in the development of the JDK and Panama projects for several years, and is one of the co-author of the Implementation of Foreign Function and Memory API (Third Preview) [2]. His contributions include more than 80 commits in the JDK [3] Votes are due by 16:00 UTC on April 2, 2024. Only current Members of the Core Libraries Group [4] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [5]. best regards, -- daniel [1] https://openjdk.org/census#pminborg [2] https://github.com/openjdk/jdk/commit/cbccc4c8172797ea2f1b7c301d00add3f517546d [3] https://github.com/search?q=repo%3Aopenjdk%2Fjdk+author%3Aminborg=commits [4] https://openjdk.org/census#core-libs [5] https://openjdk.org/groups/#member-vote [oracle_sig_logo.gif] Lance Andersen | Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: RFR: 8327167: Add add() example for Leap Day in Calendar's doc
On Thu, 7 Mar 2024 19:28:13 GMT, Naoto Sato wrote: > A simple doc update to include a leap day example in the `Calendar` class. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18158#pullrequestreview-1923575615
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes [v3]
On Sat, 3 Feb 2024 17:28:29 GMT, Eirik Bjørsnøs 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" > > 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 six additional > commits since the last revision: > > - Rename ZipFileSystem.Entry.posixPerms to externalFileAttributes > - For clarity, use "externalFileAttributes" instead of "externalAttributes" > - Merge branch 'master' into zipentry-external-attributes > - Update copyright years for 2024 > - Merge branch 'master' into zipentry-external-attributes > - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16952#pullrequestreview-1923567034
Re: RFR: 8327261: Parsing test for Double/Float succeeds w/o testing all bad cases [v2]
On Tue, 5 Mar 2024 17:26:11 GMT, Naoto Sato wrote: >> Fixing test cases. For bad test cases, only the first case was run, and the >> rest were ignored. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18113#pullrequestreview-1917875583
Re: RFR: 8327208: Remove unused method java.util.jar.Manifest.make72Safe
On Mon, 4 Mar 2024 09:55:09 GMT, Eirik Bjørsnøs wrote: > Please review this cleanup PR which suggests we remove the package-private, > unused and deprecated method `java.util.jar.Manifest.make72Safe`. > > This method was marked deprecated after a cleanup/refactoring in > [JDK-8066619](https://bugs.openjdk.org/browse/JDK-8066619) caused it to fall > out of use. It should rather be removed. > > Some tests reference the `make72Safe` methods in comment, or implement the > legacy versions of the same logic to simulate pre JDK 11 line break behavior > of `Manifest.write`. These comments and method names are adjusted to not > reference the now removed method. > > This change was initially discussed here: > https://mail.openjdk.org/pipermail/core-libs-dev/2024-February/119819.html Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18104#pullrequestreview-1915256398
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v12]
On Sat, 2 Mar 2024 18:51:16 GMT, Eirik Bjørsnøs wrote: >> Please review this PR which proposes that we officially deprecate the >> following four methods in the `java.util.zip` package: >> >> * `Inflater.getTotalIn()` >> * `Inflater.getTotalOut()` >> * `Deflater.getTotalIn()` >> * `Deflater.getTotalOut()` >> >> Since these legacy methods return `int`, they cannot safely return the >> number of bytes processed without the risk of losing information about the >> magnitude or even sign of the returned value. >> >> The corresponding methods `getBytesRead()` and `getBytesWritten()` methods >> introduced in Java 5 return `long`, and should be used instead when >> obtaining this information. >> >> Unrelated to the deprecation itself, the documentation currently does not >> specify what these methods are expected to return when the number of >> processed bytes is greater than `Integer.MAX_VALUE`. This PR aims to clarify >> this in the API specification. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Make the new specification text an @implSpec Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17919#pullrequestreview-1913187374
Re: RFR: 8326915: NPE when a validating parser is restricted [v2]
On Fri, 1 Mar 2024 18:02:24 GMT, Joe Wang wrote: >> Fix a NPE when a validating parser is restricted by the JDKCatalog resolve >> property. Also slightly improved the error msg with the catalog name. >> >> Test: new test added >> existing test CatalogSupport5 would fail without the Null check on >> ErrorReporter. It's a separate issue not covered by this fix. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > input.substring second argument not needed; proper test method name Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18071#pullrequestreview-1911761210
Re: RFR: 8326915: NPE when a validating parser is restricted
On Thu, 29 Feb 2024 21:00:09 GMT, Joe Wang wrote: > Fix a NPE when a validating parser is restricted by the JDKCatalog resolve > property. Also slightly improved the error msg with the catalog name. > > Test: new test added > existing test CatalogSupport5 would fail without the Null check on > ErrorReporter. It's a separate issue not covered by this fix. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18071#pullrequestreview-1910945957
Re: RFD: java.util.jar.Manifest.make72Safe has an invalid @deprecation tag
Hi Eirik, I also be we should be OK to remove. That being said, as part of the change, we should update open/test/jdk/sun/security/tools/jarsigner/PreserveRawManifestEntryAndDigest.java Best Lance On Feb 29, 2024, at 7:38 AM, Jaikiran Pai wrote: Hello Eirik, I think that method (and its references in some test code comments) can be removed. Like you note it's a package private method within the java.util.jar package and I don't see its usage (other than test code comments) anywhere within the JDK repo. That @Deprecated annotation on that method seems to have been introduced as part of https://bugs.openjdk.org/browse/JDK-8066619 (review thread https://mail.openjdk.org/pipermail/core-libs-dev/2018-December/057030.html). I'm guessing it likely was an oversight to have added that annotation to an internal method instead of just removing its usage. -Jaikiran On 29/02/24 3:09 am, Eirik Bjørsnøs wrote: Hi, The non-public method java.util.jar.Manifest.make72Safe is marked @Deprecated(since="13"). However, the Javadoc comment has a '@deprecation' tag which presumably should have been `@deprecated`. I first thought of proposing a PR to fix the invalid tag, but then I noticed that the method is non-public and has no callers in OpenJDK. Not sure I understand why this internal method was marked deprecated in the first place and not simply removed? Are we worried about people calling this reflectively? What would be the best way to go about this? (a) Do nothing (b) Just fix the tag @deprecation -> @deprecated (c) Fix the tag, add forRemoval=true (d) Just delete the method altogether (it's internal after all). Cheers, Eirik. [oracle_sig_logo.gif] Lance Andersen | Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Integrated: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc
On Mon, 26 Feb 2024 19:55:47 GMT, Lance Andersen wrote: > This PR updates the javadoc and comments within java.util.zip/jar and zipfs > module summary so that it is consistent with the use of "ZIP". > > In addition, open/src/java.base/share/classes/java/util/zip/package-info.java > has been updated to point to the higher level location of the PKWARE > APPNOTE.TXT has PKWare recently changed the direct path the the latest > version of the spec. > > It is also worth noting that error messages were not updated as part of the > PR and will be updated separately to keep the javadoc changes separate This pull request has now been integrated. Changeset: 38ad5145 Author:Lance Andersen URL: https://git.openjdk.org/jdk/commit/38ad514589764d16b312152474e2446c3339da39 Stats: 123 lines in 12 files changed: 0 ins; 0 del; 123 mod 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc Reviewed-by: dfuchs, jpai, gli - PR: https://git.openjdk.org/jdk/pull/18011
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
On Wed, 28 Feb 2024 14:03:58 GMT, Jaikiran Pai wrote: > GitHub doesn't allow me to comment on unchanged lines in the PR, but while > reviewing this I noticed 2 things: > > * It looks like http://www.pkware.com/documents/casestudies/APPNOTE.TXT is > now auto redirecting to > https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.9.TXT. I'm not sure > if it's temporary or a permanent thing. I cannot answer that specifically but we need to rely on PKWARE to keep this as accurate as possible. The link has been broken several times by PKWare so I think the best that we can do is keep checking. > * The other thing is we don't seem to have a `@spec` entry in > `src/java.base/share/classes/java/util/zip/package-info.java` pointing to > http://www.pkware.com/documents/casestudies/APPNOTE.TXT. We are pointing to > ZLIB RFC 1950. Do you think we should add a `@spec` entry to point to > http://www.pkware.com/documents/casestudies/APPNOTE.TXT? Of course as a > separate PR. Good Question, let's discuss and handle separately as I don't know the history of why it was not included - PR Comment: https://git.openjdk.org/jdk/pull/18011#issuecomment-1969151426
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
On Wed, 28 Feb 2024 13:58:01 GMT, Jaikiran Pai wrote: > Hello Lance, this doc-only change looks good to me. > > These changes are merely changing the case of `zip` and aren't changing any > specification of the APIs and that looks fine to me. > > I had imagined that we would be changing only the public API documentations > but I suspect you decided to update even code comments and internal class > comments to keep everything consistent. I am not opposing it and I ask this > so that I can use this decision in future when adding anything in this area > or reviewing code. Hi Jai, thank you. As part of updating the relevant class which contained public facing javadoc, I also updated the casing for comments as you point out for consistency. I did not want to go through all of the tests and non-public facing classes as part of this PR. - PR Comment: https://git.openjdk.org/jdk/pull/18011#issuecomment-1969122748
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
On Wed, 28 Feb 2024 14:13:57 GMT, Guoxiong Li wrote: > I search the package `java.util.zip` and find several `zip64` in the file > [ZipConstants64.java](https://github.com/openjdk/jdk/blob/baa67f1130947c7204fc12018d25bfb48528784c/src/java.base/share/classes/java/util/zip/ZipConstants64.java#L51). > It seems you didn't fix all the files in the package `java.util.zip/jar`. Is > it your intent? Or you miss some files? The intent of the change is to make the javadoc consistent. There were a couple of files that Eirik mentioned that I addressed but as I indicated in my reply, unless there is a missing javadoc update, I do not plan to address other areas with this PR but can/will in a following - PR Comment: https://git.openjdk.org/jdk/pull/18011#issuecomment-1969116524
Re: RFR: 8310837: Use ByteArrayLittleEndian in java.util.zip [v7]
On Tue, 16 Jan 2024 16:38:34 GMT, Glavo wrote: >> Using `ByteArrayLittleEndian` is simpler and faster. >> >> `make test TEST="micro:java.util.zip.ZipFileOpen"`: >> >> >> Benchmark (size) Mode Cnt Score Error >> Units >> - ZipFileOpen.openCloseZipFile 512 avgt 15 39052.832 ± 107.496 >> ns/op >> + ZipFileOpen.openCloseZipFile 512 avgt 15 36275.539 ± 663.193 >> ns/op >> - ZipFileOpen.openCloseZipFile1024 avgt 15 77106.494 ± 4159.300 >> ns/op >> + ZipFileOpen.openCloseZipFile1024 avgt 15 71955.013 ± 2296.050 >> ns/op > > Glavo has updated the pull request incrementally with one additional commit > since the last revision: > > Add tests As mentioned earlier, it would be beneficial to measure the potential benefit of this proposed change with ZipFS which has similar albeit slightly different approach to the macros and include as part of this PR of a follow-on PR - PR Comment: https://git.openjdk.org/jdk/pull/14632#issuecomment-1968951462
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v11]
On Wed, 28 Feb 2024 09:19:20 GMT, Eirik Bjørsnøs wrote: >> Please review this PR which proposes that we officially deprecate the >> following four methods in the `java.util.zip` package: >> >> * `Inflater.getTotalIn()` >> * `Inflater.getTotalOut()` >> * `Deflater.getTotalIn()` >> * `Deflater.getTotalOut()` >> >> Since these legacy methods return `int`, they cannot safely return the >> number of bytes processed without the risk of losing information about the >> magnitude or even sign of the returned value. >> >> The corresponding methods `getBytesRead()` and `getBytesWritten()` methods >> introduced in Java 5 return `long`, and should be used instead when >> obtaining this information. >> >> Unrelated to the deprecation itself, the documentation currently does not >> specify what these methods are expected to return when the number of >> processed bytes is greater than `Integer.MAX_VALUE`. This PR aims to clarify >> this in the API specification. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Update method description and deprecation note for the three remaining > methods Looks good Eirik, thank you for your efforts on the doc updates - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17919#pullrequestreview-1906144415
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v10]
On Tue, 27 Feb 2024 20:01:30 GMT, Eirik Bjørsnøs wrote:
> After some offline discussion about the redundant text, we decided to keep to
> the new method description text, but reduce the `@deprecated` note to simply
> refer to the replacement method:
>
> ```
> /**
> * Returns the total number of compressed bytes input so far.
> *
> * This method returns the equivalent of {@code (int) getBytesRead()}
> * and therefore cannot return the correct value when it is greater
> * than {@link Integer#MAX_VALUE}.
> *
> * @deprecated Use {@link #getBytesRead()} instead
> *
> * @return the total number of compressed bytes input so far
> */
> ```
>
> Barring any objections, I'll to go ahead with the update of the rest of the
> methods and update the CSR draft tomorrow.
Yes please move forward and thank you
-
PR Comment: https://git.openjdk.org/jdk/pull/17919#issuecomment-1967517931
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
> This PR updates the javadoc and comments within java.util.zip/jar and zipfs > module summary so that it is consistent with the use of "ZIP". > > In addition, open/src/java.base/share/classes/java/util/zip/package-info.java > has been updated to point to the higher level location of the PKWARE > APPNOTE.TXT has PKWare recently changed the direct path the the latest > version of the spec. > > It is also worth noting that error messages were not updated as part of the > PR and will be updated separately to keep the javadoc changes separate Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Address initial feedback for JDK-8326687 - Changes: - all: https://git.openjdk.org/jdk/pull/18011/files - new: https://git.openjdk.org/jdk/pull/18011/files/0208b860..baa67f11 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18011=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18011=00-01 Stats: 10 lines in 4 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18011/head:pull/18011 PR: https://git.openjdk.org/jdk/pull/18011
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
On Tue, 27 Feb 2024 16:12:03 GMT, Lance Andersen wrote: >> This PR updates the javadoc and comments within java.util.zip/jar and zipfs >> module summary so that it is consistent with the use of "ZIP". >> >> In addition, >> open/src/java.base/share/classes/java/util/zip/package-info.java has been >> updated to point to the higher level location of the PKWARE APPNOTE.TXT has >> PKWare recently changed the direct path the the latest version of the spec. >> >> It is also worth noting that error messages were not updated as part of the >> PR and will be updated separately to keep the javadoc changes separate > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Address initial feedback for JDK-8326687 Updated ZipOutputStream.java and the other file you mentioned. I intentionally did not update any files which were not generating javadoc. We can certainly address any additional files and tests not related to javadoc via a separate issue. Thank you Eirik and Daniel for the review - PR Review: https://git.openjdk.org/jdk/pull/18011#pullrequestreview-1903973731
Re: RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc [v2]
On Tue, 27 Feb 2024 15:08:11 GMT, Eirik Bjørsnøs wrote: >> Lance Andersen has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address initial feedback for JDK-8326687 > > src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 54: > >> 52: * Whether to use ZIP64 for zip files with more than 64k entries. >> 53: * Until ZIP64 support in zip implementations is ubiquitous, this >> 54: * system property allows the creation of zip files which can be > > Suggestion: > > * Whether to use ZIP64 for ZIP files with more than 64k entries. > * Until ZIP64 support in ZIP implementations is ubiquitous, this > * system property allows the creation of ZIP files which can be Fixed not sure how Missed this when updating ZipOutputStream - PR Review Comment: https://git.openjdk.org/jdk/pull/18011#discussion_r1504509576
RFR: 8326687: Inconsistent use of "ZIP", "Zip" and "zip" in java.util.zip/jar zipfs javadoc
This PR updates the javadoc and comments within java.util.zip/jar and zipfs module summary so that it is consistent with the use of "ZIP". In addition, open/src/java.base/share/classes/java/util/zip/package-info.java has been updated to point to the higher level location of the PKWARE APPNOTE.TXT has PKWare recently changed the direct path the the latest version of the spec. It is also worth noting that error messages were not updated as part of the PR and will be updated separately to keep the javadoc changes separate - Commit messages: - Initial update for JDK-8326687 Changes: https://git.openjdk.org/jdk/pull/18011/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18011=00 Issue: https://bugs.openjdk.org/browse/JDK-8326687 Stats: 113 lines in 9 files changed: 0 ins; 0 del; 113 mod Patch: https://git.openjdk.org/jdk/pull/18011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18011/head:pull/18011 PR: https://git.openjdk.org/jdk/pull/18011
Re: RFR: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater [v8]
On Mon, 26 Feb 2024 14:29:19 GMT, Eirik Bjørsnøs wrote:
>> Please review this PR which proposes that we officially deprecate the
>> following four methods in the `java.util.zip` package:
>>
>> * `Inflater.getTotalIn()`
>> * `Inflater.getTotalOut()`
>> * `Deflater.getTotalIn()`
>> * `Deflater.getTotalOut()`
>>
>> Since these legacy methods return `int`, they cannot safely return the
>> number of bytes processed without the risk of losing information about the
>> magnitude or even sign of the returned value.
>>
>> The corresponding methods `getBytesRead()` and `getBytesWritten()` methods
>> introduced in Java 5 return `long`, and should be used instead when
>> obtaining this information.
>>
>> Unrelated to the deprecation itself, the documentation currently does not
>> specify what these methods are expected to return when the number of
>> processed bytes is higher than `Integer.MAX_VALUE`. This PR aims to clarify
>> this in the API specification.
>>
>> Initally, this PR handles only `Inflater.getTotalIn()`. The other three
>> methods will be updated once the wordsmithing for this method stabilizes.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reduce new method description text to one sentence
Sorry for the delay, but finally had a chance to take a look at your proposed
wording
src/java.base/share/classes/java/util/zip/Inflater.java line 642:
> 640: *
> 641: * This method returns the equivalent of {@code (int) getBytesRead()}
> 642: * and therefore cannot return the correct number of compressed bytes
This sentence to me seems a bit hard to digest and you basically get the point
across in the `@deprecated` message
Perhaps add `@see getBytesRead`
-
PR Review: https://git.openjdk.org/jdk/pull/17919#pullrequestreview-1901832690
PR Review Comment: https://git.openjdk.org/jdk/pull/17919#discussion_r1503191920
Integrated: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment
On Sat, 24 Feb 2024 14:56:01 GMT, Lance Andersen wrote: > Please review this PR which addresses the handling of invalid UTF-8 byte > sequences in the entry name of a LOC file header and a Zip file comment which > is returned via ZipFile::getComment. > > As part of the change, `ZipFile::getComment` will now return `null` if an > invalid UTF-8 byte sequence is encountered while converting the byte array to > a String. The CSR for this change has also been approved. > > Mach5 tiers 1-3 are clean with this change. This pull request has now been integrated. Changeset: 20c71cea Author:Lance Andersen URL: https://git.openjdk.org/jdk/commit/20c71ceacdcb791f5b70cda456bdc47bdd9acf6c Stats: 289 lines in 3 files changed: 176 ins; 16 del; 97 mod 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment Reviewed-by: jpai, alanb - PR: https://git.openjdk.org/jdk/pull/17995
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v3]
On Sun, 25 Feb 2024 15:26:35 GMT, Alan Bateman wrote: > (In passing, the casing of "zip file" is very inconsistent in the javadoc, > it's "ZIP file" in some places, "zip file" in others, the change here uses > "Zip file"). Thank you for pointing out the discrepancy above. Let's decide on which way to go and I will update under a separate PR. I would suggest "ZIP file" based on the PKWare APPNOTE.TXT, but the man pages for info-zip command _zip_ command uses "zip file" Apache commons also uses "ZIP file" or "Zip archive" - PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501878600
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sun, 25 Feb 2024 12:20:36 GMT, Eirik Bjørsnøs wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Updates based on 1st round of feedback
>
> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 524:
>
>> 522: : zc.toString(b, len);
>> 523: } catch(Exception ex) {
>> 524: throw (ZipException) new ZipException(
>
> Whitespace nit:
>
> Suggestion:
>
> } catch (Exception ex) {
> throw (ZipException) new ZipException(
fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501822894
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v3]
> Please review this PR which addresses the handling of invalid UTF-8 byte > sequences in the entry name of a LOC file header and a Zip file comment which > is returned via ZipFile::getComment. > > As part of the change, `ZipFile::getComment` will now return `null` if an > invalid UTF-8 byte sequence is encountered while converting the byte array to > a String. The CSR for this change has also been approved. > > Mach5 tiers 1-3 are clean with this change. Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Updates based on 2nd round of feedback - Changes: - all: https://git.openjdk.org/jdk/pull/17995/files - new: https://git.openjdk.org/jdk/pull/17995/files/dfd49101..637acee4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17995=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17995=01-02 Stats: 14 lines in 3 files changed: 6 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17995.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17995/head:pull/17995 PR: https://git.openjdk.org/jdk/pull/17995
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sun, 25 Feb 2024 12:16:14 GMT, Jaikiran Pai wrote:
> Hello Lance, the changes look fine to me. `ZipFile.java` file will need a
> copyright year update.
Thank you Jai, updated
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 189:
>
>> 187: try (ZipFile zf = new ZipFile(ZIP_FILE.toFile())) {
>> 188: var comment = zf.getComment();
>> 189: System.out.printf("Comment= %s%n", comment);
>
> Should we assert for the valid expected `comment` here?
Sure I can add the validation
-
PR Comment: https://git.openjdk.org/jdk/pull/17995#issuecomment-1962954345
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501822988
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sat, 24 Feb 2024 18:54:07 GMT, Lance Andersen wrote: >> Please review this PR which addresses the handling of invalid UTF-8 byte >> sequences in the entry name of a LOC file header and a Zip file comment >> which is returned via ZipFile::getComment. >> >> As part of the change, `ZipFile::getComment` will now return `null` if an >> invalid UTF-8 byte sequence is encountered while converting the byte array >> to a String. The CSR for this change has also been approved. >> >> Mach5 tiers 1-3 are clean with this change. > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Updates based on 1st round of feedback Thanks for the feedback. Updated accordingly - PR Review: https://git.openjdk.org/jdk/pull/17995#pullrequestreview-1899679545
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment
On Sat, 24 Feb 2024 17:15:02 GMT, Eirik Bjørsnøs wrote: > Since the CSR is already approved, I'll add a question here: > > `ZipFile` performs a lot of validation while opening ZIP files, including > throwning ZipException for invalid entry names or comments. Why handle the > ZIP file comment differently (lazily)? If this comment was also validated by > the constructor, then the API change for ZipFile::getComment would not be > needed. > > Do we have reason to belive the encoding quality of ZIP file comments is less > reliable than that of ZIP entry comments? Or is there some other reason this > validation is done lazily? Yes, there are some libraries/tools that are using the Zip file comment for its own purposes such as idea_rt.jar which is part of IntelliJ. - PR Comment: https://git.openjdk.org/jdk/pull/17995#issuecomment-1962571347
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sat, 24 Feb 2024 16:57:50 GMT, Eirik Bjørsnøs wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Updates based on 1st round of feedback
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 331:
>
>> 329: try {
>> 330: return res.zsrc.zc.toString(res.zsrc.comment);
>> 331: } catch(IllegalArgumentException iae) {
>
> Suggestion:
>
> } catch (IllegalArgumentException iae) {
fixed
> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 526:
>
>> 524: throw (ZipException) new ZipException(
>> 525: "invalid LOC header (bad entry
>> name)").initCause(ex);
>> 526: }
>
> There is a lot going on here:
>
> * The creation of a ZipEntry
> * The interpretation of the general purpose bit flag
> * The split to call either a static or non-static toString depending on the
> flag interpretation
> * The exception handling, including the somewhat unusual initCause and cast
>
> The comment at 517 seems even more detached from the logic it tries to
> describe after this change.
>
> Perhaps we could benefit from introducing a `zipCoderFromFlag(int flag)`
> method, similar to that in ZipFile?
>
> This would allow the section to be rewritten to something that hide the flag
> parsing details, leaves us with a single (polymorphic) toString invocation
> and extracts `createZipEntry` from the try/catch scope.
>
> Something like this: (I also changed the exception handling here, that's a
> bit orthogonal to the main points above)
>
> Suggestion:
>
> String name;
> try {
> name = zipCoderForFlag(flag).toString(b, len);
> } catch (IllegalArgumentException ex) {
> ZipException e = new ZipException("invalid LOC header (bad entry
> name)");
> e.initCause(ex);
> throw e;
> }
> ZipEntry e = createZipEntry(name);
I believe you meant `ZipCoderFromPos(int flag)`. I don't think it is needed in
ZipInputStream but I did move the call to createZipEntry out of the try block
per your suggestion
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 49:
>
>> 47: * @summary Validate that a ZipException is thrown when opening a ZIP
>> file via
>> 48: * ZipFile, traversing a Zip File via ZipInputStream, with invalid UTF-8
>> 49: * byte sequences in the name or comment fields fails with ZipException.
>
> The leading summary sentence needs a cleanup. Here's one suggestion:
>
> Suggestion:
>
> * @summary Validate that a ZipException is thrown when a ZIP file with
> * invalid UTF-8 byte sequences in the name or comment fields is opened via
> * ZipFile or traversed via ZipInputStream.
Updated
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 60:
>
>> 58: private static final byte[] INVALID_UTF8_BYTE_SEQUENCE = {(byte)
>> 0xF0, (byte) 0xA4, (byte) 0xAD};
>> 59: // Expected error message when an invalid entry name or entry
>> comment is
>> 60: // encountered when accessing a CEN Header
>
> This two-line comment could benefit from an earlier line break.
The break makes sense as is in IntelliJ, so please clarify
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 64:
>
>> 62:
>> 63: // Expected error message when an invalid entry name is encountered
>> when
>> 64: // accessing a LOC Header
>
> This two-line comment could benefit from an earlier line break.
Same comment as above
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 65:
>
>> 63: // Expected error message when an invalid entry name is encountered
>> when
>> 64: // accessing a LOC Header
>> 65: private static final String LOC_BAD_ENTRY_NAME_OR_COMMENT = "invalid
>> LOC header (bad entry name)";
>
> Should this be renamed `LOC_BAD_ENTRY_NAME`?
Yep, I thought I made that change but must have just thought about it and never
went back to it
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 128:
>
>> 126: * 0x93: Comment : [ZipFileComment]
>> 127: */
>> 128: public static byte[] VALID_ZIP = {
>
> I see this byte array encoding of ZIP files is a pattern used across tests.
> My preference would be to encode this in Base64 or hex, since that would make
> the consumption by external command line tools easier and the encoded
> representation somewhat prettier.
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
On Sat, 24 Feb 2024 18:51:22 GMT, Lance Andersen wrote: >> Please review this PR which addresses the handling of invalid UTF-8 byte >> sequences in the entry name of a LOC file header and a Zip file comment >> which is returned via ZipFile::getComment. >> >> As part of the change, `ZipFile::getComment` will now return `null` if an >> invalid UTF-8 byte sequence is encountered while converting the byte array >> to a String. The CSR for this change has also been approved. >> >> Mach5 tiers 1-3 are clean with this change. > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Updates based on 1st round of feedback Thank you for your input Eirik, I have pushed updates to address the suggestions you raised - PR Review: https://git.openjdk.org/jdk/pull/17995#pullrequestreview-1899544786
Re: RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment [v2]
> Please review this PR which addresses the handling of invalid UTF-8 byte > sequences in the entry name of a LOC file header and a Zip file comment which > is returned via ZipFile::getComment. > > As part of the change, `ZipFile::getComment` will now return `null` if an > invalid UTF-8 byte sequence is encountered while converting the byte array to > a String. The CSR for this change has also been approved. > > Mach5 tiers 1-3 are clean with this change. Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Updates based on 1st round of feedback - Changes: - all: https://git.openjdk.org/jdk/pull/17995/files - new: https://git.openjdk.org/jdk/pull/17995/files/e76ff749..dfd49101 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17995=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17995=00-01 Stats: 24 lines in 3 files changed: 1 ins; 2 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/17995.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17995/head:pull/17995 PR: https://git.openjdk.org/jdk/pull/17995
RFR: 8321156: Improve the handling of invalid UTF-8 byte sequences for ZipInputStream::getNextEntry and ZipFile::getComment
Please review this PR which addresses the handling of invalid UTF-8 byte sequences in the entry name of a LOC file header and a Zip file comment which is returned via ZipFile::getComment. As part of the change, `ZipFile::getComment` will now return `null` if an invalid UTF-8 byte sequence is encountered while converting the byte array to a String. The CSR for this change has also been approved. Mach5 tiers 1-3 are clean with this change. - Commit messages: - Add tests for JDK-8321156 - Merge branch 'master' into JDK-8321156 - Initial changes for JDK-8321156 Changes: https://git.openjdk.org/jdk/pull/17995/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17995=00 Issue: https://bugs.openjdk.org/browse/JDK-8321156 Stats: 283 lines in 3 files changed: 171 ins; 16 del; 96 mod Patch: https://git.openjdk.org/jdk/pull/17995.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17995/head:pull/17995 PR: https://git.openjdk.org/jdk/pull/17995
Re: RFR: 8278527: java/util/concurrent/tck/JSR166TestCase.java fails nanoTime test
On Thu, 22 Feb 2024 09:49:31 GMT, Jaikiran Pai wrote: > Can I get a review of this change which proposes to remove the `SystemTest` > from among the `JSR166TestCase`? > > As noted in https://bugs.openjdk.org/browse/JDK-8278527 the > `SystemTest.nanoTime` test has been intermittently failing since a few years > now. Martin and Paul have investigated this failure in the past and they note > that this test is fragile and should be removed: > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14463658=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14463658 > > https://bugs.openjdk.org/browse/JDK-8278527?focusedId=14508958=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14508958 > > The commit in this PR removes the `SystemTest` that was enrolled into > `JSR166TestCase`. > > tier1 testing went fine with this change. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17960#pullrequestreview-1896400193
Integrated: 8326351: Update the Zlib version in open/src/java.base/share/legal/zlib.md to 1.3.1
On Tue, 20 Feb 2024 19:56:53 GMT, Lance Andersen wrote: > Please review the updates to open/src/java.base/share/legal/zlib.md to update > the file from zlib 1.2.13 to zlib 1.3.1 which was missed as part of the PR > for [JDK-8324632](https://bugs.openjdk.org/browse/JDK-8324632) This pull request has now been integrated. Changeset: f0f4d63f Author: Lance Andersen URL: https://git.openjdk.org/jdk/commit/f0f4d63fa9c9f487198b2a2b7b410b590e1437bc Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod 8326351: Update the Zlib version in open/src/java.base/share/legal/zlib.md to 1.3.1 Reviewed-by: iris, naoto, jpai - PR: https://git.openjdk.org/jdk/pull/17935
RFR: 8326351: Update the Zlib version in open/src/java.base/share/legal/zlib.md to 1.3.1
Please review the updates to open/src/java.base/share/legal/zlib.md to update the file from zlib 1.2.13 to zlib 1.3.1 which was missed as part of the PR for [JDK-8324632](https://bugs.openjdk.org/browse/JDK-8324632) - Commit messages: - Update zlib.md for zlib 1.3.1 Changes: https://git.openjdk.org/jdk/pull/17935/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17935=00 Issue: https://bugs.openjdk.org/browse/JDK-8326351 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17935.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17935/head:pull/17935 PR: https://git.openjdk.org/jdk/pull/17935
Re: RFR: 8326158: Javadoc for java.time.DayOfWeek#minus(long)
On Tue, 20 Feb 2024 18:34:59 GMT, Naoto Sato wrote: > Fixing a typo in the said method. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17933#pullrequestreview-1891265805
Re: CFV: New Core Libraries Group Member: Viktor Klang
Vote: yes On 2/19/24 3:40 PM, Stuart Marks wrote: > I hereby nominate Viktor Klang [6] to Membership in the Core Libraries Group > [4]. > Viktor has been a member of the Java team in Oracle since 2022, and he is > currently a Committer on the JDK project. Viktor has led JEP 461 [1], an > enhancement to the Streams API to support new intermediate stream operations. > Viktor has also contributed several other changes to the JDK [2][3], focusing > primarily on the java.util.concurrent package. Prior to his work on OpenJDK, > Viktor's past work includes Akka, Reactive Streams, and many other > contributions to the Scala world. > Votes are due by 23:59 UTC on March 4, 2024. > Only current Members of the Core Libraries Group [4] are eligible to vote on > this nomination. Votes must be cast in the open by replying to this mailing > list. > For Lazy Consensus voting instructions, see [5]. > s'marks > [1] https://openjdk.org/jeps/461 > [2] https://github.com/openjdk/jdk/commits?author=vklang%40openjdk.org > [3] https://github.com/openjdk/jdk/commits?author=viktorklang-ora > [4] https://openjdk.org/census#core-libs > [5] https://openjdk.org/groups/#member-vote > [6] https://openjdk.org/census#vklang
Re: CFV: New Core Libraries Group Member: Raffaello Giulietti
Vote: yes On Feb 13, 2024, at 3:25 PM, Brian Burkhalter wrote: I hereby nominate Raffaello Giulietti to Membership in the Core Libraries Group. Raffaello has been working in the Core Library team at Oracle since April, 2022. He has authored more than 50 contributions to OpenJDK in a number of areas including numerics, formatting, regular expressions, and random number generation. Votes are due by Wednesday, February 28, 2023. Only current Members of the Core Libraries Group [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list For Lazy Consensus voting instructions, see [2]. Brian Burkhalter [1] https://openjdk.org/census#core-libs [2] https://openjdk.org/groups/#member-vote [oracle_sig_logo.gif] Lance Andersen | Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v16]
On Mon, 5 Feb 2024 13:14:39 GMT, Eirik Bjørsnøs 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 AND at least one of the >> 'compressed size' and 'uncompressed size' have the expected Zip64 "magic" >> value 0x. 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 Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 230 commits: > > - Update readZipInputStream to verify that the ZipInputStream finds a single > zip entry with the expected contents > - Merge branch 'master' into data-descriptor > - Merge branch 'master' into data-descriptor > - Update comment of expect64BitDataDescriptor to reflect relaxed validation > - Dial down validation of the Zip64 extra field > - 8321712: C2: "failed: Multiple uses of register" in > C2_MacroAssembler::vminmax_fp > >Co-authored-by: Volodymyr Paprotski >Reviewed-by: kvn, thartmann, epeter, jbhateja > - 8319128: sun/security/pkcs11 tests fail on OL 7.9 aarch64 > >Reviewed-by: mbaesken > - 8322971: KEM.getInstance() should check if a 3rd-party security provider > is signed > >Reviewed-by: mullan, valeriep > - 8320890: [AIX] Find a better way to mimic dl handle equality > >Reviewed-by: stuefe, mdoerr > - 8323276: StressDirListings.java fails on AIX > >Reviewed-by: jpai, dfuchs > - ... and 220 more: https://git.openjdk.org/jdk/compare/692c9f88...e8d3b904 The latest updates seem OK. Thank you Eirik - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12524#pullrequestreview-1876488622
Re: RFR: 8303891: Speed up Zip64SizeTest using a small ZIP64 file [v8]
On Fri, 9 Feb 2024 12:31:27 GMT, Eirik Bjørsnøs wrote: >> Please review this PR which suggests we speed up the `Zip64SizeTest` using a >> small-sized ZIP64 ZIP file specifically created to reproduce the issue being >> tested. >> >> The disk space requirement of this test is known to cause problems in some >> builds, see [JDK-8259866](https://bugs.openjdk.org/browse/JDK-8259866) >> >> By using a sparse file, we reduce consumed disk space from 5GB to 266 bytes >> and also reduce the runtime from ~35 seconds to ~1 seconds on my Macbook Pro. >> >> The PR also fixes the `@summary` tag, which seems to have been copied from >> an unrelated test. > > Eirik Bjørsnøs has updated the pull request incrementally with two additional > commits since the last revision: > > - Use three dots consistently for representing an ellipsis > - Add zipdetails of the Zip64 extra field in updateCENHeaderToZip64 Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/12948#pullrequestreview-1872850714
Re: RFR: 8303891: Speed up Zip64SizeTest using a small ZIP64 file [v6]
On Fri, 9 Feb 2024 10:33:30 GMT, Eirik Bjørsnøs wrote:
>> Please review this PR which suggests we speed up the `Zip64SizeTest` using a
>> small-sized ZIP64 ZIP file specifically created to reproduce the issue being
>> tested.
>>
>> The disk space requirement of this test is known to cause problems in some
>> builds, see [JDK-8259866](https://bugs.openjdk.org/browse/JDK-8259866)
>>
>> By using a sparse file, we reduce consumed disk space from 5GB to 266 bytes
>> and also reduce the runtime from ~35 seconds to ~1 seconds on my Macbook Pro.
>>
>> The PR also fixes the `@summary` tag, which seems to have been copied from
>> an unrelated test.
>
> Eirik Bjørsnøs has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Spell fix for "Contents"
> - Add comments to explain how we temporarily use the 'unknown' tag to
> prevent ZipEntry.setExtra0 from processing the extra field.
thank you for the latest updates Eirik.
Overall it is good a few more comment suggestions for extra clarity.
test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java line 114:
> 112:
> 113: // Make an extra field with the correct size for an 8-byte
> 'uncompressed size'
> 114: // Zip64 field. Temporarily use the 'unknown' tag 0x9902 to
> make
I would suggest adding a reference to the following in the APPNOTE.TXT to make
clearer where that value came from
> 4.6.1 Third party mappings commonly used are:
another suggestion would be to show the CEN here with this change I think would
make it easier for someone who is not as familiar with APPNOTE.TXT
test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java line 140:
> 138: * @param zip the ZIP file to update to ZIP64
> 139: */
> 140: private static void updateCENHeaderToZip64(byte[] zip) {
Again minor but perhaps articulate that after this call, the extra data will
be(obvious to you and I but probably not to others...)
> * 00B4 Extra ID #00010001 'ZIP64'
> * 00B6 Length 0008
> * 00B8 Uncompressed Size 0005
-
Marked as reviewed by lancea (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/12948#pullrequestreview-1872219560
PR Review Comment: https://git.openjdk.org/jdk/pull/12948#discussion_r1484225117
PR Review Comment: https://git.openjdk.org/jdk/pull/12948#discussion_r1484229268
Re: RFR: 8325304: Several classes in java.util.jar and java.util.zip don't specify the behaviour for null arguments [v7]
On Wed, 7 Feb 2024 12:37:23 GMT, Jaikiran Pai wrote: >> Can I please get a review of this doc-only change which updates the javadoc >> of several classes in `java.util.jar` and `java.util.zip` to specify their >> behaviour when `null` arguments are passed to the constructor or methods of >> those classes? >> >> For these updated classes, I have individually checked that they indeed >> throw a `NullPointerException` when `null` is passed to their constructor or >> methods. The couple of places where `null` is accepted have been updated to >> mention that `null` is allowed. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > undo changes to CheckedInputStream and CheckedOutputStream Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17728#pullrequestreview-1867805176
Re: RFR: JDK-8325189: Enable this-escape javac warning in java.base
On Fri, 2 Feb 2024 23:36:41 GMT, Joe Darcy wrote: > After the "this-escape" lint warning was added to javac (JDK-8015831), the > base module was not updated to be able to compile with this warning enabled. > This PR makes the necessary changes to allow the base module to build with > the warning enabled. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17692#pullrequestreview-1866015274
Re: RFR: 8325304: Several classes in java.util.jar and java.util.zip don't specify the behaviour for null arguments [v2]
On Tue, 6 Feb 2024 10:31:06 GMT, Jaikiran Pai wrote: >> Can I please get a review of this doc-only change which updates the javadoc >> of several classes in `java.util.jar` and `java.util.zip` to specify their >> behaviour when `null` arguments are passed to the constructor or methods of >> those classes? >> >> For these updated classes, I have individually checked that they indeed >> throw a `NullPointerException` when `null` is passed to their constructor or >> methods. The couple of places where `null` is accepted have been updated to >> mention that `null` is allowed. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > include CheckedInputStream and CheckedOutputStream Hi Jai, Thank you for tackling this. An initial comment on a quick read I noticed you use "can cause" here and later use "will cause". I think we should try to be consistent. Perhaps we should use "will result in a..." - PR Review: https://git.openjdk.org/jdk/pull/17728#pullrequestreview-1864907969
Re: RFR: 8324635: (zipfs) Regression in Files.setPosixFilePermissions called on existing MSDOS entries [v2]
On Fri, 2 Feb 2024 21:20:29 GMT, Eirik Bjørsnøs wrote: >> Please review this PR to fix to a regression in ZipFileSystem, introduced by >> JDK-8322565 in PR #17170. >> >> When `Files.setPosixFilePermissions` is called on an existing MSDOS entry, >> then `Entry.posixPerms` field will be -1 (all 1s in binary). The logic >> introduced by JDK-8322565 did not account for this and incorrectly sets the >> leading seven bits to all ones instead of all zeros. >> >> The fix is to introduce a branch for `Entry.posixPerms` being -1 and deal >> with that case separately. >> >> The PR adds a test case to `TestPosix` which reproduces the regression. >> While visiting this test, I also fixed an incorrect spelling of >> `setPosixFilePermissions` (also introduced by #17170). > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > For clarity, add comments with the before/after zipdetails output of the > relevant CEN fields Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17556#pullrequestreview-1860426767
Re: RFR: 8324635: (zipfs) Regression in Files.setPosixFilePermissions called on existing MSDOS entries [v2]
On Fri, 2 Feb 2024 21:17:48 GMT, Eirik Bjørsnøs wrote:
>> test/jdk/jdk/nio/zipfs/TestPosix.java line 757:
>>
>>> 755: try (FileSystem fs = FileSystems.newFileSystem(ZIP_FILE,
>>> ENV_POSIX)) {
>>> 756: Path path = fs.getPath("hello.txt");
>>> 757: Files.setPosixFilePermissions(path,
>>> EnumSet.of(OWNER_READ));
>>
>> It might be helpful to show the before/after output of the CEN fields here
>> just for extra clarity
>
>> It might be helpful to show the before/after output of the CEN fields here
>> just for extra clarity
>
> Thanks! Can you take a look at 4bb77e1 which shows the `zipdetails` output
> for relevant fields before/after calling `setPosixFilePermissions`.
thank you Looks good
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17556#discussion_r1476753283
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes [v2]
On Fri, 2 Feb 2024 20:19:51 GMT, Eirik Bjørsnøs wrote: > > I think the change is OK, any reason to not make it > > `externalFileAttributes`? > > None other than length / verbosity. But since this refers to the _attributes > of the external file_, dropping any name element also loses some semantics, > introducing a potential for confusion. Perhaps verbosity is the sensible > choice here. > > If you agree to the above, I can update the PR to rename to the following > names instead: > > * `ZipFile.externalFileAttributes` > * `JavaUtilZipFileAccess.java.[set|get]ExternalFileAttributes` > * `JarSigner.externalFileAttributesDetected` > * `jarsigner/Main.externalFileAttributesDetected` > * `external.file.attributes.detected` in `Resources.java` > > WDYT? I think the proposed change above makes things clearer. Perhaps also make the same change in zipfs as well while you are at it? - PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-1924670785
Re: RFR: 8321396: Retire test/jdk/java/util/zip/NoExtensionSignature.java [v2]
On Wed, 24 Jan 2024 10:28:46 GMT, Eirik Bjørsnøs wrote: >> Please review this PR which suggests we retire the ZIP test >> `NoExtensionSignature` along with its `test.jar` test vector. >> >> The concern of a missing data descriptor signature is covered by the >> recently updated `DataDescriptorSignatureMissing` test, see #12959. That >> test is more complete, includes more documentation and uses a >> programmatically generated test vector. >> >> Careful analysis of the deleted `test.jar` test vector revealed that it >> contains a local header with non-zero `compressed size` and `uncompressed >> size` fields for a streaming-mode entry. `APPNOTE.TXT` mandates that when >> bit 3 of the general purpose bit flag is set, then these fields and the >> `crc` field should all be set to zero. >> >> By injecting assertions into `ZipInputStream.readLOC` I was able to >> determine that `NoExtensionSignature` is the only test currently parsing a >> ZIP file with such non-zero fields in streaming mode. >> >> Because of this, and out of caution, this PR introduces a new test >> `DataDescriptorIgnoreCrcAndSizeFields` which explicitly verifies that >> `ZipInputStream` does not read any non-zero `crc`, `compressed size` or >> `uncompressed size` field values from a local header when in streaming mode. >> `ZipInputStream` should ignore these values and it would be good to have a >> test which asserts that this invariant holds even when the fields are >> non-zero. > > 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: > > - Merge branch 'master' into retire-no-extension-signature > - Rename 'nameAndContent' parameter to 'expected' > - Retire the test NoExtensionSignature in favor of > DataDescriptorSignatureMissing. Introduce the new test > DataDescriptorIgnoreCrcAndSizeFields covering unrelated aspects implicitly > tested by NoExtensionSignature. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16975#pullrequestreview-1860257395
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v5]
On Mon, 22 Jan 2024 13:58:00 GMT, Eirik Bjørsnøs wrote: >> Please consider this PR which makes `DeflaterOutputStream.close()` always >> close its wrapped output stream exactly once. >> >> Currently, closing of the wrapped output stream happens outside the finally >> block where `finish()` is called. If `finish()` throws, this means the >> wrapped stream will not be closed. This can potentially lead to leaking >> resources such as file descriptors or sockets. >> >> This fix is to move the closing of the wrapped stream inside the finally >> block. >> >> Additionally, the `closed = true;` statement is moved to the start of the >> close method. This makes sure we only ever close the wrapped stream once >> (this aligns with the overridden method `FilterOutputStream.close´) >> >> Specification: This change brings the implementation of >> `DeflaterOutputStream.close()` in line with its specification: *Writes >> remaining compressed data to the output stream and closes the underlying >> stream.* >> >> Risk: This is a behavioural change. There is a small risk that existing code >> depends on the close method not following its specification. >> >> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which >> simulates the failure condition and verifies that the wrapped stream was >> closed under failing and non-failing conditions. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Spell out what is checked by each test instead of using the word "correct" Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17209#pullrequestreview-1860246844
Re: RFR: 8324635: (zipfs) Regression in Files.setPosixFilePermissions called on existing MSDOS entries
On Wed, 24 Jan 2024 14:34:03 GMT, Eirik Bjørsnøs wrote:
> Please review this PR to fix to a regression in ZipFileSystem, introduced by
> JDK-8322565 in PR #17170.
>
> When `Files.setPosixFilePermissions` is called on an existing MSDOS entry,
> then `Entry.posixPerms` field will be -1 (all 1s in binary). The logic
> introduced by JDK-8322565 did not account for this and incorrectly sets the
> leading seven bits to all ones instead of all zeros.
>
> The fix is to introduce a branch for `Entry.posixPerms` being -1 and deal
> with that case separately.
>
> The PR adds a test case to `TestPosix` which reproduces the regression. While
> visiting this test, I also fixed an incorrect spelling of
> `setPosixFilePermissions` (also introduced by #17170).
Looks OK overall. One minor suggestion
test/jdk/jdk/nio/zipfs/TestPosix.java line 757:
> 755: try (FileSystem fs = FileSystems.newFileSystem(ZIP_FILE,
> ENV_POSIX)) {
> 756: Path path = fs.getPath("hello.txt");
> 757: Files.setPosixFilePermissions(path, EnumSet.of(OWNER_READ));
It might be helpful to show the before/after output of the CEN fields here just
for extra clarity
-
Marked as reviewed by lancea (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17556#pullrequestreview-1860241911
PR Review Comment: https://git.openjdk.org/jdk/pull/17556#discussion_r1476630176
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes [v2]
On Tue, 30 Jan 2024 16:17:13 GMT, Eirik Bjørsnøs 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" > > 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: > > - Update copyright years for 2024 > - Merge branch 'master' into zipentry-external-attributes > - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes I think the change is OK, any reason to not make it `externalFileAttributes`? - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16952#pullrequestreview-1860228844
Integrated: 8324632: Update Zlib Data Compression Library to Version 1.3.1
On Mon, 29 Jan 2024 16:57:00 GMT, Lance Andersen wrote: > Hi all, > > Please review this PR which updates zlib from 1.3 to 1.3.1 in OpenJDK > > The [Zlib Data Compression Library](https://github.com/madler/zlib) has > released Zlib 1.3.1 on January 24, 2024. > > There are a [small number of > updates](https://github.com/madler/zlib/compare/v1.3.1..v1.3) between 1.3 and > 1.3.1 > > Mach5 tiers1-3 have run clean This pull request has now been integrated. Changeset: b5c267fc Author:Lance Andersen URL: https://git.openjdk.org/jdk/commit/b5c267fc8a0af50be9e3d1d09cdaa6bf4bb29851 Stats: 164 lines in 14 files changed: 82 ins; 35 del; 47 mod 8324632: Update Zlib Data Compression Library to Version 1.3.1 Reviewed-by: iris, alanb - PR: https://git.openjdk.org/jdk/pull/17619
Re: RFR: 8324632: Update Zlib Data Compression Library to Version 1.3.1
On Mon, 29 Jan 2024 17:15:25 GMT, Alan Bateman wrote: > @LanceAndersen Can you confirm that there are no changes to the 1.3..1.3.1 > diffs? @AlanBateman, yes that is correct, there are no OpenJDK specific changes, it is a straight port of the zlib 1.3.1 changes to our implementation - PR Comment: https://git.openjdk.org/jdk/pull/17619#issuecomment-1915235240
RFR: 8324632: Update Zlib Data Compression Library to Version 1.3.1
Hi all, Please review this PR which updates zlib from 1.3 to 1.3.1 in OpenJDK The [Zlib Data Compression Library](https://github.com/madler/zlib) has released Zlib 1.3.1 on January 24, 2024. There are a [small number of updates](https://github.com/madler/zlib/compare/v1.3.1..v1.3) between 1.3 and 1.3.1 Mach5 tiers1-3 have run clean - Commit messages: - update zlib to zlib 1.3.1 Changes: https://git.openjdk.org/jdk/pull/17619/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17619=00 Issue: https://bugs.openjdk.org/browse/JDK-8324632 Stats: 164 lines in 14 files changed: 82 ins; 35 del; 47 mod Patch: https://git.openjdk.org/jdk/pull/17619.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17619/head:pull/17619 PR: https://git.openjdk.org/jdk/pull/17619
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v15]
On Fri, 26 Jan 2024 14:34:39 GMT, Eirik Bjørsnøs wrote: > To help make progress here, I have relaxed the validation such that we now > check: > > * That the "streaming mode" bit 3 flag is set > * That at least one of the LOC's size fields are marked 0x. > * That the LOC extra field has a field with header ID 0x1 (Zip64) > > Any reading/validation of the contents of the Zip64 extra field has been > removed. > > @jaikiran Is this closer to what you'd like to see? Have not forgotten this, hope to get to it next week - PR Comment: https://git.openjdk.org/jdk/pull/12524#issuecomment-1912678510
Re: RFR: 8324657: Intermittent OOME on exception message create
On Mon, 22 Jan 2024 20:52:32 GMT, Roger Riggs wrote: > When an exception handler for an OutOfMemoryError uses string concatenation > to compose an exception message, the invoke dynamic string format > implementation may itself exhaust memory, preventing the exception from being > handled. > Explicit use of String.concat() call can improve exception handling. > > Writing a test of the exact failure condition has proved challenging due to > the unpredictable state of memory when OOME occurs. The replacement of "+" > with String.concat() is simple and direct. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17522#pullrequestreview-1837408236
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v12]
On Mon, 22 Jan 2024 14:34:25 GMT, Eirik Bjørsnøs wrote: > @LanceAndersen Do you have a cent or two to spare? Let me try and dig out from a couple of things and circle back to this again - PR Review Comment: https://git.openjdk.org/jdk/pull/12524#discussion_r1462519461
Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v4]
On Mon, 15 Jan 2024 10:26:53 GMT, Eirik Bjørsnøs wrote: >> Please consider this PR which makes `DeflaterOutputStream.close()` always >> close its wrapped output stream exactly once. >> >> Currently, closing of the wrapped output stream happens outside the finally >> block where `finish()` is called. If `finish()` throws, this means the >> wrapped stream will not be closed. This can potentially lead to leaking >> resources such as file descriptors or sockets. >> >> This fix is to move the closing of the wrapped stream inside the finally >> block. >> >> Additionally, the `closed = true;` statement is moved to the start of the >> close method. This makes sure we only ever close the wrapped stream once >> (this aligns with the overridden method `FilterOutputStream.close´) >> >> Specification: This change brings the implementation of >> `DeflaterOutputStream.close()` in line with its specification: *Writes >> remaining compressed data to the output stream and closes the underlying >> stream.* >> >> Risk: This is a behavioural change. There is a small risk that existing code >> depends on the close method not following its specification. >> >> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which >> simulates the failure condition and verifies that the wrapped stream was >> closed under failing and non-failing conditions. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Update test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java > > Remove extra whitespace > > Co-authored-by: Andrey Turbanov The changes look good overall.See suggestion for comment improvement but not required, just makes it clearer test/jdk/java/util/zip/ZipOutputStream/CloseWrappedStream.java line 91: > 89: /** > 90: * Check that the exception handling is correct when the > 91: * wrapped stream throws while being closed This comment could use a bit of wordsmithing to indicate what "correct" means - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17209#pullrequestreview-1824541172 PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1453817997
Re: RFR: 8323571: Regression in source resolution process
On Thu, 11 Jan 2024 18:36:39 GMT, Joe Wang wrote: > Fix a regression in the source resolution process where it failed to > recognize a custom InputSource when both the public and system IDs are null. > The particular issue is fixed at line 1233, the rest of the changes is > formatting to make the null-check on InputSource the first. > > Test is provided by Marc, thanks Marc! > (https://github.com/junit-team/junit5/issues/3594) > > XML tests, including the new test, pass with this change. Marked as reviewed by lancea (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17377#pullrequestreview-1816320558
Re: RFR: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile [v10]
On Tue, 9 Jan 2024 07:29:49 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 two additional > commits since the last revision: > > - Address review comments for ZIP64 End Of Central header tests > - Copyright update for 2024 Internal mach5 tiers 1-3 completed - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17038#pullrequestreview-1814329966
Re: RFR: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile [v7]
On Fri, 15 Dec 2023 11:15:30 GMT, Lance Andersen 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. > >> 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... > Thanks for your review, @LanceAndersen! > > You might want to take a quick look at the lastest updates addressing your > review before I integrate. Looks good. I am running mach5 tiers1-3 internally once it completes, I will approve again - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1885761673
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
