JDK 14 RFR of JDK-8231262: Suppress warnings on non-serializable instance fields in security libs serializable classes

[Sean Mullan]

On 10/8/19 9:56 PM, Joe Darcy wrote:
> Hi Sean,
> 
> Getting back to this review...
> 
> On 9/26/2019 1:55 PM, Sean Mullan wrote:
>> On 9/26/19 4:20 PM, Sean Mullan wrote:
>>>> Would you prefer I revise the patch where there are multiple 
>>>> SuppressWarnings("serial") on fields to put a single one on the 
>>>> class instead?
>>>
>>> Yes, but only in the cases where we are clearly using some form of 
>>> alternate serialization like ASN.1 encoding. I need to double-check 
>>> the review again (it's a bit more time consuming because I have to 
>>> look at the code in more detail), but the two that I spotted so far are:
>>>
>>> src/java.base/share/classes/sun/security/x509/X509CertImpl.java
>>> src/java.security.jgss/share/classes/sun/security/krb5/internal/KRBError.java 
>>> (from the JDK-8231368 review)
>>
>> Ok, I double-checked everything. The only other class in the webrev 
>> that uses an alternate serial form is:
>>
>> sun/security/x509/X509Key.java
>>
>> but since that only has one field that is not Serializable, it 
>> probably is ok to leave as-is.
>>
>>
> I've put the updated webrev at:
> 
>      http://cr.openjdk.java.net/~darcy/8231262.1/
> 
> The difference from the first webrev is I marked a field in X509Key.java 
> as transient; diff of webrev patches:
> 
>  > --- old/src/java.base/share/classes/sun/security/x509/X509Key.java 
> 2019-10-08 18:37:02.143999531 -0700
>  > +++ new/src/java.base/share/classes/sun/security/x509/X509Key.java 
> 2019-10-08 18:37:01.815999531 -0700
>  > @@ -84,7 +84,7 @@
> 214,215c214,215
> < +    @SuppressWarnings("serial") // Not statically typed as Serializable
> <      private BitArray bitStringKey = null;
> ---
>  > -    private BitArray bitStringKey = null;
>  > +    private transient BitArray bitStringKey = null;
> 
> For src/java.base/share/classes/sun/security/x509/X509CertImpl.java, I 
> don't see any of the read* or write* methods associated with 
> serialization defined in the class. I would seem clearer to me to having 
> the @SuppressWarning("serial") at each field, but if you'd prefer to 
> have it at the class level, I'll defer to your judgement on the matter.

X509CertImpl extends X509Certificate which extends Certificate. 
Certificate has a writeReplace method.

I now think that a better fix for classes that use a custom serial 
format is to mark all the instance fields that are not part of that 
custom format as transient. This includes the fields that are primitive 
types as well as other types (and whether or not they actually implement 
Serializable or not). But maybe we need to document a best practice for 
cases like this. JB does provide the following advice in Item 87 of Eff 
Java (p. 351 of 3rd edition): "If you use a custom serialized form, most 
or all of the instance fields should be labeled transient, ..."

In the case of X509Key, that would mean marking the algid, key, 
unusedBits and bitStringKey fields transient, and for X509CertImpl I 
suppose this would mean all of the instance fields.

If this is more than you bargained for when fixing this :), feel free to 
file a follow-on issue and assign it to me.

Thanks,
Sean


> 
> FYI, of the files being modified only 
> src/java.base/share/classes/javax/security/auth/Subject.java uses 
> readFields or putFields.
> 
> Thanks,
> 
> -Joe
>