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
