On Mon, 8 Jan 2024 18:38:52 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Raffaello Giulietti has updated the pull request with a new target base due >> to a merge or a rebase. The incremental webrev excludes the unrelated >> changes brought in by the merge/rebase. The pull request contains 15 >> additional commits since the last revision: >> >> - Merge branch 'master' into 8275338 >> - Simplified event messages. >> Remove ckecker allocation. >> - Corrected @Label of event and of field. >> - Removed @module from test. >> - Merge branch 'master' into 8275338 >> - Renamed an event field. >> - Minor changes. >> - Removed event kind. >> serialVersionUID must have type long. >> Test now base on keyword search in event message. >> Commented test classes about misdeclarations. >> - Changes according to reviewer's comments. >> - Better name for a label, corrected name of removed field. >> - ... and 5 more: https://git.openjdk.org/jdk/compare/09cef802...9ca1f36d > > src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java > line 70: > >> 68: privilegedCheckAccessibleMethod(cl, WRITE_REPLACE_NAME, >> 69: WRITE_REPLACE_PARAM_TYPES, Object.class); >> 70: privilegedCheckAccessibleMethod(cl, READ_RESOLVE_NAME, > > Thinking ahead to when the security manager is removed, can the code that > needs private access to field values (SUID) be more narrowly accessed? > Perhaps switch to using a VarHandle and MethodHandles.Lookup. This may be a > longer term issue and beyond the scope of this PR. > > In the naming of the `PrivilegedXXX` methods, I would drop the "privileged" > part of the name. > The methods do not change the privilege level and operate correctly if when > the caller has the privileges needed. And all of these methods are read-only > so there is no/little risk in their implementations and avoid refitting the > terminology later. They are called `privilegedXXX` because they _are_ (already) privileged, not because they change the privileges. But yes, in view of removing the security manager, the implementation could be more "modern". Will have a look. > test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 33: > >> 31: import org.junit.jupiter.params.provider.MethodSource; >> 32: >> 33: import java.io.*; > > Explicit imports are preferred. Oops, this is the (overridable) IDE default choice ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445928358 PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1445930028
