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

[Sean Mullan]

On 9/23/19 4:16 PM, Joe Darcy wrote:
> 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.

Ok. Well I understand that the benefit here is to try to detect 
serialization issues, and that is great. But at least in some cases like 
in security libs where classes are doing their own alternate 
serialization and have more than a few fields, the workaround of leaving 
the warnings in or having to mark all the fields as transient - either 
approach is not ideal to me. Could the SuppressWarnings("serial") 
annotation be applied at the class level so that it basically said "I 
know what I'm doing here, don't warn me about serialization issues"?

--Sean