jdk
jdk copied to clipboard
openjdk
8333796: Add missing serialization functionality to sun.reflect.ReflectionFactory
Created in DRAFT mode to get early feedback/reviews.
cc: @AlanBateman
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [ ] Change requires CSR request JDK-8335438 to be approved
- [x] Commit message must refer to an issue
Issues
- JDK-8333796: Add missing serialization functionality to sun.reflect.ReflectionFactory (Enhancement - P4)
- JDK-8335438: Add missing serialization functionality to sun.reflect.ReflectionFactory (CSR)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19702/head:pull/19702
$ git checkout pull/19702
Update a local copy of the PR:
$ git checkout pull/19702
$ git pull https://git.openjdk.org/jdk.git pull/19702/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19702
View PR using the GUI difftool:
$ git pr show -t 19702
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19702.diff
:wave: Welcome back dmlloyd! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@dmlloyd This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8333796: Add missing serialization functionality to sun.reflect.ReflectionFactory
Reviewed-by: liach, rriggs
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 138 new commits pushed to the master branch:
- db7ee3dad1d9c9578794d946dd5de1f51d77e5a1: 8344223: Remove calls to SecurityManager and doPrivileged in java.net.URLClassLoader after JEP 486 integration
- 6f4dfa66268c7aef0298af7f18d8e8bd4eb21656: 8344190: Cleanup code in sun.net.www.protocol.http and sun.net.www.protocol.https after JEP 486 integration
- 7bb4474d81a55028de5434f445747c56a8dc333c: 8344579: Clean up forward declarations and includes
- 21b8749bfdede7dfee3e8433dd9443320db99076: 8344479: Declare MetaspaceObj::operator delete to be deleted
- ea7e722ca04752f0b58bf98e0a1907c015644fb5: 8344010: RISC-V: Zacas do not work with LW locking
- e2f8f1aded319034a79fe78af7e011e83df75d62: 8344621: ProblemList runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
- afee7405bd13cbe1cb829dd150a9de7e6faf49ae: 8343541: C1: Plain memory accesses are emitted with membars with +AlwaysAtomicAccesses
- 3a4a9b7af7693a836c3caa3112d0d68100535b28: 8340145: Problem with generic pattern matching results in internal compiler error
- cf158bc6cdadfdfa944b8ec1d3dc7069c8f055a9: 8341631: JShell should auto-import java.io.IO.*
- 5b12a87dcb47b5783f179534e2de43d5a920a489: 8344060: Remove doPrivileged calls from shared implementation code in the java.desktop module : part 1
- ... and 128 more: https://git.openjdk.org/jdk/compare/752e1629555f0ec8630373ec87b049afdd709ea6...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@liach, @RogerRiggs) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@dmlloyd
The label @AlanBateman is not a valid label.
These labels are valid:
graalserviceabilityhotspothotspot-compileride-supportkullai18nshenandoahjdkjavadocsecurityhotspot-runtimejmxbuildnioclientcore-libscompilernethotspot-gchotspot-jfr
@dmlloyd The following label will be automatically applied to this pull request:
core-libs
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.
I've updated the PR with a new access control solution, but it's not ideal since it involves internal reflection. To solve this issue, unless I'm misunderstanding something, I think we would need to have access to Lookup#IMPL_LOOKUP from the method generators. The only way I can think to do this (without exposing the lookup from JavaLangInvokeAccess, which I assume would not be desirable) would be to move the generators into the java.lang.invoke package, adding methods to JavaLangInvokeAccess for that purpose. Would this be the recommended approach, or is there some other better approach that I do not see?
I added a commit which moves the bytecode gen to the JLI package and accesses it via SharedSecrets, which is reasonably similar to what other methods in ReflectionFactory are already doing.
Edit: Per Alan's feedback this was moved back again. See the resolved discussion for details.
I skimmed through the latest update (d137f43) and I think you've got this to a good place and a sensible/workable proposal. I've asked from help from others on the security review of this.
Right now, I'm still wonder if defaultReadObjectForSerialization/defaultWriteObjectForSerialization should return the same as readObjectForSerialization/writeObjectForSerialization when the readObject/writeObject methods are defined. This is more of a concern for a class with a readObject of course as that readObject will likely check invariants that would be bypassed if the serialization library always uses the defaultXXX methods.
I'd probably drop the "Of" suffix from serialPersistentFieldsOf and serialVersionUIDOf but naming isn't important right now.
I skimmed through the latest update (d137f43) and I think you've got this to a good place and a sensible/workable proposal. I've asked from help from others on the security review of this.
Great. There are a couple more tests I want to add but once that's done I'll mark it "Ready for review".
Right now, I'm still wonder if defaultReadObjectForSerialization/defaultWriteObjectForSerialization should return the same as readObjectForSerialization/writeObjectForSerialization when the readObject/writeObject methods are defined. This is more of a concern for a class with a readObject of course as that readObject will likely check invariants that would be bypassed if the serialization library always uses the defaultXXX methods.
By spec, the readObject method must call either OIS#defaultReadObject or OIS#readFields, but they are allowed to choose which one they call. If they call defaultReadObject, then the serialization library would still need defaultReadObjectForSerialization in order to implement it. If defaultReadObjectForSerialization falls back to calling the user readObject, we then would (at best) have a stack overflow situation, because the user's readObject would call defaultReadObject, which in turn would call back into the user's readObject, etc.
So, in some cases, the (de)serializer can use only readObjectForSerialization (specifically, when there is a user readObject method which uses readFields); in other cases it can use only defaultReadObjectForSerialization (when the user didn't provide a readObject); and, in the final case, it would need to use both (when the user readObject calls OIS#defaultReadObject).
The same thing applies on the write side.
I'd probably drop the "Of" suffix from serialPersistentFieldsOf and serialVersionUIDOf but naming isn't important right now.
I'll do so.
/csr
Since this is an API addition, this will require a CSR; Valhalla folks can check if this API is future-proof in the CSR.
/csr
Since this is an API addition, this will require a CSR; Valhalla folks can check if this API is future-proof in the CSR.
According to https://wiki.openjdk.org/display/csr/CSR+FAQs, only changes to public and exported APIs in the jdk.*, java.*, and javax.* packages need a CSR. This PR changes public and exported APIs in the sun.misc package, and changes public but non-exported APIs in the jdk.internal.reflect package, so I don't think it qualifies, does it?
According to https://wiki.openjdk.org/display/csr/CSR+FAQs, only changes to public and exported APIs in the
jdk.*,java.*, andjavax.*packages need a CSR. This PR changes public and exported APIs in thesun.miscpackage, and changes public but non-exported APIs in thejdk.internal.reflectpackage, so I don't think it qualifies, does it?
CSR is required for changes to any public and exported API, including in sun.misc package, and including trivial signature changes (such as making a method default in an interface, or marking a utility class final). The presence of a CSR doesn't necessary mean a patch will be harder to integrate; it's often for archival and quality-ensurance purposes. Here's an example CSR for changes to APIs in sun.misc: https://bugs.openjdk.org/browse/JDK-8331686 (#19174)
@dmlloyd has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@dmlloyd please create a CSR request for issue JDK-8333796 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
Webrevs
- 10: Full (8b83bd70)
- 09: Full (7a85d620)
- 08: Full (d57c4346)
- 07: Full - Incremental (997dba08)
- 06: Full - Incremental (11bc95e8)
- 05: Full - Incremental (38f7b79a)
- 04: Full - Incremental (34d45238)
- 03: Full - Incremental (bf658a39)
- 02: Full - Incremental (2a734c5e)
- 01: Full - Incremental (1d8f1b22)
- 00: Full (e7334655)
Is there a high level description of how these methods would be used? There seems to be a lot of generation of code that duplicates what OIS/OOS already do and that seems unnecessary.
Is there a high level description of how these methods would be used?
They would be used identically to how the readObjectForSerialization and writeObjectForSerialization methods are used. The caller would pass the object as a pseudo-"receiver" and the OIS/OOS, and it would be as if the caller had called a method handle to a readObject or writeObject that writes or reads the serializable fields of the object based on acquiring a GetField/PutField object.
There seems to be a lot of generation of code that duplicates what OIS/OOS already do and that seems unnecessary.
The current iteration simulates the behavior described above by generating methods that exactly and literally implements the behavior as specified. That said, I agree that there could possibly be a way to reuse OIS/OOS code for this instead, so that the method works as if it had been generated (but instead would be a stub which calls the relevant OIS/OOS method(s) with the appropriate arguments). It might require some small refactoring. My initial thought was to avoid touching that code as much as possible, but I can look into reusing the existing Unsafe-based code more and see what that would look like.
The tests should include serializing some sample JDK classes that include a hierarchy of 2-4 levels. It would help fles any interactions in the sequencing of calling the default serialization for each of a concrete class's supertypes.
The tests should include serializing some sample JDK classes that include a hierarchy of 2-4 levels. It would help fles any interactions in the sequencing of calling the default serialization for each of a concrete class's supertypes.
I'm having some difficulty with this, first of all finding a good candidate JDK class which would be accessible from the test which has multiple levels of serializable hierarchy. Secondly AFAICT this change doesn't relate in any way to sub/superclass calling sequencing (that would be the caller's responsibility). The API deals strictly with a single level of a class hierarchy at a time. So, I'm not quite clear on what else to check; the tests I have presently will verify that the method meets its contract, but if there's some subtlety I missed (very possible) then I'd like to add tests for those case(s).
@RogerRiggs The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).
Could anyone provide a second review? I'm hoping to get this finished before rampdown for 24, which is about 8 weeks away now, and there are still a few steps to go yet.
Thanks!
@dmlloyd Please update the CSR to match the PR and mark it proposed to get the review started.
I think I'm just needing two reviewers now (and a sponsor after that). It's close!
The failure on Mac is some kind of tooling setup error:
xcode-select: error: invalid developer directory '/Applications/Xcode_14.3.1.app/Contents/Developer'
With the SecurityManager (jep 486) integrated, cleanup of referernces to SecurityManager etc will need to be cleanedup. It can be done as a separate PR or included in this PR.