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() [v2]
On Thu, 10 Feb 2022 20:35:19 GMT, Sean Mullan wrote:
>> 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.
>
> 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.
-
PR: https://git.openjdk.java.net/jdk/pull/7348
Re: RFR: 8280409: JarFile::verifiableEntry can fail with NPE accessing ze.getName() [v2]
On Wed, 9 Feb 2022 21:16:08 GMT, Lance Andersen wrote:
>>> 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.
Agree on returning null to maintain current behavior. I would also lean towards
amending the specification to specify what has been long-standing 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: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:11:38 GMT, Lance Andersen wrote: > 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. - 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 Tue, 8 Feb 2022 15:57:00 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/util/jar/JarFile.java line 871:
>>
>>> 869: }
>>> 870: // ZipEntry::getName should not return null
>>> 871: if(ze.getName() != null) {
>>
>> Nit, add space after "if"
>
> 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!
-
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:27:46 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 871:
>
>> 869: }
>> 870: // ZipEntry::getName should not return null
>> 871: if(ze.getName() != null) {
>
> Nit, add space after "if"
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 is null.
> src/java.base/share/classes/java/util/jar/JarFile.java line 877:
>
>> 875: }
>> 876: // ZipEntry returned from JarFile::getJarEntry should not be
>> null
>> 877: if(ze == null) {
>
> Nit, add space after "if"
ze can't be null here.
-
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 23:02:54 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
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reduce Exception checking to JarFile::verifiableEntry
Marked as reviewed by mullan (Reviewer).
src/java.base/share/classes/java/util/jar/JarFile.java line 871:
> 869: }
> 870: // ZipEntry::getName should not return null
> 871: if(ze.getName() != null) {
Nit, add space after "if"
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 "!".
src/java.base/share/classes/java/util/jar/JarFile.java line 877:
> 875: }
> 876: // ZipEntry returned from JarFile::getJarEntry should not be null
> 877: if(ze == null) {
Nit, add space after "if"
-
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
