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

[Sean Mullan]

On 9/26/19 4:10 PM, Joe Darcy wrote:
> 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?

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)

--Sean