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

[Joe Darcy]

Hi Sean,

On 9/23/2019 12:19 PM, Sean Mullan wrote:
> Hi Joe,
>
> It's a little odd to suppress the warnings in the X509CertImpl class 
> since it is a subclass of java.security.cert.Certificate which 
> implements the writeReplace method so these fields are not serialized.
>
> Also for other classes like X509Key which are internal it is a little 
> odd to suppress the warnings for fields like bitStringKey that are not 
> Serializable and are never serialized. It is probably better to mark 
> them as transient, but I'm not really sure it is worth making those 
> changes for otherwise stable code. I guess when I look at some of the 
> warnings, I might think there is an issue when there really isn't.
>
> I suppose these are not things you can easily detect at compile time, 
> but I am wondering what you think.
>
>
Thanks for the feedback. One motivation to send out these review is to 
help tune-up the warning analysis.

A brief philosophical note, when designing any sort of warning scheme 
the cost of false positives vs false negatives needs to be weighed as 
any static check can likely only approximate the full range of dynamic 
behavior. The checks as written give up, i.e. do not attempt to analyze 
and warn, if the class uses serialPersistentFields. It would be possible 
for the checks to similarly decline to analyze if any of the various 
write* or read* serialization methods are defined. My current impression 
is that the net benefit for the checks when write* methods are defined 
is still worthwhile, even though there are some false positives.

As you observe, given the handling of the serial methods, bitStringKey 
in X509Key is effectively transient. I think it would be better serial 
coding for the field to be explicitly marked as transient to make it 
more obviously consistent with the implementation. Moreover, all the 
instance fields in X509Key except encodedKey look to be effectively 
transient. Since the class already has a serialVersionUID, marking the 
fields as transient is a serial-compatible change.

Cheers,

-Joe