Re: RFR: 8287696: Avoid redundant Hashtable.containsKey call in JarVerifier.doneWithMeta
On Sat, 28 May 2022 12:00:00 GMT, Andrey Turbanov wrote: > Hashtable doesn't allow `null` values. So, instead of pair > `containsKey`/`remove` calls, we can directly call `remove` and then compare > result with `null`. > https://github.com/openjdk/jdk/blob/2c461acfebd28fe5ef62805cbb004f91a3b18f08/src/java.base/share/classes/java/util/jar/JarVerifier.java#L433-L436 The changes to doneWithMeta() seem reasonable and the other changes remove unused code so look OK to me - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.org/jdk/pull/8935
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code [v3]
On Tue, 24 May 2022 09:52:29 GMT, Alexey Ivanov wrote: >> Replaces usages of articles that follow each other in all combinations: >> a/the, an?/an?, the/the… >> >> It's the last issue in the series, and it still touches different areas of >> the code. > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright to 2022 Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8771
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code
On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > It's the last issue in the series, and it still touches different areas of > the code. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8771
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8683
Re: RFR: 8284893: Fix typos in java.base [v4]
On Tue, 19 Apr 2022 16:50:12 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Oracle copyrights > - Also revert changes in ASM (3rd party code) Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer The ZipFS and Jar changes seem OK - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines
On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan wrote: > Hi > > I have reviewed the code for removing double semicolons at the end of lines > > all the best > matteo What problem are you having editing the PR header? You should be able to do so as the author of the PR - PR: https://git.openjdk.java.net/jdk/pull/7268
Re: RFR: JDK-8282686: Add constructors taking a cause to SocketException
On Fri, 4 Mar 2022 21:17:01 GMT, Joe Darcy wrote: > Please review this small API enhancement to add the usual constructors taking > a cause to SocketException and then update uses of initiCause on creating > SocketException to instead pass the cause via the constructor. > > Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282688 Looks fine, would be worth including a couple of tests for coverage - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7705
Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines
On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan wrote: > Hi > > I have reviewed the code for removing double semicolons at the end of lines > > all the best > matteo The changes look OK. The copyright year probably should be updated as part of this PR - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7268
Integrated: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName()
On Fri, 4 Feb 2022 12:42:39 GMT, Lance Andersen wrote: > Hi all, > > Please review the attached patch to address > > - That JarFile::getInputStream did not check for a null ZipEntry passed as a > parameter > - Have Zip/JarFile::getInputStream throw a ZipException in the event that an > unexpected exception occurs > > Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar > test runs > > Best > Lance This pull request has now been integrated. Changeset: a020b6ba Author:Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/a020b6ba8f38fe85fb26972a51e4c1068408b1c1 Stats: 1103 lines in 3 files changed: 1071 ins; 0 del; 32 mod 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() Reviewed-by: mullan, alanb - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v8]
> Hi all, > > Please review the attached patch to address > > - That JarFile::getInputStream did not check for a null ZipEntry passed as a > parameter > - Have Zip/JarFile::getInputStream throw a ZipException in the event that an > unexpected exception occurs > > Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar > test runs > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Modified and clarified test comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7348/files - new: https://git.openjdk.java.net/jdk/pull/7348/files/839d99f7..e540a3a1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7348=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7348=06-07 Stats: 64 lines in 1 file changed: 34 ins; 18 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/7348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348 PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]
On Sat, 19 Feb 2022 10:59:56 GMT, Alan Bateman wrote: > > Ok, thank you for the feedback. I just pushed a change with additional > > comments on the jar creation which hopefully will address your input above. > > It's a bit better but I think it needs a clear step-by-step instructions in a > comment before declaration of VALID_ENTRY_NAME to show how the JAR file is > created, signed (move the instructions that have been placed on the TestNG > setup method), and then converted to the byte array. Further maintainers will > thank you. Just pushed a revised set of comments. Hopefully this will get us across the goal line. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v7]
> Hi all, > > Please review the attached patch to address > > - That JarFile::getInputStream did not check for a null ZipEntry passed as a > parameter > - Have Zip/JarFile::getInputStream throw a ZipException in the event that an > unexpected exception occurs > > Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar > test runs > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: remove extra '}' - Changes: - all: https://git.openjdk.java.net/jdk/pull/7348/files - new: https://git.openjdk.java.net/jdk/pull/7348/files/818c2857..839d99f7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7348=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7348=05-06 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348 PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v6]
> Hi all, > > Please review the attached patch to address > > - That JarFile::getInputStream did not check for a null ZipEntry passed as a > parameter > - Have Zip/JarFile::getInputStream throw a ZipException in the event that an > unexpected exception occurs > > Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar > test runs > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: remove trailing space - Changes: - all: https://git.openjdk.java.net/jdk/pull/7348/files - new: https://git.openjdk.java.net/jdk/pull/7348/files/490c986d..818c2857 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7348=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7348=04-05 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348 PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]
On Fri, 18 Feb 2022 17:20:26 GMT, Alan Bateman wrote: > > If you feel there is still something lacking for documentation, I can > > certainly make another pass clarify/add it, but I tried to cover the steps > > (but I also understand what might be obvious to me might not be as obvious > > as I thought). > > Yes, I think clear instructions are important to have for tests like this. It > doesn't need much but what you know is scattered across a method and a > comment on a byte array, just not clear/easy. Ok, thank you for the feedback. I just pushed a change with additional comments on the jar creation which hopefully will address your input above. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v5]
> Hi all, > > Please review the attached patch to address > > - That JarFile::getInputStream did not check for a null ZipEntry passed as a > parameter > - Have Zip/JarFile::getInputStream throw a ZipException in the event that an > unexpected exception occurs > > Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar > test runs > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Add additional comments describing how the jars are created - Changes: - all: https://git.openjdk.java.net/jdk/pull/7348/files - new: https://git.openjdk.java.net/jdk/pull/7348/files/d5cf8db8..490c986d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7348=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7348=03-04 Stats: 29 lines in 1 file changed: 28 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348 PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]
On Fri, 18 Feb 2022 17:05:53 GMT, Alan Bateman wrote: > > > The updates changes to ZipFile/JarFile look okay. I don't have time to > > > study the test too closely right now but it will need to include > > > instructions on how to re-create the signed JAR that is stored in the > > > byte array. > > > > > > Those instructions are already in the comments for the constant > > `SIGNED_VALID_ENTRY_NAME` > > That's the keytool command to sign the JAR. What I meant is the complete > steps to create the JAR file, sign it, and then create the byte array. The `createByteArray` method which is at the bottom of the test class documents how the byte array can be made from a jar. The signed jar is created using the steps defined in `SIGNED_VALID_ENTRY_NAME` from the jar derived from `VALID_ENTRY_NAME` If you feel there is still something lacking for documentation, I can certainly make another pass clarify/add it, but I tried to cover the steps (but I also understand what might be obvious to me might not be as obvious as I thought). - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]
On Fri, 18 Feb 2022 12:09:53 GMT, Alan Bateman wrote: > The updates changes to ZipFile/JarFile look okay. I don't have time to study > the test too closely right now but it will need to include instructions on > how to re-create the signed JAR that is stored in the byte array. Those instructions are already in the comments for the constant `SIGNED_VALID_ENTRY_NAME` - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v3]
On Fri, 11 Feb 2022 17:43:47 GMT, Lance Andersen wrote:
>> src/java.base/share/classes/java/util/jar/JarFile.java line 881:
>>
>>> 879: ze = getJarEntry(entryName);
>>> 880: } else {
>>> 881: throw new ZipException("ZipEntry::getName returned null");
>>
>> Throwing ZipException when ZipEntry::getName returns null is still
>> surprising but not terrible. The spec for getInputStream specifies
>> ZipException for when a zip file format occurs so we might have to extend
>> that to add "or the zip entry name is null".
>
> If you would like me to update the ZipException to clarify this I can. The
> original issue was due to a format error in the CEN so the current wording
> covers that. It does not cover the case where ZipEntry is subclassed and
> ZipEntry::getName() returns null which is what your suggested wording would
> address.
>
> Please note the above change addresses the signed jar scenario. I can add an
> additional check in JarFile::getInputStream to check for null from
> ZipEntry::getName so that it covers unsigned jars and signed jars not being
> verified.
>
> The issue will not occur with ZipEFile::getInputStream given its use of
> ZipEntry.name which can never be null.
>
> Another thought would be to return null for the InputStream similar to when
> the ZipEntry does not represent a file within the Jar
>
> Please let me know your preference
Per a discussion with Sean and Alan, we are now returning null for the
InputStream to be consistent with ZipFile::getInputStream and how unsigned jars
are handled
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v4]
> Hi all, > > Please review the attached patch to address > > - That JarFile::getInputStream did not check for a null ZipEntry passed as a > parameter > - Have Zip/JarFile::getInputStream throw a ZipException in the event that an > unexpected exception occurs > > Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar > test runs > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Return null when ZipEntry::getName is null - Changes: - all: https://git.openjdk.java.net/jdk/pull/7348/files - new: https://git.openjdk.java.net/jdk/pull/7348/files/32f6c284..d5cf8db8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7348=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7348=02-03 Stats: 201 lines in 2 files changed: 34 ins; 96 del; 71 mod Patch: https://git.openjdk.java.net/jdk/pull/7348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348 PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v3]
On Fri, 11 Feb 2022 13:45:47 GMT, Alan Bateman wrote:
>> Lance Andersen has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Return a null InputStream when the ZipEntry is not found in the Jar
>> - Address formatting and message feedback
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 881:
>
>> 879: ze = getJarEntry(entryName);
>> 880: } else {
>> 881: throw new ZipException("ZipEntry::getName returned null");
>
> Throwing ZipException when ZipEntry::getName returns null is still surprising
> but not terrible. The spec for getInputStream specifies ZipException for
> when a zip file format occurs so we might have to extend that to add "or the
> zip entry name is null".
If you would like me to update the ZipException to clarify this I can. The
original issue was due to a format error in the CEN so the current wording
covers that. It does not cover the case where ZipEntry is subclassed and
ZipEntry::getName() returns null which is what your suggested wording would
address.
Please note the above change addresses the signed jar scenario. I can add an
additional check in JarFile::getInputStream to check for null from
ZipEntry::getName so that it covers unsigned jars and signed jars not being
verified.
The issue will not occur with ZipEntry::getInputStream given its use of
ZipEntry.name which can never be null.
Please let me know your preference
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Thu, 10 Feb 2022 20:37:50 GMT, Sean Mullan wrote: >> Agree on returning null to maintain current behavior. I would also lean >> towards amending the specification to specify what has been long-standing >> behavior. > > If we had to do it over again, I do think throwing IAE is more appropriate > because this case would probably be due to a bug in the application code. Now > code has to defensively check for a null return value. I don't know, maybe we > don't want to modify the specification at this point and leave this as > undefined behavior. > Agree on returning null to maintain current behavior. I would also lean > towards amending the specification to specify what has been long-standing > behavior. I just updated the PR to return null - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v3]
> Hi all, > > Please review the attached patch to address > > - That JarFile::getInputStream did not check for a null ZipEntry passed as a > parameter > - Have Zip/JarFile::getInputStream throw a ZipException in the event that an > unexpected exception occurs > > Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar > test runs > > Best > Lance Lance Andersen has updated the pull request incrementally with two additional commits since the last revision: - Return a null InputStream when the ZipEntry is not found in the Jar - Address formatting and message feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/7348/files - new: https://git.openjdk.java.net/jdk/pull/7348/files/6c75384a..32f6c284 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7348=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7348=01-02 Stats: 95 lines in 3 files changed: 41 ins; 20 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/7348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348 PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 18:06:04 GMT, Lance Andersen wrote:
>>> ze can't be null here.
>>
>> Actually it can be: Consider the following:
>>
>>
>> try (JarFile jf = new JarFile(SIGNED_VALID_ENTRY_NAME_JAR.toFile(),
>> true)) {
>> var ze = new ZipEntry("org/gotham/Batcave.class");
>> var ex= expectThrows(ZipException.class,
>> () -> jf.getInputStream(ze) );
>> // Validate that we receive the expected message from
>> // JarFile::verifiableEntry when ZipEntry::getName returns null
>> assertTrue( ex != null && ex.getMessage().equals("Error:
>> ZipEntry should not be null!"));
>> }
>>
>>
>> The above code does generate the error.
>
>> Nit, add space after "if"
>
> will fix
So a bit more on this. If the ZipEntry passed to `ZipFile::getInputStream`
does not represent an entry within the current Zip/Jar,
`ZipFile::getInputStream` will return a null for the InputStream:
@Test
public static void ZipFileZipEntryNullGetInputStreamTest() throws Exception
{
try (ZipFile zf = new ZipFile(VALID_ENTRY_NAME_JAR.toFile())) {
var ze = new ZipEntry("org/gotham/Batcave.class");
var is = zf.getInputStream(ze);
// As the ZipEntry cannot be found, the returned InpuStream is null
assertNull(is);
}
}
JarFile::getInputStream will also return null when the jar is not signed or
we are not verifying the jar:
@Test
public static void JarFileZipEntryNullGetInputStreamTest() throws Exception
{
try (JarFile jf = new JarFile(VALID_ENTRY_NAME_JAR.toFile())) {
var ze = new ZipEntry("org/gotham/Batcave.class");
var is = jf.getInputStream(ze);
// As the ZipEntry cannot be found, the returned InpuStream is null
assertNull(is);
}
try (JarFile jf = new JarFile(SIGNED_INVALID_ENTRY_NAME_JAR.toFile(),
false)) {
var ze = new ZipEntry("org/gotham/Batcave.class");
var is = jf.getInputStream(ze);
// As the ZipEntry cannot be found, the returned InpuStream is null
assertNull(is);
}
}
This behavior dates back to at least JDK 1.3
So I think we should return null instead of throwing an Exception when the
resulting ZipEntry is null that is returned from the call
to`JarFile::getJarEntry` (which calls `ZipFile::getEntry`) for consistency with
ZipFile and when the Jar is not signed or not verified.
I also noticed that `ZipFile::getInputStream` and `JarFile::getInputStream` do
not mention that a null will be returned if the specified ZipEntry is not found
within the Jar/Zip. I guess I could open a CSR as part of this fix to clarify
this 20+ year behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 18:56:02 GMT, Alan Bateman wrote: >>> I'm almost tempted to have getInputStream(ZipEntry) be re-specified to >>> throw IAE if the zip entry name is null. >> >> I personally think it is best to continue throw the NPE as that provides >> symmetry with ZipFile::getInputStream, aligns with the current javadoc >> where a null parameter will throw an NPE unless specified elsewhere, there >> are existing tests which check for an NPE if JarFile::getInpuStream(null) is >> called. > >> I personally think it is best to continue throw the NPE as that provides >> symmetry with ZipFile::getInputStream, aligns with the current javadoc where >> a null parameter will throw an NPE unless specified elsewhere, there are >> existing tests which check for an NPE if JarFile::getInpuStream(null) is >> called. > > I think the scenario that we are discussing is where the parameter is not > null. It's the case where getInputStream(ZipEntry) is called with a ZipEntry > that reports its name as null. I don't think it is covered by existing > javadoc. Ah, OK, sorry I was focused on the NPE and uber focused on the `ZipEntry` passed to `getInputStream`. If you *prefer* an IAE, I can change that when `ZipEntry::getName` returns null. I do think we are technically covered via `ZipException` which is defined : ` if a zip file format error has occurred` and unless something has gone amiss with the Zip/Jar LOC/CEN, you should expect a non-null value. Please let me know your preference as if we go the IAE route, I will need to create a CSR which I was hoping to avoid - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 18:05:25 GMT, Lance Andersen wrote:
>> ze can't be null here.
>
>> ze can't be null here.
>
> Actually it can be: Consider the following:
>
>
> try (JarFile jf = new JarFile(SIGNED_VALID_ENTRY_NAME_JAR.toFile(),
> true)) {
> var ze = new ZipEntry("org/gotham/Batcave.class");
> var ex= expectThrows(ZipException.class,
> () -> jf.getInputStream(ze) );
> // Validate that we receive the expected message from
> // JarFile::verifiableEntry when ZipEntry::getName returns null
> assertTrue( ex != null && ex.getMessage().equals("Error: ZipEntry
> should not be null!"));
> }
>
>
> The above code does generate the error.
> Nit, add space after "if"
will fix
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 15:55:50 GMT, Alan Bateman wrote:
> ze can't be null here.
Actually it can be: Consider the following:
try (JarFile jf = new JarFile(SIGNED_VALID_ENTRY_NAME_JAR.toFile(),
true)) {
var ze = new ZipEntry("org/gotham/Batcave.class");
var ex= expectThrows(ZipException.class,
() -> jf.getInputStream(ze) );
// Validate that we receive the expected message from
// JarFile::verifiableEntry when ZipEntry::getName returns null
assertTrue( ex != null && ex.getMessage().equals("Error: ZipEntry
should not be null!"));
}
The above code does generate the error.
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 15:31:51 GMT, Sean Mullan wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reduce Exception checking to JarFile::verifiableEntry
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 874:
>
>> 872: ze = getJarEntry(ze.getName());
>> 873: } else {
>> 874: throw new ZipException("Error: ZipEntry::getName returned
>> null!");
>
> I'd probably leave out the "Error:" and the "!".
OK, I can do that if you prefer.
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 18:06:52 GMT, Lance Andersen wrote: >> Ah, yes - good catch! > > Will do. > I'm almost tempted to have getInputStream(ZipEntry) be re-specified to throw > IAE if the zip entry name is null. I personally think it is best to continue throw the NPE as that provides symmetry with ZipFile::getInputStream, aligns with the current javadoc where a null parameter will throw an NPE unless specified elsewhere, there are existing tests which check for an NPE if JarFile::getInpuStream(null) is called. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Tue, 8 Feb 2022 16:15:20 GMT, Sean Mullan wrote: >> if ZipEntry is extended and getName() overridden then you can't trust the >> name. So I think you'll have extract the name rather than calling >> ZipEntry::getName twice. I'm almost tempted to have getInputStream(ZipEntry) >> be re-specified to throw IAE if the zip entry name is null. > > Ah, yes - good catch! Will do. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Mon, 7 Feb 2022 20:16:43 GMT, Lance Andersen wrote: >> If you are pretty sure the only other case are as above, I wonder if a >> simpler fix would be to change `verifiableEntry()` to check for these null >> cases and throw a `ZipException` which will get directly propagated by >> `getInputStream()`? > > I can certainly throw a ZipException from `verifiableEntry`. I am a bit > reluctant to not catch any other `Exception` and then throw a `ZipException` > from `getInputStream()` as it is certainly possible of encountering some > other issue due to some stray value in the CEN. > > So I will update `verifiableEntry` to validate `ZipEntry` and` > ZipEntry::getName()` potential issues Per an offline-discussion with Sean, I narrowed the Exception checking to JarFile::verifiableEntry. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
> Hi all, > > Please review the attached patch to address > > - That JarFile::getInputStream did not check for a null ZipEntry passed as a > parameter > - Have Zip/JarFile::getInputStream throw a ZipException in the event that an > unexpected exception occurs > > Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar > test runs > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Reduce Exception checking to JarFile::verifiableEntry - Changes: - all: https://git.openjdk.java.net/jdk/pull/7348/files - new: https://git.openjdk.java.net/jdk/pull/7348/files/b50ebf05..6c75384a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7348=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7348=00-01 Stats: 162 lines in 3 files changed: 90 ins; 21 del; 51 mod Patch: https://git.openjdk.java.net/jdk/pull/7348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348 PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName()
On Mon, 7 Feb 2022 18:44:10 GMT, Sean Mullan wrote: >> Looking at this a bit more, it looks like `JariFile::initializeVerifier` is >> the only place currently in `JarFile` that could throw a `JarException` and >> that method could be called from `JarFile::getInputStream` >> >> As `verifiableEntry` is only called from `JarFile::getInputStream `and this >> change validates the `ZipEntry` for being null, which is should, the only >> other possible error would be in the unlikely event. `ZipEntry` was >> subclassed passed into `JarFile::getInputStream` resulting in the call to >> `ZipEntry::getName` returning null(in which case `ZipFile::getEntry` will >> return a `NullPointerException`) or the name being invalid resulting once >> again in a possible null value for the returned `ZipEntry` from >> `JarFile::getJarEntry` (which calls `ZipFile::getEntry`) >> >> >> I am still leaning towards a `ZipException`, not a `JarException`, thrown >> from `verifiableEntry`. >> >> That being said, I will go with whatever the consensus is. > > If you are pretty sure the only other case are as above, I wonder if a > simpler fix would be to change `verifiableEntry()` to check for these null > cases and throw a `ZipException` which will get directly propagated by > `getInputStream()`? I can certainly throw a ZipException from `verifiableEntry`. I am a bit reluctant to not catch any other `Exception` and then throw a `ZipException` from `getInputStream()` as it is certainly possible of encountering some other issue due to some stray value in the CEN. So I will update `verifiableEntry` to validate `ZipEntry` and` ZipEntry::getName()` potential issues - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName()
On Mon, 7 Feb 2022 15:16:43 GMT, Sean Mullan wrote: >> JarFile::getInputStream. mentions ZipException but not JarException which is >> why I chose this. If we change this to JarException, I would need to update >> the javadoc and create a CSR. >> >> Please let me know your preference > > `JarException` is a subclass of `ZipException` though, so I think this would > be ok to throw and still be compliant with the specification. Looking at this a bit more, it looks like `JariFile::initializeVerifier` is the only place currently in `JarFile` that could throw a `JarException` and that method could be called from `JarFile::getInputStream` As `verifiableEntry` is only called from `JarFile::getInputStream `and this change validates the `ZipEntry` for being null, which is should, the only other possible error would be in the unlikely event. `ZipEntry` was subclassed passed into `JarFile::getInputStream` resulting in the call to `ZipEntry::getName` returning null(in which case `ZipFile::getEntry` will return a `NullPointerException`) or the name being invalid resulting once again in a possible null value for the returned `ZipEntry` from `JarFile::getJarEntry` (which calls `ZipFile::getEntry`) I am still leaning towards a `ZipException`, not a `JarException`, thrown from `verifiableEntry`. That being said, I will go with whatever the consensus is. - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName()
On Fri, 4 Feb 2022 16:06:34 GMT, Lance Andersen wrote: > > Could these unexpected exceptions also occur when using the > > `JarInputStream` API? > > It's a different code path as Zip/JarFile leverage the CEN where > Zip/JarInputStream leverage the LOC. I can give it a go and if there is an > issue will create a separate issue Not an issue given how Zip/JarInputStream work - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName()
On Fri, 4 Feb 2022 15:55:33 GMT, Sean Mullan wrote: > Could these unexpected exceptions also occur when using the `JarInputStream` > API? It's a different code path as Zip/JarFile leverage the CEN where Zip/JarInputStream leverage the LOC. I can give it a go and if there is an issue will create a separate issue - PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName()
On Fri, 4 Feb 2022 15:06:59 GMT, Alan Bateman wrote:
>> Hi all,
>>
>> Please review the attached patch to address
>>
>> - That JarFile::getInputStream did not check for a null ZipEntry passed as a
>> parameter
>> - Have Zip/JarFile::getInputStream throw a ZipException in the event that an
>> unexpected exception occurs
>>
>> Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar
>> test runs
>>
>> Best
>> Lance
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 840:
>
>> 838: throws IOException
>> 839: {
>> 840: Objects.requireNonNull(ze, "ze");
>
> Is the NPE specified?
Yes it is specified in the JarFile main paragraph
`Unless otherwise noted, passing a null argument to a constructor or method in
this class will cause a NullPointerException to be thrown.`
ZipFile::getInputStream validates the argument as soon as it is passed to
getInputStream.
> src/java.base/share/classes/java/util/jar/JarFile.java line 866:
>
>> 864: } catch (Exception e2) {
>> 865: // Any other Exception should be a ZipException
>> 866: throw (ZipException) new ZipException("Zip file format
>> error").initCause(e2);
>
> If there is ZIP format error then I would expect ZipException or the more
> general IOException is already thrown. So I think this is catching other
> cases, maybe broken manifests or signed JAR files, in which case a
> JarException may be better.
JarFile::getInputStream. mentions ZipException but not JarException which is
why I chose this. If we change this to JarException, I would need to update
the javadoc and create a CSR.
Please let me know your preference
-
PR: https://git.openjdk.java.net/jdk/pull/7348
RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName()
Hi all, Please review the attached patch to address - That JarFile::getInputStream did not check for a null ZipEntry passed as a parameter - Have Zip/JarFile::getInputStream throw a ZipException in the event that an unexpected exception occurs Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar test runs Best Lance - Commit messages: - Update copyright year - Address Zip/JarFile::getInputStream Exception handling Changes: https://git.openjdk.java.net/jdk/pull/7348/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7348=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280409 Stats: 1081 lines in 3 files changed: 1026 ins; 26 del; 29 mod Patch: https://git.openjdk.java.net/jdk/pull/7348.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348 PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: JDK-8281082: Improve javadoc references to JOSS
On Tue, 1 Feb 2022 21:11:39 GMT, Joe Darcy wrote: > The references to JOSS, the Java Object Serialization Specification, are not > done consistently in the API javadoc. This should be improved. > > I'll update copyright years before pushing. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7315
Re: RFR: 8279918: Fix various doc typos [v2]
On Thu, 13 Jan 2022 14:01:04 GMT, Pavel Rappo wrote: >> - Most of the typos are of a trivial kind: missing whitespace. >> - If any of the typos should be fixed in the upstream projects instead, >> please say so; I will drop those typos from the patch. >> - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant >> formatting artefact and thus can be removed. >> - `` is an apostrophe, which does not require to be encoded. > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Additional typos Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos [v2]
On Thu, 13 Jan 2022 13:48:02 GMT, Pavel Rappo wrote: > > Yes an "or" is probably worthwhile to add. > > I would prefer to leave it for the follow-up cleanup if only because adding > "or" here would make it inconsistent with other `@throws SQLException` > descriptions in that file and perhaps elsewhere in java.sql: > > * java/sql/Statement.java:59 > * java/sql/Statement.java:85 > * java/sql/Statement.java:346 > * java/sql/Statement.java:524 > * java/sql/Statement.java:745 > * java/sql/Statement.java:814 > * java/sql/Statement.java:860 > * java/sql/Statement.java:913 > * java/sql/Statement.java:962 > * java/sql/Statement.java:1225 > * java/sql/Statement.java:1269 > * java/sql/Statement.java:1314 I am fine with that. I guess a semi-colon could also be used vs. a comma to divide the Exception scenarios listed - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos [v2]
On Thu, 13 Jan 2022 12:37:46 GMT, Pavel Rappo wrote: > > The above is a bit confusing as it reads(same in ImageInputStream.java). I > > think that that can be looked at later. > > Just to clarify: you are okay with removing the ` ` entity, right? As for > ImageInputStream, some doc comments should probably use `@inheritDoc`. But > this is a much bigger clean-up. Yes I am. It just does not flow as well as it could IMHO - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:29:35 GMT, Pavel Rappo wrote:
>> src/java.sql/share/classes/java/sql/Statement.java line 784:
>>
>>> 782: * statement returns a {@code ResultSet} object, the second
>>> argument
>>> 783: * supplied to this method is not an
>>> 784: * {@code int} array whose elements are valid column indexes, the
>>> method is called on a
>>
>> Should it be "or the method is called on...", i.e. add the "or", otherwise
>> it's a list of problems but we don't know if they are all required, or are
>> alternatives. It probably does not mean that all these have to be true to
>> throw the exception, but it doesn't say that. We are nitpicking of course.
>
> You are right in that this `@throws` description reads a bit weird in its
> current form. That said, I wouldn't touch it in this PR for two reasons.
> Firstly, this wording seems to be consistent with other locations in that
> file. Secondly, this is a spec territory.
Yes an "or" is probably worthwhile to add.
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:30:07 GMT, Pavel Rappo wrote:
> - Most of the typos are of a trivial kind: missing whitespace.
> - If any of the typos should be fixed in the upstream projects instead,
> please say so; I will drop those typos from the patch.
> - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant
> formatting artefact and thus can be removed.
> - `` is an apostrophe, which does not require to be encoded.
Overall this looks good, thanks for the clean up :-)
A few comments below
src/java.base/share/classes/java/io/DataInput.java line 496:
> 494: * ceases. If the character {@code '\r'}
> 495: * is encountered, it is discarded and, if
> 496: * the following byte converts to the
The above is a bit confusing as it reads(same in ImageInputStream.java). I
think that that can be looked at later.
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:28:34 GMT, Kevin Walls wrote: >> I thought about it too, but then noticed how the position of `[]` mimics >> that of the respective method signatures in code. > > OK, so lines 264, 295, 329, 364, 431 are arguably wrong as well? Separating > the [] completely looks quite rare. > I'll leave it up to you. 8-) I think that can be a follow on clean up. - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8274811: Remove superfluous use of boxing in java.base
On Sat, 11 Sep 2021 12:11:50 GMT, Andrey Turbanov wrote: > Usages of primitive types should be preferred and makes code easier to read. > Similar cleanups: > 1. [JDK-8273168](https://bugs.openjdk.java.net/browse/JDK-8273168) > java.desktop > 2. [JDK-8274234](https://bugs.openjdk.java.net/browse/JDK-8274234) > java.sql.rowset Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5481
Re: RFR: JDK-8276681: Malformed Javadoc inline tags in JDK source jdk/internal/net/http/ResponseSubscribers.java
On Sat, 20 Nov 2021 04:09:51 GMT, Tim Prinzing wrote: > JDK-8276681: Malformed Javadoc inline tags in JDK source > jdk/internal/net/http/ResponseSubscribers.java Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6486
Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal
On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian wrote:
> Here are the code changes for the "Deprecate finalizers in the standard Java
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code
> review.
>
> This change makes the indicated deprecations, and updates the API spec for
> JEP 421. It also updates the relevant @SuppressWarning annotations.
>
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).
Marked as reviewed by lancea (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/6465
Re: RFR: 8274949: Use String.contains() instead of String.indexOf() in java.base
On Fri, 17 Sep 2021 08:56:47 GMT, Andrey Turbanov wrote: > String.contains was introduced in Java 5. > Some code in java.base still uses old approach with `String.indexOf` to check > if String contains specified substring. > I propose to migrate such usages. Makes code shorter and easier to read. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5559
Re: RFR: 8274835: Remove unnecessary castings in java.base
On Thu, 9 Sep 2021 20:12:47 GMT, Andrey Turbanov wrote: > Redundant castings make code harder to read. > Found them by IntelliJ IDEA. > I tried to select only casts which are definitely safe to remove. Also didn't > touch primitive types casts. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5454
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v4]
On Wed, 22 Sep 2021 15:35:54 GMT, Pavel Rappo wrote: >> 8274075: Fix miscellaneous typos in java.base > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing "the" > > (Spotted by Brian Burkhalter.) Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v3]
On Wed, 22 Sep 2021 13:01:34 GMT, Pavel Rappo wrote: >> 8274075: Fix miscellaneous typos in java.base > > Pavel Rappo has updated the pull request incrementally with two additional > commits since the last revision: > > - Fix "non-white space" > >JDK predominantly uses "non-whitespace". There's only one more use of > "non-white space", in com/sun/tools/javac/tree/DocTreeMaker.java:716, which > will go away soon. > - Undo "wrapped" -> "wrapping" Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 16:48:53 GMT, Brian Burkhalter wrote: >> Pavel Rappo has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Tweak wording for Throwable constructor parameters > > src/java.base/share/classes/java/lang/Throwable.java line 68: > >> 66: * Further, doing so would tie the API of the upper layer to the details >> of >> 67: * its implementation, assuming the lower layer's exception was a checked >> 68: * exception. Throwing a "wrapping exception" (i.e., an exception >> containing a > > Are we sure that this should be "wrapping" and not "wrapped?" See also for > example `java.security.cert.CertPathValidatorException.` It does seem a bit strange to say "Throwing a wrapping" - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8274075: Fix miscellaneous typos in java.base [v2]
On Tue, 21 Sep 2021 13:16:02 GMT, Pavel Rappo wrote: >> 8274075: Fix miscellaneous typos in java.base > > Pavel Rappo has updated the pull request incrementally with one additional > commit since the last revision: > > Tweak wording for Throwable constructor parameters Overall looks like a good clean up. just not sure of "wrapping" vs. "wrapped" given the sentence starts with "Throwing". - PR: https://git.openjdk.java.net/jdk/pull/5610
Re: RFR: 8273401: Remove JarIndex support in URLClassPath
On Tue, 7 Sep 2021 07:03:20 GMT, Alan Bateman wrote: >> There is a bug for URLClassPath.findResources with JarIndex. >> With some discussions about the bug, the current priority is to remove the >> JAR index support in URLClassPath, >> and don’t need to do anything to the jar tool in the short term, except just >> to move JarIndex to the jdk.jartool module. >> >> The PR includes: >> 1. remove the JarIndex support in URLClassPath >> 2. move JarIndex into jdk.jartool module. > > src/java.base/share/classes/java/util/jar/JarFile.java line 220: > >> 218: * The index file name. >> 219: */ >> 220: public static final String INDEX_NAME = "META-INF/INDEX.LIST"; > > Adding this as a public field means it becomes part of the API, so it > shouldn't be public here. Agree - PR: https://git.openjdk.java.net/jdk/pull/5383
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
On Tue, 31 Aug 2021 02:08:48 GMT, Weijun Wang wrote: >> This change modifies the default value of the `java.security.manager` system >> property from "allow" to "disallow". This means unless it's explicitly set >> to "allow", any call to `System.setSecurityManager()` would throw an UOE. >> >> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are >> updated to confirm this behavior change. Two other tests are updated because >> they were added after JDK-8267184 and do not have >> `-Djava.security.manager=allow` on its `@run` line even it they need to >> install a `SecurityManager` at runtime. >> >> Please note that this code change requires jtreg to be upgraded to 6.1, >> where a security manager [will not be >> set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > sections etc Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote:
> This change modifies the default value of the `java.security.manager` system
> property from "allow" to "disallow". This means unless it's explicitly set to
> "allow", any call to `System.setSecurityManager()` would throw an UOE.
>
> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are
> updated to confirm this behavior change. Two other tests are updated because
> they were added after JDK-8267184 and do not have
> `-Djava.security.manager=allow` on its `@run` line even it they need to
> install one at runtime.
Looks good Max.
One minor suggestion below
src/java.base/share/classes/java/lang/SecurityManager.java line 128:
> 126: * null
> 127: * None
> 128: * Always throws {@code UnsupportedOperationException}
Not sure "Always" is needed, could just be "Throws UOE"
-
Marked as reviewed by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5204
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller [v2]
On Wed, 30 Jun 2021 15:45:25 GMT, Weijun Wang wrote: >> Add a cache to record which sources have called `System::setSecurityManager` >> and only print out warning lines once for each. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > update cache key from String to Class Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Mon, 28 Jun 2021 18:03:56 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. > > Note: this is copied from https://github.com/openjdk/jdk17/pull/152. The revisions you made as part of the push to JDK 18 look fine Max. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4615
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
On Fri, 25 Jun 2021 23:40:27 GMT, Weijun Wang wrote: >> More refactoring to limit the scope of `@SuppressWarnings` annotations. >> >> Sometimes I introduce new methods. Please feel free to suggest method names >> you like to use. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > one more Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. Changes look good Max - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v3]
On Mon, 14 Jun 2021 22:34:03 GMT, Weijun Wang wrote: >> More loudly and precise warning messages when a security manager is either >> enabled at startup or installed at runtime. >> >> This is new PR for the `openjdk/jdk17` repo copied from >> https://github.com/openjdk/jdk/pull/4400. A new commit is added. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > new warning text again (6/14) Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/13
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v6]
On Mon, 31 May 2021 15:02:57 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > default behavior reverted to allow Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267110: Update java.util to use instanceof pattern variable
On Tue, 18 May 2021 10:37:21 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.util` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Changes look good. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4088
Re: RFR: 8263190: Update java.io, java.math, and java.text to use instanceof pattern variable
On Mon, 8 Mar 2021 18:48:30 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.io`, > `java.math`, and `java.text` packages to make use of the `instanceof` pattern > variable? > > Kind regards, > Patrick Looks good - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2879
Re: RFR: JDK-8263104: fix warnings for empty paragraphs
On Fri, 5 Mar 2021 19:36:13 GMT, Jonathan Gibbons wrote: > Please review some simple cleanup for empty `` tags. > > Two of the tags are completely redundant, and simply deleted. > > The other three, in _package.html_ files are generally undesirable. Although > the presumed intent of the `id` declaration is to label the `@see` info, > proximity in the source code does not ensure proximity in the generated code. > The actual result is an empty paragraph at the end of the enclosing generated > ``, and before the generated output for the `@since` tag. > > The better solution is to move the `id` declaration into the `@see href...` and then delete the empty ``. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2850
Re: RFR: JDK-8262875: doccheck: empty paragraphs, etc in java.base module
On Tue, 2 Mar 2021 19:35:47 GMT, Jonathan Gibbons wrote: > Please review some minor doc fixes, for issues found by _doccheck_.There > are two kinds of errors that are addressed. > > 1. Incorrect use of ``. In HTML, `` marks the *beginning* of a > paragraph. It is not a terminator, to mark the end of a paragraph, or a > separator to mark the boundary between paragraphs. In particular, it should > not be used at the end of a description before a javadoc block tag, such as > `@param` or before other HTML block tags, like `` or ``. > > 2. References to the id `package-description`, following the recent > standardization of all ids generated by javadoc, Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2795
Re: RFR: 8253299: Manifest bytes are read twice when verifying a signed JAR
On Thu, 19 Nov 2020 17:08:21 GMT, Alan Bateman wrote: >> Small change to retrieve the raw bytes of manifest during verifying signed >> JAR. > > Marked as reviewed by alanb (Reviewer). I can sponsor once you integrate - PR: https://git.openjdk.java.net/jdk/pull/1299
Re: RFR: 8253299: Manifest bytes are read twice when verifying a signed JAR
On Wed, 18 Nov 2020 21:59:01 GMT, Hai-May Chao wrote: > Small change to retrieve the raw bytes of manifest during verifying signed > JAR. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1299
Re: RFR: 8253299: Manifest bytes are read twice when verifying a signed JAR
On Wed, 18 Nov 2020 21:59:01 GMT, Hai-May Chao wrote: > Small change to retrieve the raw bytes of manifest during verifying signed > JAR. The changes looks good. I am assuming that we do not need an additional test for this and if so, please add a noreg label such as noreg-trivial to the bug - PR: https://git.openjdk.java.net/jdk/pull/1299
Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag
On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons wrote: > The change is (just) to remove legacy usages of a JDK-private custom tag. looks fine - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/814
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v7]
On Wed, 14 Oct 2020 14:52:29 GMT, Lance Andersen wrote: >> Volker Simonis has refreshed the contents of this pull request, and previous >> commits have been removed. The incremental >> views will show differences compared to the previous content of the PR. > > The changes look good. Thank you for the updates. I have kicked off mach5 > jdk-tier1,jdk-tier2,jdk-tier3 runs across > all of the platforms. Mach5 run is clean :-) - PR: https://git.openjdk.java.net/jdk/pull/520
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v2]
On Mon, 12 Oct 2020 11:46:31 GMT, Volker Simonis wrote: >> I don't believe we discuss/reference the data descriptor for a Zip entry >> (outside of the PKWare Zip specification) so I >> am not sure we should reference it in the javadoc. >> Placing the sentence after "The default compression method will be used if >> no compression method was specified for the >> entry" makes sense > > Thanks for your input. I've tried to somehow merge both suggestions :) > > Did you had a chance to look at the CSR? I think somebody has to review it > before I can move it to "Finalized". > > Thank you and best regards, > Volker I have added myself as a reviewer based on the updates made by you and Alan - PR: https://git.openjdk.java.net/jdk/pull/520
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v7]
On Wed, 14 Oct 2020 10:54:30 GMT, Volker Simonis wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v6]
On Tue, 13 Oct 2020 19:07:33 GMT, Volker Simonis wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v6]
On Tue, 13 Oct 2020 19:07:33 GMT, Volker Simonis wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v6]
On Tue, 13 Oct 2020 19:07:33 GMT, Volker Simonis wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]
On Tue, 13 Oct 2020 18:46:59 GMT, Alan Bateman wrote: >> Volker Simonis has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now >> contains one commit: >> 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's >> compressed size > > src/java.base/share/classes/java/util/jar/JarOutputStream.java line 85: > >> 83: * ZipEntry#setCompressedSize(long)} method, then the compressed >> 84: * size will be set to the actual compressed size after deflation. >> 85: * > > I'm happy with the wording. What you think about put a p tag before "The > default compression method ..." so that it > goes into its own paragraph? I'm asking because it's inconsistent to have the > first paragraph include the details on > the compression method and the details on the current time pushed into a > second paragraph - if you generate the javadoc > then you'll see what I mean. I think that is a good idea to at the paragraph tag as you suggest - PR: https://git.openjdk.java.net/jdk/pull/520
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]
On Tue, 13 Oct 2020 11:40:36 GMT, Volker Simonis wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v4]
On Wed, 7 Oct 2020 17:04:02 GMT, Lance Andersen wrote: >> I totally agree. What about using just the last sentence (as you've >> proposed) in the spec section and add the other to >> as @implNote? O you think the last sentence will be enough? > > I think we can just go with the last sentence/paragraph. Perhaps we can > further simplify the paragraph/sentence with > something like: > The compressed entry size will be recalculated for compressed (DEFLATED) > entries when ZipEntry::setCompressedSize has > not been explicitly called on the ZipEntry. > or > > The compressed (DEFLATED) entry size will be recalculated when > ZipEntry::setCompressedSize has not been explicitly > called on the ZipEntry. I think the wording looks much better now - PR: https://git.openjdk.java.net/jdk/pull/520
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v3]
On Mon, 12 Oct 2020 15:49:29 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/util/jar/JarOutputStream.java line 81:
>>
>>> 79: * any previous entry. The default compression method will be
>>> 80: * used if no compression method was specified for the entry.
>>> 81: * When writing a compressed (i.e. {@code ZipEntry.DEFLATED}))
>>
>> I would leave as DEFLATED given it is used this way in other ZipOutputStream
>> methods and I would also remove the "i.e."
>
> I would prefer to keep it as "When writing as compressed (DEFLATED) ..." to
> keep the terminology consistent when
> linking to the setCompressedMethod.
Agree, no need for ZipEntry.DEFLATED which is what I was trying to say (sorry
if that was not clear). Only suggesting
ZipEntry.DEFLATED -> DEFLATED and remove the ".i.e."
-
PR: https://git.openjdk.java.net/jdk/pull/520
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v3]
On Mon, 12 Oct 2020 11:44:28 GMT, Volker Simonis wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v3]
On Mon, 12 Oct 2020 11:44:28 GMT, Volker Simonis wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v3]
On Mon, 12 Oct 2020 12:48:36 GMT, Alan Bateman wrote:
>> Volker Simonis has refreshed the contents of this pull request, and previous
>> commits have been removed. The incremental
>> views will show differences compared to the previous content of the PR. The
>> pull request contains one new commit since
>> the last revision:
>> 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's
>> compressed size
>
> src/java.base/share/classes/java/util/jar/JarOutputStream.java line 86:
>
>> 84: * compressed size will be set to the actual compressed size after
>> 85: * deflation. The current time will be used if the entry has no set
>> 86: * modification time.
>
> Thanks for combining the wording, I think this looks good. I'd probably drop
> "i.e." so that it's just "DEFLATED"
> parentheses.
I might consider making "The current time..." its own paragraph separate from
the compression discussion as I think it
would be clearer that way.
> src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 216:
>
>> 214: e.flag = 8;
>> 215: }
>> 216: else if (!e.manual_csize) {
>
> I assume the existing expression that set if csize has been set so that we
> don't set the flag to 8 in two branches.
I was thinking about that myself
-
PR: https://git.openjdk.java.net/jdk/pull/520
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v2]
On Sun, 11 Oct 2020 17:22:58 GMT, Alan Bateman wrote:
>> Volker Simonis has refreshed the contents of this pull request, and previous
>> commits have been removed. The incremental
>> views will show differences compared to the previous content of the PR.
>
> src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 189:
>
>> 187: * {@code ZipEntry.DEFLATED}) entries when {@link
>> ZipEntry#setCompressedSize(long)}
>> 188: * has not been explicitly called on the {@code ZipEntry}.
>> 189: *
>
> Here's an alternative that might be a bit clearer to readers.
>
> "When writing a compressed (deflated) entry, and the compressed size has not
> been explicitly set with the
> setCompressedSize method, then the compressed size written to the entry's
> data descriptor will be its actual compressed
> size." I think we should put it after the "The default compression method
> .." sentence and move the sentence on the
> time stamp to the end.
I don't believe we discuss/reference the data descriptor for a Zip entry
(outside of the PKWare Zip specification) so I
am not sure we should reference it in the javadoc.
Placing the sentence after "The default compression method will be used if no
compression method was specified for the
entry" makes sense
-
PR: https://git.openjdk.java.net/jdk/pull/520
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to recalculate ZipEntry's compressed size [v2]
On Fri, 9 Oct 2020 10:28:34 GMT, Volker Simonis wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v3]
On Thu, 1 Oct 2020 14:42:21 GMT, Jaikiran Pai wrote: >> Can I please get a review and a sponsor for a fix for >> https://bugs.openjdk.java.net/browse/JDK-8242882? >> >> As noted in that JBS issue, if the size of the Manifest entry in the jar >> happens to be very large (such that it exceeds >> the `Integer.MAX_VALUE`), then the current code in `JarFile#getBytes` can >> lead to a `NegativeArraySizeException`. This >> is due to the: if (len != -1 && len <= 65535) block which evaluates to >> `true` when the size of the manifest entry is >> larger than `Integer.MAX_VALUE`. As a result, this then ends up calling the >> code which can lead to the >> `NegativeArraySizeException`. The commit in this PR fixes that issue by >> changing those `if/else` blocks to prevent >> this issue and instead use a code path that leads to the >> `InputStream#readAllBytes()` which internally has the >> necessary checks to throw the expected `OutOfMemoryError`. This commit also >> includes a jtreg test case which >> reproduces the issue and verifies the fix. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Second round of review comments addressed Hi Jaikiran, Yes I think you are OK to move forward with the integration, - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 8253952: Refine ZipOutputStream.putNextEntry() to ignore ZipEntry's compressed size
On Wed, 7 Oct 2020 16:43:53 GMT, Volker Simonis wrote: >> Alan makes a good point. Perhaps we keep things simple and focus solely on >> tweaking the description of the change in >> behavior which is described in your last paragraph > > I totally agree. What about using just the last sentence (as you've proposed) > in the spec section and add the other to > as @implNote? O you think the last sentence will be enough? I think we can just go with the last sentence/paragraph. Perhaps we can further simplify the paragraph/sentence with something like: The compressed entry size will be recalculated for compressed (DEFLATED) entries when ZipEntry::setCompressedSize has not been explicitly called on the ZipEntry. or The compressed (DEFLATED) entry size will be recalculated when ZipEntry::setCompressedSize has not been explicitly called on the ZipEntry. - PR: https://git.openjdk.java.net/jdk/pull/520
Re: RFR: 8253952: Work around wrong usage of ZipOutputStream.putNextEntry() in user code
On Tue, 6 Oct 2020 10:02:09 GMT, Volker Simonis wrote:
> ### Summary
>
> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
> which can lead to the `ZipException "invalid
> entry compressed size"`.
> ### Motivation
>
> In general it is not safe to directly write a ZipEntry obtained from
> `ZipInputStream.getNextEntry()`,
> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
> `ZipOutputStream.putNextEntry()` to a
> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
> and write it to the `ZipOutputStream` as
> follows:
> ZipEntry entry;
> ZipInputStream zis = new ZipInputStream(...);
> ZipOutputStream zos = new ZipOutputStream(...);
> while((entry = zis.getNextEntry()) != null) {
> zos.putNextEntry(entry);
> zis.transferTo(zos);
> }
> The problem with this code is that the zip file format does not record the
> compression level used for deflation in its
> entries. In general, it doesn't even mandate a predefined compression ratio
> per compression level. Therefore the
> compressed size recorded in a `ZipEntry` read from a zip file might differ
> from the new compressed size produced by the
> receiving `ZipOutputStream`. Such a difference will result in a
> `ZipException` with the following message:
> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
> got 7 bytes)
>
> The correct way of copying all entries from one zip file into another
> requires the creation of a new `ZipEntry` or at
> least resetting of the compressed size field. E.g.:
> while((entry = zis.getNextEntry()) != null) {
> ZipEntry newEntry = new ZipEntry(entry.getName());
> zos.putNextEntry(newEntry);
> zis.transferTo(zos);
> }
> or:
> while((entry = zis.getNextEntry()) != null) {
> entry.setCompressedSize(-1);
> zos.putNextEntry(entry);
> zis.transferTo(zos);
> }
> Unfortunately, there's a lot of user code out there which gets this wrong and
> uses the bad coding pattern described
> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
> size (expected 12 but got 7 bytes)"` gives
> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
> instances of this anti-pattern on GitHub when
> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
> wrapper task][1] is affected as well as the
> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
> occurrences of this pattern in OpenJDK (see
> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
> (e.g.
> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
> :). ### Description So while this has
> clearly been a problem before, it apparently wasn't painful enough to trigger
> any action from the side of the JDK.
> However, recently quite some zlib forks with [superior deflate/inflate
> performance have evolved][6]. Using them with
> OpenJDK is quite straight-forward: one just has to configure the alternative
> implementations by setting
> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
> using these new zlib implementations for
> selected services in production and the only reason why we haven't enabled
> them by default until now is the problem
> I've just described. The reason why these new libraries uncover the described
> anti-pattern much more often is because
> their compression ratio is slightly different from that of the default zlib
> library. This can easily trigger a
> `ZipException` even if an application is not using a different compression
> levels but just a zip file created with
> another zlib version. I'd therefore like to propose the following workaround
> for the wrong
> `ZipOutputStream.putNextEntry()` usage in user code:
> - ignore the compressed size if it was implicitly determined from the zip
> file and not explicitly set by calling
>`ZipEntry.setCompressedSize()`.
>
> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
> `JarOutputStream.putNextEntry()` to explain the
> problem and why `putNextEntry()` will ignore the compressed size of a
> `ZipEntry` if that was set implicitely when
> reading that entry from a `ZipFile` or `ZipInputStream`.
>
>
> ### Technical Details
>
> A zip file consists of a stream of File Entries followed by a Central
> Directory (see [here for a more detailed
> specification][7]). Each File Entry is composed of a Local File Header (LFH)
> followed by the compressed Data and an
> optional Data Descriptor. The LFH contains the File Name and among other
> attributes the Compressed and Uncompressed
> size and CRC of the Data. In the case where the latter three attributes are
> not available at the time when the LFH is
> created, this fact will be recorded in a flag of the LFH and will trigger the
> creation of a Data Descriptor with the
> corresponding information right after the Data
Re: RFR: 8253952: Work around wrong usage of ZipOutputStream.putNextEntry() in user code
On Wed, 7 Oct 2020 15:10:06 GMT, Alan Bateman wrote:
>> ### Summary
>>
>> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
>> which can lead to the `ZipException "invalid
>> entry compressed size"`.
>> ### Motivation
>>
>> In general it is not safe to directly write a ZipEntry obtained from
>> `ZipInputStream.getNextEntry()`,
>> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
>> `ZipOutputStream.putNextEntry()` to a
>> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
>> and write it to the `ZipOutputStream` as
>> follows:
>> ZipEntry entry;
>> ZipInputStream zis = new ZipInputStream(...);
>> ZipOutputStream zos = new ZipOutputStream(...);
>> while((entry = zis.getNextEntry()) != null) {
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> The problem with this code is that the zip file format does not record the
>> compression level used for deflation in its
>> entries. In general, it doesn't even mandate a predefined compression ratio
>> per compression level. Therefore the
>> compressed size recorded in a `ZipEntry` read from a zip file might differ
>> from the new compressed size produced by the
>> receiving `ZipOutputStream`. Such a difference will result in a
>> `ZipException` with the following message:
>> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
>> got 7 bytes)
>>
>> The correct way of copying all entries from one zip file into another
>> requires the creation of a new `ZipEntry` or at
>> least resetting of the compressed size field. E.g.:
>> while((entry = zis.getNextEntry()) != null) {
>> ZipEntry newEntry = new ZipEntry(entry.getName());
>> zos.putNextEntry(newEntry);
>> zis.transferTo(zos);
>> }
>> or:
>> while((entry = zis.getNextEntry()) != null) {
>> entry.setCompressedSize(-1);
>> zos.putNextEntry(entry);
>> zis.transferTo(zos);
>> }
>> Unfortunately, there's a lot of user code out there which gets this wrong
>> and uses the bad coding pattern described
>> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
>> size (expected 12 but got 7 bytes)"` gives
>> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
>> instances of this anti-pattern on GitHub when
>> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
>> wrapper task][1] is affected as well as the
>> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
>> occurrences of this pattern in OpenJDK (see
>> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
>> (e.g.
>> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
>> :). ### Description So while this has
>> clearly been a problem before, it apparently wasn't painful enough to
>> trigger any action from the side of the JDK.
>> However, recently quite some zlib forks with [superior deflate/inflate
>> performance have evolved][6]. Using them with
>> OpenJDK is quite straight-forward: one just has to configure the alternative
>> implementations by setting
>> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
>> using these new zlib implementations for
>> selected services in production and the only reason why we haven't enabled
>> them by default until now is the problem
>> I've just described. The reason why these new libraries uncover the
>> described anti-pattern much more often is because
>> their compression ratio is slightly different from that of the default zlib
>> library. This can easily trigger a
>> `ZipException` even if an application is not using a different compression
>> levels but just a zip file created with
>> another zlib version. I'd therefore like to propose the following
>> workaround for the wrong
>> `ZipOutputStream.putNextEntry()` usage in user code:
>> - ignore the compressed size if it was implicitly determined from the zip
>> file and not explicitly set by calling
>>`ZipEntry.setCompressedSize()`.
>>
>> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
>> `JarOutputStream.putNextEntry()` to explain the
>> problem and why `putNextEntry()` will ignore the compressed size of a
>> `ZipEntry` if that was set implicitely when
>> reading that entry from a `ZipFile` or `ZipInputStream`.
>>
>>
>> ### Technical Details
>>
>> A zip file consists of a stream of File Entries followed by a Central
>> Directory (see [here for a more detailed
>> specification][7]). Each File Entry is composed of a Local File Header (LFH)
>> followed by the compressed Data and an
>> optional Data Descriptor. The LFH contains the File Name and among other
>> attributes the Compressed and Uncompressed
>> size and CRC of the Data. In the case where the latter three attributes are
>> not available at the time when the LFH is
>> created, this fact will be recorded in a flag of the LFH and will trigger
Re: RFR: 8253952: Work around wrong usage of ZipOutputStream.putNextEntry() in user code
On Tue, 6 Oct 2020 10:02:09 GMT, Volker Simonis wrote:
> ### Summary
>
> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
> which can lead to the `ZipException "invalid
> entry compressed size"`.
> ### Motivation
>
> In general it is not safe to directly write a ZipEntry obtained from
> `ZipInputStream.getNextEntry()`,
> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
> `ZipOutputStream.putNextEntry()` to a
> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
> and write it to the `ZipOutputStream` as
> follows:
> ZipEntry entry;
> ZipInputStream zis = new ZipInputStream(...);
> ZipOutputStream zos = new ZipOutputStream(...);
> while((entry = zis.getNextEntry()) != null) {
> zos.putNextEntry(entry);
> zis.transferTo(zos);
> }
> The problem with this code is that the zip file format does not record the
> compression level used for deflation in its
> entries. In general, it doesn't even mandate a predefined compression ratio
> per compression level. Therefore the
> compressed size recorded in a `ZipEntry` read from a zip file might differ
> from the new compressed size produced by the
> receiving `ZipOutputStream`. Such a difference will result in a
> `ZipException` with the following message:
> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
> got 7 bytes)
>
> The correct way of copying all entries from one zip file into another
> requires the creation of a new `ZipEntry` or at
> least resetting of the compressed size field. E.g.:
> while((entry = zis.getNextEntry()) != null) {
> ZipEntry newEntry = new ZipEntry(entry.getName());
> zos.putNextEntry(newEntry);
> zis.transferTo(zos);
> }
> or:
> while((entry = zis.getNextEntry()) != null) {
> entry.setCompressedSize(-1);
> zos.putNextEntry(entry);
> zis.transferTo(zos);
> }
> Unfortunately, there's a lot of user code out there which gets this wrong and
> uses the bad coding pattern described
> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
> size (expected 12 but got 7 bytes)"` gives
> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
> instances of this anti-pattern on GitHub when
> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
> wrapper task][1] is affected as well as the
> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
> occurrences of this pattern in OpenJDK (see
> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
> (e.g.
> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
> :). ### Description So while this has
> clearly been a problem before, it apparently wasn't painful enough to trigger
> any action from the side of the JDK.
> However, recently quite some zlib forks with [superior deflate/inflate
> performance have evolved][6]. Using them with
> OpenJDK is quite straight-forward: one just has to configure the alternative
> implementations by setting
> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
> using these new zlib implementations for
> selected services in production and the only reason why we haven't enabled
> them by default until now is the problem
> I've just described. The reason why these new libraries uncover the described
> anti-pattern much more often is because
> their compression ratio is slightly different from that of the default zlib
> library. This can easily trigger a
> `ZipException` even if an application is not using a different compression
> levels but just a zip file created with
> another zlib version. I'd therefore like to propose the following workaround
> for the wrong
> `ZipOutputStream.putNextEntry()` usage in user code:
> - ignore the compressed size if it was implicitly determined from the zip
> file and not explicitly set by calling
>`ZipEntry.setCompressedSize()`.
>
> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
> `JarOutputStream.putNextEntry()` to explain the
> problem and why `putNextEntry()` will ignore the compressed size of a
> `ZipEntry` if that was set implicitely when
> reading that entry from a `ZipFile` or `ZipInputStream`.
>
>
> ### Technical Details
>
> A zip file consists of a stream of File Entries followed by a Central
> Directory (see [here for a more detailed
> specification][7]). Each File Entry is composed of a Local File Header (LFH)
> followed by the compressed Data and an
> optional Data Descriptor. The LFH contains the File Name and among other
> attributes the Compressed and Uncompressed
> size and CRC of the Data. In the case where the latter three attributes are
> not available at the time when the LFH is
> created, this fact will be recorded in a flag of the LFH and will trigger the
> creation of a Data Descriptor with the
> corresponding information right after the Data
Re: RFR: 8253952: Work around wrong usage of ZipOutputStream.putNextEntry() in user code
On Tue, 6 Oct 2020 10:02:09 GMT, Volker Simonis wrote:
> ### Summary
>
> Work around wrong usage of `ZipOutputStream.putNextEntry()` in user code
> which can lead to the `ZipException "invalid
> entry compressed size"`.
> ### Motivation
>
> In general it is not safe to directly write a ZipEntry obtained from
> `ZipInputStream.getNextEntry()`,
> `ZipFile.entries()`, `ZipFile.getEntry()` or `ZipFile.stream()` with
> `ZipOutputStream.putNextEntry()` to a
> `ZipOutputStream` and then read the entries data from the `ZipInputStream`
> and write it to the `ZipOutputStream` as
> follows:
> ZipEntry entry;
> ZipInputStream zis = new ZipInputStream(...);
> ZipOutputStream zos = new ZipOutputStream(...);
> while((entry = zis.getNextEntry()) != null) {
> zos.putNextEntry(entry);
> zis.transferTo(zos);
> }
> The problem with this code is that the zip file format does not record the
> compression level used for deflation in its
> entries. In general, it doesn't even mandate a predefined compression ratio
> per compression level. Therefore the
> compressed size recorded in a `ZipEntry` read from a zip file might differ
> from the new compressed size produced by the
> receiving `ZipOutputStream`. Such a difference will result in a
> `ZipException` with the following message:
> java.util.zip.ZipException: invalid entry compressed size (expected 12 but
> got 7 bytes)
>
> The correct way of copying all entries from one zip file into another
> requires the creation of a new `ZipEntry` or at
> least resetting of the compressed size field. E.g.:
> while((entry = zis.getNextEntry()) != null) {
> ZipEntry newEntry = new ZipEntry(entry.getName());
> zos.putNextEntry(newEntry);
> zis.transferTo(zos);
> }
> or:
> while((entry = zis.getNextEntry()) != null) {
> entry.setCompressedSize(-1);
> zos.putNextEntry(entry);
> zis.transferTo(zos);
> }
> Unfortunately, there's a lot of user code out there which gets this wrong and
> uses the bad coding pattern described
> before. Searching for `"java.util.zip.ZipException: invalid entry compressed
> size (expected 12 but got 7 bytes)"` gives
> ~2500 hits (~100 on StackOverflow). It's also no hard to find plenty of
> instances of this anti-pattern on GitHub when
> doing a code search for `ZipEntry` and `putNextEntry()`. E.g. [Gradle 4.x
> wrapper task][1] is affected as well as the
> latest version of the [mockableAndroidJar task][2]. I've recently fixed two
> occurrences of this pattern in OpenJDK (see
> [JDK-8240333][3] and [JDK-8240235][4]) but there still exist more of them
> (e.g.
> [`test/jdk/java/util/zip/ZipFile/CopyJar.java`][5] which is there since 1999
> :). ### Description So while this has
> clearly been a problem before, it apparently wasn't painful enough to trigger
> any action from the side of the JDK.
> However, recently quite some zlib forks with [superior deflate/inflate
> performance have evolved][6]. Using them with
> OpenJDK is quite straight-forward: one just has to configure the alternative
> implementations by setting
> `LD_LIBRARY_PATH` or `LD_PRELOAD` correspondingly. We've seen big saving by
> using these new zlib implementations for
> selected services in production and the only reason why we haven't enabled
> them by default until now is the problem
> I've just described. The reason why these new libraries uncover the described
> anti-pattern much more often is because
> their compression ratio is slightly different from that of the default zlib
> library. This can easily trigger a
> `ZipException` even if an application is not using a different compression
> levels but just a zip file created with
> another zlib version. I'd therefore like to propose the following workaround
> for the wrong
> `ZipOutputStream.putNextEntry()` usage in user code:
> - ignore the compressed size if it was implicitly determined from the zip
> file and not explicitly set by calling
>`ZipEntry.setCompressedSize()`.
>
> - Change the API-documentation of `ZipOutputStream.putNextEntry()` and
> `JarOutputStream.putNextEntry()` to explain the
> problem and why `putNextEntry()` will ignore the compressed size of a
> `ZipEntry` if that was set implicitely when
> reading that entry from a `ZipFile` or `ZipInputStream`.
>
>
> ### Technical Details
>
> A zip file consists of a stream of File Entries followed by a Central
> Directory (see [here for a more detailed
> specification][7]). Each File Entry is composed of a Local File Header (LFH)
> followed by the compressed Data and an
> optional Data Descriptor. The LFH contains the File Name and among other
> attributes the Compressed and Uncompressed
> size and CRC of the Data. In the case where the latter three attributes are
> not available at the time when the LFH is
> created, this fact will be recorded in a flag of the LFH and will trigger the
> creation of a Data Descriptor with the
> corresponding information right after the Data
Re: RFR: 8242882: opening jar file with large manifest might throw NegativeArraySizeException [v2]
On Wed, 30 Sep 2020 17:26:18 GMT, Brent Christian wrote:
>> test/jdk/java/util/jar/JarFile/LargeManifestOOMTest.java line 60:
>>
>>> 58: final OutOfMemoryError oome =
>>> Assert.expectThrows(OutOfMemoryError.class, () -> jar.getManifest());
>>> 59: // additionally verify that the OOM was for the right/expected
>>> reason
>>> 60: if (!"Required array size too large".equals(oome.getMessage()))
>>> {
>>
>> I wasn't too sure if I should add this additional check on the message of
>> the `OutOfMemoryError`, but decided to do it
>> anyway, given that from what I remember there were some discussions in the
>> `core-libs-dev` list a while back on the
>> exact messages that such OOMs should throw. So I guessed that it might be OK
>> to rely on those messages in the tests
>> within this project. However, I am open to removing specific check if it's
>> considered unnecessary.
>
> I think it's fine either way.
If you are going to validate the message, which I probably would not, it would
be important to make sure it document
if the message is changed in JarFile::getBytes, that the test needs updated.
Otherwise it will be easy to miss.
-
PR: https://git.openjdk.java.net/jdk/pull/323
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 16:59:59 GMT, Lance Andersen wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove "final" > > I am fine with this as well. I will pull over the change and just sanity > check it via mach5 and then will sponsor @jaikiran Please go ahead and integrate this and I can then sponsor (has to be done in that order) - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files [v2]
On Mon, 7 Sep 2020 18:57:11 GMT, Sean Coffey wrote: >> Continuation of RFR thread from >> http://mail.openjdk.java.net/pipermail/security-dev/2020-August/022373.html >> >> CSR has been approved. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Copyright and test clean up Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/56
Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files
On Mon, 7 Sep 2020 13:48:57 GMT, Sean Coffey wrote: > Continuation of RFR thread from > http://mail.openjdk.java.net/pipermail/security-dev/2020-August/022373.html > > CSR has been approved. I think this looks good over all Sean. In your SymLinkTest, I probably would have the test delete the file if it exists prior to writing to it. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/56
Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files
Hi Sean,
I think the changes are OK in the latest version. A couple of the files have a
2019 copyright still.
> On Aug 27, 2020, at 10:58 AM, Weijun Wang wrote:
>
>
>
>> On Aug 27, 2020, at 10:36 AM, Seán Coffey wrote:
>>
>> Thanks for the review Max. Comments inline..
>>
>> On 27/08/2020 14:45, Weijun Wang wrote:
>>> I’m OK with using one warning, but prefer it to a little more formal like
>>> "POSIX file permission and/or symlink attributes detected…”.
>>>
>>> One nit in ZipFile.java:
>>>
>>> 1098 // only set posix perms value via ZipEntry constructor
>>> for now
>>> 1099 @Override
>>> 1100 public int getExtraAttributes(ZipEntry ze) {
>>>
>>> Maybe you can just remove the comment.
>>>
>>> Do you also want to rename the “posixPermsDetected" field and loacl
>>> variable “perms” in JarSigner.java?
>>
>> Good points. Edits made.
>>
>> http://cr.openjdk.java.net/~coffeys/webrev.8250968.v3/webrev/
>>
>>>
>>> I’m not sure about the test but if zipfs is able to keep permissions inside
>>> a zip file then that POSIX byte (or whatever it’s named) is already there
>>> and we can modify it to include more bits.
>>
>> Looks like it was a conscious design decision to only allow recording of
>> POSIX permission bits for this field (& 0xFFF). I don't see anything about
>> symlink support in zipfs docs.
>
> As long as that *byte* is there and it’s not difficult to locate, we can
> manually add the *bit* for symlink and see if jarsigner can keep it.
We can create an RFE for adding support for this with Zip FS.
>
> —Max
>
>>
>> regards,
>> Sean.
>>
>>>
>>> No other comment.
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>>> On Aug 27, 2020, at 3:26 AM, Seán Coffey wrote:
>>>>
>>>> updated webrev:
>>>> http://cr.openjdk.java.net/~coffeys/webrev.8250968.v2/webrev/
>>>>
>>>> regards,
>>>> Sean.
>>>>
>>>> On 27/08/2020 07:42, Seán Coffey wrote:
>>>>> Hi Max,
>>>>>
>>>>> I looked at updating the warning string but figured that it might have
>>>>> been of no interest to end users. How about this edit then ?
>>>>>
>>>>> +{"posix.attributes.detected", "POSIX file permission attributes
>>>>> detected. These attributes are ignored when signing and are not protected
>>>>> by the signature."},
>>>>>
>>>>>>> replace with:
>>>>> +{"extra.attributes.detected", "POSIX file permission/symlink
>>>>> attributes detected. These attributes are ignored when signing and are
>>>>> not protected by the signature."},
>>>>>
>>>>> regards,
>>>>> Sean.
>>>>>
>>>>> On 26/08/2020 23:15, Weijun Wang wrote:
>>>>>> Are you going to update the warning text or create a new one?
>>>>>>
>>>>>> Thanks,
>>>>>> Max
>>>>>>
>>>>>>> On Aug 26, 2020, at 2:26 PM, Seán Coffey wrote:
>>>>>>>
>>>>>>> This is a follow on from the recent 8218021 fix. The jarsigner tool
>>>>>>> removes symlink attribute data from zipfiles when signing them.
>>>>>>> jarsigner should preserve this data. The fix involves preserving the 16
>>>>>>> bits associated with the file attributes (instead of the current 12).
>>>>>>> That's done in ZipFile. All other changes are just a refactor of the
>>>>>>> variable name.
>>>>>>>
>>>>>>> I haven't been able to automate a test for this since zipfs doesn't
>>>>>>> seem to support symlinks. Manual testing looks good.
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8250968
>>>>>>> http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html
>>>>>>>
>>>>>>> regards,
>>>>>>> Sean.
Best
Lance
--
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: 8218021: Have jarsigner preserve posix permission attributes
Hi Sean, I think the changes look good including the proposed tweaks to the message suggested by Alan. Best Lance > On Jul 2, 2020, at 4:10 AM, Seán Coffey wrote: > > Thanks for the review Alan. I'm in contact with Max already about possible > follow up enhancements in this area. It would be worked via a follow on JBS > record. > > Regarding the error message, I'm fine with your suggestion. We can go with > this then: > "POSIX file permission attributes detected. These attributes are ignored when > signing and are not protected by the signature." > > regards, > Sean. > > On 02/07/2020 08:59, Alan Bateman wrote: >> On 30/06/2020 14:51, Seán Coffey wrote: >>> >>> : >>> >>> During the CSR review, a suggestion was made to have jarsigner preserve >>> such attributes by default. Warnings about these attributes will also be >>> added during signing and verify operations (if detected). >>> >> Yes, signing should be additive so the original proposal to drop information >> from the UNIX extra block would be surprising. The intersection of those >> using zip/other tools to create zip files and then signing them with >> jarsigner is probably small but it would still be confusing for signing to >> loose information. Having jarsigner refuse to sign these zip files by >> default, with an option to override, would be a reasonable approach. The >> current proposal to printing a warning seems okay too. >> >> I've skimmed through webrev.8218021.v5 which has this warning: >> >> "POSIX file permission attributes detected. Note that these attributes are >> unsigned and not protected by the signature." >> >> I realize you've agreed this with the other Reviewers but I think that "Note >> that these attributes are unsigned ..." is confusing as it could be >> interpreted to mean that they have to be signed by some other means, or even >> that the warning is because they are using unsigned values. >> >> It might be better to tweak the second part to make it a bit clearer, up to >> you but something like "These attributes are ignored when signing and are >> not protected by the signature". >> >> -Alan Best Lance -- 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: 8218021: jarsigner strips the execute permission when signing a .zip file
HI Sean, Looks OK based on our exchanges. Thank you for your time on this one! Best Lance > On Jun 22, 2020, at 7:22 AM, Seán Coffey wrote: > > Thanks Lance. > > I've updated the patch with some extra offline feedback from yourself and Max. > A new warning is printed with use of the new flag. A warning is also printed > when file posix permissions are detected on resources being signed. Test > updated for that also. > > https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/ > <https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/> > regards, > Sean. > > On 12/06/2020 17:05, Lance Andersen wrote: >> Hi Sean, >> >> I think your changes look fine so all good FMPOV. >> >> Best >> Lance >> >>> On Jun 12, 2020, at 6:21 AM, Seán Coffey >> <mailto:sean.cof...@oracle.com>> wrote: >>> >>> Hi, >>> >>> I'd like to reboot this jarsigner enhancement request[1]. I've removed the >>> problem references to zip file name extensions. Instead, there's a new JDK >>> implementation specific jarsigner option: -keepposixperms >>> >>> https://bugs.openjdk.java.net/browse/JDK-8218021 >>> <https://bugs.openjdk.java.net/browse/JDK-8218021> >>> https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/ >>> <https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/> >>> >>> regards, >>> Sean. >>> >>> [1] >>> http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html >>> >>> <http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html> >>> >> >> >> <http://oracle.com/us/design/oracle-email-sig-198324.gif> >> <http://oracle.com/us/design/oracle-email-sig-198324.gif> >> <http://oracle.com/us/design/oracle-email-sig-198324.gif> >> <http://oracle.com/us/design/oracle-email-sig-198324.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 <mailto:lance.ander...@oracle.com> >> >> >> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.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 <mailto:lance.ander...@oracle.com>
