RFR: 8275338: Add JFR events for notable serialization situations [v10]

Roger Riggs rriggs at openjdk.org
Thu Dec 21 21:49:55 UTC 2023


On Thu, 21 Dec 2023 09:53:10 GMT, Raffaello Giulietti <rgiulietti at openjdk.org> wrote:

>> Adds serialization misdeclaration events to JFR.
>
> Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Corrected @Label of event and of field.

src/java.base/share/classes/java/io/ObjectStreamClass.java line 466:

> 464: 
> 465:         if (SerializationMisdeclarationEvent.enabled() && serializable) {
> 466:             new SerializationMisdeclarationChecker(cl).checkMisdeclarations();

Is there any benefit to avoiding the allocation and passing the class through the methods as an extra arg?

src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 149:

> 147:         if (!(spf instanceof ObjectStreamField[])) {
> 148:             commitEvent(SERIAL_PERSISTENT_FIELDS_NAME +
> 149:                     " must be an instance of ObjectStreamField[] to be effective");

Hmmm...  

The terminology "to be effective" seems a bit indirect and may lead to some head scratching.
In most cases it means it is ignored.
I might suggest to just state the condition that is required and drop the extra phrase.
For example,  "xxx should be an instance of ObjectStreamField[]".
It would result in shorter messages and if further explanation of the message is needed it can more completely elaborate on the impact of the incorrect declaration.

src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 165:

> 163:             commitEvent("method " + m + " on an enum class is not effective");
> 164:         } else if (cl.isRecord()) {
> 165:             commitEvent("method " + m + " on an record class is not effective");

"an record" -> "a record"

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434524764
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434512690
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1434513714



More information about the security-dev mailing list