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

[Joe Darcy]

Hi Sean,

On 9/26/2019 12:46 PM, Sean Mullan wrote:
> 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"?

Yes; applying SuppressWarnings("serial") to the class will silence this 
new warning for all the fields. I didn't take that approach in the 
version I sent to review since, by default, I wanted to provide the 
narrowest scope of the suppression.

Would you prefer I revise the patch where there are multiple 
SuppressWarnings("serial") on fields to put a single one on the class 
instead?

Cheers,

-Joe