[jdk17u-dev] RFR: 8302111: Serialization considerations [v2]

[Richard Reingruber]

On Thu, 20 Feb 2025 08:59:41 GMT, Goetz Lindenmaier <goetz at openjdk.org> wrote:

>> I backport this to match 17.0.13-oracle based on the commit to 21.
>> 
>> I had to resolve several files, two of them considerably:
>> 
>> src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java
>>   Resolved larger chunk.
>> src/java.base/share/classes/com/sun/crypto/provider/DHPublicKey.java
>>   Resolved some code.
>> 
>> The new code in DHPublic/PrivateKey depends on the removal
>> of IOExceptions in [JDK-8297065](https://bugs.openjdk.org/browse/JDK-8297065) "DerOutputStream operations should not throw IOExceptions". 
>> This change is in 21 but not in 17. Thus code backported from 21 calls functions of 
>> DerOutputStream that do not throw an exception in 21, but do so in 17. 
>> JDK-8297065 makes the point that these exceptions can never be thrown, which also
>> holds for 17.  So I just catch and ignore them.
>> 
>> Also, I had to adapt code because an exception constructor with cause for
>> InvalidObjectException is missing in 17.
>> This was added by [JDK-8282696](https://bugs.openjdk.org/browse/JDK-8282696) "Add constructors taking a cause
>> to InvalidObjectException and InvalidClassException" in 19.
>> I added a call to initCause() to store the causing exception.
>> 
>> I double-checked that [JDK-8297065](https://bugs.openjdk.org/browse/JDK-8297065) was not backported
>> by Oracle. Backporting this change would simplify matters considerably.
>> 
>> The remaining files only needed trivial resolves:
>> 
>> src/java.base/share/classes/java/security/Permissions.java
>> src/java.base/share/classes/java/security/SignedObject.java
>>   Only Copyright.
>> src/java.base/share/classes/java/security/Timestamp.java
>>   Copyright and import.
>> src/java.base/share/classes/java/security/UnresolvedPermissionCollection.java
>> src/java.base/share/classes/java/security/cert/CertificateRevokedException.java
>> src/java.base/share/classes/sun/security/provider/DRBG.java
>> src/java.base/share/classes/sun/security/util/ObjectIdentifier.java
>> src/java.base/share/classes/sun/security/x509/AlgIdDSA.java
>>   Only Copyright.
>> src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5Context.java
>>   Resolved imports.
>> src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5InitCredential.java
>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11SecureRandom.java
>>   Only Copyright.
>> 
>> I manually ran these tests on linux x86_64:
>> 
>> test/jdk/com/sun/security
>> test/jdk/java/awt/security
>> test/jdk/java/net/httpclient/security
>> test/jdk/java/net/httpclient/we...
>
> Goetz Lindenmaier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove mentioning of ProviderException

src/java.base/share/classes/com/sun/crypto/provider/DHPrivateKey.java line 379:

> 377:         } catch (IOException e) {
> 378:             InvalidObjectException ioe = new InvalidObjectException("Invalid encoding");
> 379:             if (ioe != null) {

Isn't this condition always true?
Note also that the throw below will throw a NPE if it `ioe` was null.
I think you can actually remove the condition.

src/java.base/share/classes/com/sun/crypto/provider/DHPublicKey.java line 188:

> 186:             derKey = null;
> 187:         }
> 188:         return derKey.toByteArray();

This looks weired if someone has a quick look at these lines of code: if an IOE occurs and is caught here, then `derKey` will be assigned null and 2 lines below an NPE will thrown because of this.

I'd suggest to wrap the whole method body in a try-catch. This would reduce the diff to jdk 21.
In the IOE catch clause you shouldn't say that the IOE is ignored but you should state that it cannot even occur since `DerOutputStream` is a `ByteArrayOutputStream` which doesn't do any I/O. Then just return null or throw an `InternalError` are something else if more appropriate.

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

PR Review Comment: https://git.openjdk.org/jdk17u-dev/pull/3278#discussion_r1963035399
PR Review Comment: https://git.openjdk.org/jdk17u-dev/pull/3278#discussion_r1963829813