On 10/8/19 9:56 PM, Joe Darcy wrote:
Hi Sean,
Getting back to this review...
On 9/26/2019 1:55 PM, Sean Mullan wrote:
On 9/26/19 4:20 PM, Sean Mullan wrote:
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)
Ok, I double-checked everything. The only other class in the webrev
that uses an alternate serial form is:
sun/security/x509/X509Key.java
but since that only has one field that is not Serializable, it
probably is ok to leave as-is.
I've put the updated webrev at:
http://cr.openjdk.java.net/~darcy/8231262.1/
The difference from the first webrev is I marked a field in X509Key.java
as transient; diff of webrev patches:
> --- old/src/java.base/share/classes/sun/security/x509/X509Key.java
2019-10-08 18:37:02.143999531 -0700
> +++ new/src/java.base/share/classes/sun/security/x509/X509Key.java
2019-10-08 18:37:01.815999531 -0700
> @@ -84,7 +84,7 @@
214,215c214,215
< + @SuppressWarnings("serial") // Not statically typed as Serializable
< private BitArray bitStringKey = null;
---
> - private BitArray bitStringKey = null;
> + private transient BitArray bitStringKey = null;
For src/java.base/share/classes/sun/security/x509/X509CertImpl.java, I
don't see any of the read* or write* methods associated with
serialization defined in the class. I would seem clearer to me to having
the @SuppressWarning("serial") at each field, but if you'd prefer to
have it at the class level, I'll defer to your judgement on the matter.
X509CertImpl extends X509Certificate which extends Certificate.
Certificate has a writeReplace method.
I now think that a better fix for classes that use a custom serial
format is to mark all the instance fields that are not part of that
custom format as transient. This includes the fields that are primitive
types as well as other types (and whether or not they actually implement
Serializable or not). But maybe we need to document a best practice for
cases like this. JB does provide the following advice in Item 87 of Eff
Java (p. 351 of 3rd edition): "If you use a custom serialized form, most
or all of the instance fields should be labeled transient, ..."
In the case of X509Key, that would mean marking the algid, key,
unusedBits and bitStringKey fields transient, and for X509CertImpl I
suppose this would mean all of the instance fields.
If this is more than you bargained for when fixing this :), feel free to
file a follow-on issue and assign it to me.
Thanks,
Sean
FYI, of the files being modified only
src/java.base/share/classes/javax/security/auth/Subject.java uses
readFields or putFields.
Thanks,
-Joe
