RFR: 8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards

Ziyi Luo
All of the "isAssignableFrom" checks in "engineGetKeySpec" appear to be backwards in Java's KeyFactorySpi.engineGetKeySpec implementations. In most cases, the requested KeySpec is equal to the concrete implementation so the inversion does not matter. But there are few cases, as presented in the added jtreg test, will cause unexpected behavior (e.g., ClassCastException rather than an InvalidKeySpecException). The fix is trivial.

-------------

Commit messages:
 - 8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards

Changes: https://git.openjdk.java.net/jdk/pull/2682/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2682&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8254717
  Stats: 112 lines in 15 files changed: 71 ins; 0 del; 41 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2682.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2682/head:pull/2682

PR: https://git.openjdk.java.net/jdk/pull/2682
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards

Jamil Nimeh-2
On Tue, 23 Feb 2021 01:47:48 GMT, Ziyi Luo <[hidden email]> wrote:

> All of the "isAssignableFrom" checks in "engineGetKeySpec" appear to be backwards in Java's KeyFactorySpi.engineGetKeySpec implementations. In most cases, the requested KeySpec is equal to the concrete implementation so the inversion does not matter. But there are few cases, as presented in the added jtreg test, will cause unexpected behavior (e.g., ClassCastException rather than an InvalidKeySpecException). The fix is trivial.
>
> Co-author @SalusaSecondus

The fix itself makes sense and looks good to me.  However I think it will cause two other tests to break.  Please try running the following two tests with your changes and see if they fail for you as they did for me:
open/test/jdk/sun/security/rsa/TestKeyFactory.java
open/test/jdk/sun/security/rsa/pss/TestPSSKeySupport.java

Also just a minor nit: some of the modified files should have their copyright dates updated to 2021.

test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 2:

> 1: /*
> 2:  * Copyright (c) 2011, Amazon.com, Inc. or its affiliates. All rights reserved.

Nit: Should this be 2021?

test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 27:

> 25:  * @test
> 26:  * @bug 8254717
> 27:  * @summary Two users facing implications caused by "isAssignableFrom" checks in "engineGetKeySpec" being backwards.

Nit: I think standard practice is to have the summary be the synopsis from JDK-8254717 (isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2682
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards [v2]

Ziyi Luo
In reply to this post by Ziyi Luo
> All of the "isAssignableFrom" checks in "engineGetKeySpec" appear to be backwards in Java's KeyFactorySpi.engineGetKeySpec implementations. In most cases, the requested KeySpec is equal to the concrete implementation so the inversion does not matter. But there are few cases, as presented in the added jtreg test, will cause unexpected behavior (e.g., ClassCastException rather than an InvalidKeySpecException). The fix is trivial.
>
> Co-author @SalusaSecondus

Ziyi Luo has updated the pull request incrementally with one additional commit since the last revision:

  8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards (r2)

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2682/files
  - new: https://git.openjdk.java.net/jdk/pull/2682/files/0e4956db..d1972302

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2682&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2682&range=00-01

  Stats: 26 lines in 15 files changed: 2 ins; 0 del; 24 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2682.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2682/head:pull/2682

PR: https://git.openjdk.java.net/jdk/pull/2682
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards

Ziyi Luo
In reply to this post by Jamil Nimeh-2
On Wed, 24 Feb 2021 02:39:57 GMT, Jamil Nimeh <[hidden email]> wrote:

>> All of the "isAssignableFrom" checks in "engineGetKeySpec" appear to be backwards in Java's KeyFactorySpi.engineGetKeySpec implementations. In most cases, the requested KeySpec is equal to the concrete implementation so the inversion does not matter. But there are few cases, as presented in the added jtreg test, will cause unexpected behavior (e.g., ClassCastException rather than an InvalidKeySpecException). The fix is trivial.
>>
>> Co-author @SalusaSecondus
>
> The fix itself makes sense and looks good to me.  However I think it will cause two other tests to break.  Please try running the following two tests with your changes and see if they fail for you as they did for me:
> open/test/jdk/sun/security/rsa/TestKeyFactory.java
> open/test/jdk/sun/security/rsa/pss/TestPSSKeySupport.java
>
> Also just a minor nit: some of the modified files should have their copyright dates updated to 2021.

Hi Jamil,

Thanks for reviewing this PR.

> The fix itself makes sense and looks good to me. However I think it will cause two other tests to break. Please try running the following two tests with your changes and see if they fail for you as they did for me:
> open/test/jdk/sun/security/rsa/TestKeyFactory.java
> open/test/jdk/sun/security/rsa/pss/TestPSSKeySupport.java

Nice catch. I updated both tests in rev-2.

> Also just a minor nit: some of the modified files should have their copyright dates updated to 2021.

Done

-------------

PR: https://git.openjdk.java.net/jdk/pull/2682
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards [v2]

Ziyi Luo
In reply to this post by Jamil Nimeh-2
On Wed, 24 Feb 2021 02:40:36 GMT, Jamil Nimeh <[hidden email]> wrote:

>> Ziyi Luo has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards (r2)
>
> test/jdk/java/security/KeyFactory/KeyFactoryGetKeySpecForInvalidSpec.java line 2:
>
>> 1: /*
>> 2:  * Copyright (c) 2011, Amazon.com, Inc. or its affiliates. All rights reserved.
>
> Nit: Should this be 2021?

Good catch! Fixed

-------------

PR: https://git.openjdk.java.net/jdk/pull/2682
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards [v2]

Jamil Nimeh-2
In reply to this post by Ziyi Luo
On Tue, 2 Mar 2021 17:26:15 GMT, Ziyi Luo <[hidden email]> wrote:

>> All of the "isAssignableFrom" checks in "engineGetKeySpec" appear to be backwards in Java's KeyFactorySpi.engineGetKeySpec implementations. In most cases, the requested KeySpec is equal to the concrete implementation so the inversion does not matter. But there are few cases, as presented in the added jtreg test, will cause unexpected behavior (e.g., ClassCastException rather than an InvalidKeySpecException). The fix is trivial.
>>
>> Co-author @SalusaSecondus
>
> Ziyi Luo has updated the pull request incrementally with one additional commit since the last revision:
>
>   8254717: isAssignableFrom checks in KeyFactorySpi.engineGetKeySpec appear to be backwards (r2)

Thanks for fixing those two tests and the nits.  Looks good.

-------------

Marked as reviewed by jnimeh (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2682