Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8242888: Convert dynamic proxy to hidden classes #8278

Closed
wants to merge 4 commits into from

Conversation

liach
Copy link
Member

@liach liach commented Apr 17, 2022

Convert dynamic proxies to hidden classes. Modifies the serialization of proxies (requires change in "Java Object Serialization Specification"). Makes the proxies hidden in stack traces. Removes duplicate logic in proxy building.

The main compatibility changes and their rationales are:

  1. Modification to the serialization specification: In the "An instance of the class is allocated... The contents restored appropriately" section, I propose explicitly state that handling of proxies are unspecified as to allow implementation freedom, though I've seen deliberate attempts for proxies to implement interfaces with readResolve in order to control their serialization behavior.
    • This is for the existing generated constructor accessor is bytecode-based, which cannot refer to hidden classes.
    • An alternative is to preserve the behavior, where the serialization constructor calls invokespecial on the closest serializable superclass' no-arg constructor, like in JDK-8242888: Convert dynamic proxy to hidden classes #1830 by @DasBrain.
    • My rationale against preservation is such super calls are unsafe and should be discouraged in the long term. Calling the existing constructor with a dummy argument, as in my implementation, would be more safe.
  2. The disappearance of proxies in stack traces.
    • Same behavior exists in lambda expressions: For instance, in ((Runnable) () -> { throw new Error(); }).run();, the run method, implemented by the lambda, will not appear in the stack trace, and isn't too problematic.

A summary of the rest of the changes:

  1. Merged the two passes of determining module and package of the proxy into one. This reduced a lot of code and allowed anchor class (for hidden class creation) selection be done together as well.
  2. Exposed internal API for obtaining a full-privileged lookup to the rest of java.base. This API is intended for implementation of legacy (pre MethodHandles.Lookup) caller sensitive public APIs so they don't need more complex tricks to obtain proper permissions as lookups.
  3. Implements 8229959: passes methods computed by proxy generator as class data to the hidden proxy class to reduce generated proxy class size and improve performance.

In addition, since this change is somewhat large, should we keep the old proxy generator as well and have it toggled through a command-line flag (like the old v49 proxy generator or the old reflection implementation)?

Please feel free to comment or review. This change definitely requires a CSR, but I have yet to determine what specifications should be changed.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issue

Contributors

  • Johannes Kuhn <jkuhn@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8278/head:pull/8278
$ git checkout pull/8278

Update a local copy of the PR:
$ git checkout pull/8278
$ git pull https://git.openjdk.java.net/jdk pull/8278/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8278

View PR using the GUI difftool:
$ git pr show -t 8278

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8278.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 17, 2022

👋 Welcome back liach! 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.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 17, 2022
@openjdk
Copy link

openjdk bot commented Apr 17, 2022

@liach The following labels will be automatically applied to this pull request:

  • core-libs
  • kulla
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org kulla kulla-dev@openjdk.org labels Apr 17, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 17, 2022

Webrevs

@liach
Copy link
Member Author

liach commented Apr 17, 2022

/csr needed
/contributor add @DasBrain
/label remove kulla

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Apr 17, 2022
@openjdk
Copy link

openjdk bot commented Apr 17, 2022

@liach has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@liach please create a CSR request for issue JDK-8242888. This pull request cannot be integrated until the CSR request is approved.

@openjdk
Copy link

openjdk bot commented Apr 17, 2022

@liach
Contributor Johannes Kuhn <jkuhn@openjdk.org> successfully added.

@openjdk openjdk bot removed the kulla kulla-dev@openjdk.org label Apr 17, 2022
@openjdk
Copy link

openjdk bot commented Apr 17, 2022

@liach
The kulla label was successfully removed.

@mlbridge
Copy link

mlbridge bot commented Apr 17, 2022

Mailing list message from Peter Firmstone on security-dev:

We re-implemented a subset of Java Serialization, prior to the creation
Java serialization filters.? Field types are read ahead from the stream
and invariant's validated during construction for failure atomicity (a
special constructor signature is used for deserialization), there are
also stream limits that require periodical stream resets, to avoid
million laugh style attacks. Also the source of the serialized data must
be authenticated before permission is granted for parsing serial data.??
We also removed the ability to deserialize object graphs with circular
links.

This is used for an Remove invocation framework called JERI (Jini
extensible remote invocation).? Serialization has been given a public
API to allow extensibility, that is to allow other serialization
protocols to be used via configuration, using a Serialization layer.

Service and service discovery architecture makes use of JERI, for
marshaling object parameters securely over the network, for remote
method calls on services, via remote proxy's.

The way that JERI serializes proxy's has changed significantly since
Jini, instead of marshaling proxy's, a bootstrap proxy (local code only)
is marshaled first, the client first authenticates the connection, the
bootstrap proxy is used to provide information for dynamic downloading
of any required jar files, or wiring of dependencies prior to the
unmarshaling of the service proxy.? Unlike Java RMI, which uses
RMIClassLoading for class resolution, each Jeri Endpoint is assigned a
ClassLoader for class resolution during deserialization / object
unmarshaling, this solves the OSGi deserialzation class resolution problem.

https://github.com/pfirmstone/JGDMS/blob/trunk/JGDMS/jgdms-platform/src/main/java/org/apache/river/api/io/ProxySerializer.java

https://github.com/pfirmstone/JGDMS/blob/c1edf5892306f24f8f97f459f499dec54984b08f/JGDMS/jgdms-platform/src/main/java/org/apache/river/api/io/AtomicMarshalInputStream.java#L2263

https://github.com/pfirmstone/JGDMS/blob/c1edf5892306f24f8f97f459f499dec54984b08f/JGDMS/jgdms-platform/src/main/java/org/apache/river/api/io/AtomicMarshalInputStream.java#L853

Would be nice to do some testing with the changes.

--
Regards,

Peter Firmstone
0498 286 363
Zeus Project Services Pty Ltd.

On 18/04/2022 2:24 am, liach wrote:

@liach
Copy link
Member Author

liach commented Apr 18, 2022

I now think that this should be split into probably four separate patches:

  1. Merge the 2 passes of scanning interfaces, and store the exported/non-exported package within the classloader value info for dynamic modules
  2. Use the JLIA access to obtain lookup than spin an accessor in every proxy class
  3. Update serialization constructor generation to use method handles (as in JDK-8242888: Convert dynamic proxy to hidden classes #1830) for hidden classes, don't change the serialization specs for now, as it affects security.

And finally, convert proxies to hidden classes like in this pr. There will be an option (most likely command line) to restore old proxies. And in the new proxies, method objects can be passed as class data, like in the current patch. The jshell test change will be in the final conversion patch.

@mlbridge
Copy link

mlbridge bot commented Apr 18, 2022

Mailing list message from Alan Bateman on core-libs-dev:

On 17/04/2022 17:24, liach wrote:

Convert dynamic proxies to hidden classes. Modifies the serialization of proxies (requires change in "Java Object Serialization Specification"). Makes the proxies hidden in stack traces. Removes duplicate logic in proxy building.

The main compatibility changes and their rationales are:
1. Modification to the serialization specification: In the "An instance of the class is allocated... The contents restored appropriately" section, I propose explicitly state that handling of proxies are unspecified as to allow implementation freedom, though I've seen deliberate attempts for proxies to implement interfaces with `readResolve` in order to control their serialization behavior.
- This is for the existing generated constructor accessor is bytecode-based, which cannot refer to hidden classes.
- An alternative is to preserve the behavior, where the serialization constructor calls `invokespecial` on the closest serializable superclass' no-arg constructor, like in #1830 by @DasBrain.
- My rationale against preservation is such super calls are unsafe and should be discouraged in the long term. Calling the existing constructor with a dummy argument, as in my implementation, would be more safe.
2. The disappearance of proxies in stack traces.
- Same behavior exists in lambda expressions: For instance, in `((Runnable) () -> { throw new Error(); }).run();`, the `run` method, implemented by the lambda, will not appear in the stack trace, and isn't too problematic.

It's great that you have time to experiment in this area but just to set
expectations that it will likely require significant discussion and
maybe prototyping of alternatives. It suspect the Reviewer effort will
end up being many times the effort required to do the actual work
because of the compatibility and security issues that will need to be
worked through.

I think you need to add non-discoverability to the list of compatibility
issues. Do we know for sure that there aren't frameworks and libraries
that use Class.forName on proxy classes? We've had issues in the past
with changes in this area.

It's too early to say but it might be that the compatibility risks may
nudge this one into creating a new API.

-Alan.

@mlbridge
Copy link

mlbridge bot commented Apr 18, 2022

Mailing list message from Johannes Kuhn on core-libs-dev:

On 18-Apr-22 9:36, Alan Bateman wrote:

On 17/04/2022 17:24, liach wrote:

Convert dynamic proxies to hidden classes. Modifies the serialization
of proxies (requires change in "Java Object Serialization
Specification"). Makes the proxies hidden in stack traces. Removes
duplicate logic in proxy building.

The main compatibility changes and their rationales are:
1. Modification to the serialization specification: In the "An
instance of the class is allocated... The contents restored
appropriately" section, I propose explicitly state that handling of
proxies are unspecified as to allow implementation freedom, though
I've seen deliberate attempts for proxies to implement interfaces with
`readResolve` in order to control their serialization behavior.
??? - This is for the existing generated constructor accessor is
bytecode-based, which cannot refer to hidden classes.
??? - An alternative is to preserve the behavior, where the
serialization constructor calls `invokespecial` on the closest
serializable superclass' no-arg constructor, like in #1830 by @DasBrain.
??? - My rationale against preservation is such super calls are unsafe
and should be discouraged in the long term. Calling the existing
constructor with a dummy argument, as in my implementation, would be
more safe.
2. The disappearance of proxies in stack traces.
??? - Same behavior exists in lambda expressions: For instance, in
`((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
implemented by the lambda, will not appear in the stack trace, and
isn't too problematic.

It's great that you have time to experiment in this area but just to set
expectations that it will likely require significant discussion and
maybe prototyping of alternatives. It suspect the Reviewer effort will
end up being many times the effort required to do the actual work
because of the compatibility and security issues that will need to be
worked through.

I think you need to add non-discoverability to the list of compatibility
issues. Do we know for sure that there aren't frameworks and libraries
that use Class.forName on proxy classes? We've had issues in the past
with changes in this area.

It's too early to say but it might be that the compatibility risks may
nudge this one into creating a new API.

-Alan.

Proxies will have to be rethought at some future point - wrt Valhalla.

The current specification says:

A proxy class implements exactly the interfaces specified at its
creation, in the same order. Invoking getInterfaces on its Class object
will return an array containing the same list of interfaces (in the
order specified at its creation), [...]

In the current design Valhalla will add two interfaces - IdentityObject
and ValueObject (afaik). One of them have to be implemented as well.
Also, because the superclass is java.lang.reflect.Proxy, and that class
has a protected final field, it can currently not implement ValueObject.

An other thing are annotations - currently they are implemented using
Proxies. This implementation detail surfaces when serializing an
annotation. Other implementation strategies are possible - for example
spinning a record at runtime.
But this leads to the question - how can one migrate away from
serialized proxies in a compatible way?

In the end - a lot of stuff in the JDK depends on Proxies, and their
limitations now begins to surface.

A new API may not be a bad idea. :)

- Johannes

@mlbridge
Copy link

mlbridge bot commented Apr 18, 2022

Mailing list message from Remi Forax on core-libs-dev:

----- Original Message -----

From: "Johannes Kuhn" <info at j-kuhn.de>
To: "Alan Bateman" <Alan.Bateman at oracle.com>, "core-libs-dev" <core-libs-dev at openjdk.java.net>
Sent: Monday, April 18, 2022 12:53:45 PM
Subject: Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

On 18-Apr-22 9:36, Alan Bateman wrote:

On 17/04/2022 17:24, liach wrote:

Convert dynamic proxies to hidden classes. Modifies the serialization
of proxies (requires change in "Java Object Serialization
Specification"). Makes the proxies hidden in stack traces. Removes
duplicate logic in proxy building.

The main compatibility changes and their rationales are:
1. Modification to the serialization specification: In the "An
instance of the class is allocated... The contents restored
appropriately" section, I propose explicitly state that handling of
proxies are unspecified as to allow implementation freedom, though
I've seen deliberate attempts for proxies to implement interfaces with
`readResolve` in order to control their serialization behavior.
??? - This is for the existing generated constructor accessor is
bytecode-based, which cannot refer to hidden classes.
??? - An alternative is to preserve the behavior, where the
serialization constructor calls `invokespecial` on the closest
serializable superclass' no-arg constructor, like in #1830 by @DasBrain.
??? - My rationale against preservation is such super calls are unsafe
and should be discouraged in the long term. Calling the existing
constructor with a dummy argument, as in my implementation, would be
more safe.
2. The disappearance of proxies in stack traces.
??? - Same behavior exists in lambda expressions: For instance, in
`((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
implemented by the lambda, will not appear in the stack trace, and
isn't too problematic.

It's great that you have time to experiment in this area but just to set
expectations that it will likely require significant discussion and
maybe prototyping of alternatives. It suspect the Reviewer effort will
end up being many times the effort required to do the actual work
because of the compatibility and security issues that will need to be
worked through.

I think you need to add non-discoverability to the list of compatibility
issues. Do we know for sure that there aren't frameworks and libraries
that use Class.forName on proxy classes? We've had issues in the past
with changes in this area.

It's too early to say but it might be that the compatibility risks may
nudge this one into creating a new API.

-Alan.

Proxies will have to be rethought at some future point - wrt Valhalla.

The current specification says:

A proxy class implements exactly the interfaces specified at its
creation, in the same order. Invoking getInterfaces on its Class object
will return an array containing the same list of interfaces (in the
order specified at its creation), [...]

In the current design Valhalla will add two interfaces - IdentityObject
and ValueObject (afaik). One of them have to be implemented as well.
Also, because the superclass is java.lang.reflect.Proxy, and that class
has a protected final field, it can currently not implement ValueObject.

Recently, we (the Valhalla EG) have decided that IdentityObject/ValueObject were not the right way to represent identity and value class.
So no problem anymore on that side.

An other thing are annotations - currently they are implemented using
Proxies. This implementation detail surfaces when serializing an
annotation. Other implementation strategies are possible - for example
spinning a record at runtime.
But this leads to the question - how can one migrate away from
serialized proxies in a compatible way?

In the end - a lot of stuff in the JDK depends on Proxies, and their
limitations now begins to surface.

A new API may not be a bad idea. :)

Yes !
And we can leverage invokedynamic/method handles to avoid boxing/ bad perf.
The idea is that first time an implementation of an abstract method is required, an object (implementing an interface similar to InvocationHandler) acting as a bootstrap method is called to provide a method handle that will be used as implementation.

see https://github.com/forax/hidden_proxy for a prototype of that idea.

- Johannes

R?mi

@mlchung
Copy link
Member

mlchung commented Apr 18, 2022

It's good to see more experiment and prototype for this issue. Like Alan said, the spec change, compatibility risks and security issues in particular on the serialization spec/impl change require lot of discussions and also prototyping of other alternatives. A new API may also be one alternative to consider.

The current spec of Proxy class is defined with null protection domain (same protection domain as the bootstrap class loader). Lookup::defineHiddenClass defines the hidden class in the same protection domain as the defining class loader. This would become a non-issue when the security manager is removed. However, before the removal of security manager, we still need to look into the compatibility risks and what and how applications/libraries depend on this behavior.

During the development of JEP 371, I had a prototype of converting dynamic proxies to hidden class by adding a shim class (that's what your prototype does). That raises the question "how to get a Lookup on a package for the dynamic proxy class to be injected in without injecting a shim class (i.e. your anchor class)?" This functionality could be considered as a separate RFE. However, frameworks don't always have the access to inject a class in a package defined to a class loader. This is something worth looking into.

@liach
Copy link
Member Author

liach commented Apr 18, 2022

I strongly recommend a new API over revamping Proxy itself. https://github.com/forax/hidden_proxy would be a good prototype that serves most needs of the current Proxy API (except a few, like invoking default superinterface method). With hidden classes, I don't see much value in leaving the new API's produced instance in separate modules; what we have for hidden classes should be fine for the most part.

Imo the main obstacle is still the handling of serialization. The annotations are serializable, but currently hidden classes do not work with serialization at all and must use writeReplace and readResolve. And how to migrate annotations off proxies without breaking serialization is still a question as well. Maybe we can upgrade invocation handlers to allow them to declare custom readResolve logic for the proxy to facilitate migration away. How the new API will treat serialization is still undetermined.

@mlbridge
Copy link

mlbridge bot commented Apr 18, 2022

Mailing list message from Remi Forax on core-libs-dev:

----- Original Message -----

From: "liach" <duke at openjdk.java.net>
To: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "security-dev" <security-dev at openjdk.java.net>
Sent: Monday, April 18, 2022 10:01:39 PM
Subject: Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

On Sun, 17 Apr 2022 16:17:30 GMT, liach <duke at openjdk.java.net> wrote:

Convert dynamic proxies to hidden classes. Modifies the serialization of proxies
(requires change in "Java Object Serialization Specification"). Makes the
proxies hidden in stack traces. Removes duplicate logic in proxy building.

The main compatibility changes and their rationales are:
1. Modification to the serialization specification: In the "An instance of the
class is allocated... The contents restored appropriately" section, I propose
explicitly state that handling of proxies are unspecified as to allow
implementation freedom, though I've seen deliberate attempts for proxies to
implement interfaces with `readResolve` in order to control their serialization
behavior.
- This is for the existing generated constructor accessor is bytecode-based,
which cannot refer to hidden classes.
- An alternative is to preserve the behavior, where the serialization
constructor calls `invokespecial` on the closest serializable superclass'
no-arg constructor, like in #1830 by @DasBrain.
- My rationale against preservation is such super calls are unsafe and should be
discouraged in the long term. Calling the existing constructor with a dummy
argument, as in my implementation, would be more safe.
2. The disappearance of proxies in stack traces.
- Same behavior exists in lambda expressions: For instance, in `((Runnable) ()
-> { throw new Error(); }).run();`, the `run` method, implemented by the
lambda, will not appear in the stack trace, and isn't too problematic.

A summary of the rest of the changes:
1. Merged the two passes of determining module and package of the proxy into
one. This reduced a lot of code and allowed anchor class (for hidden class
creation) selection be done together as well.
2. Exposed internal API for obtaining a full-privileged lookup to the rest of
`java.base`. This API is intended for implementation of legacy (pre
`MethodHandles.Lookup`) caller sensitive public APIs so they don't need more
complex tricks to obtain proper permissions as lookups.
3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959):
passes methods computed by proxy generator as class data to the hidden proxy
class to reduce generated proxy class size and improve performance.

In addition, since this change is somewhat large, should we keep the old proxy
generator as well and have it toggled through a command-line flag (like the old
v49 proxy generator or the old reflection implementation)?

Please feel free to comment or review. This change definitely requires a CSR,
but I have yet to determine what specifications should be changed.

I strongly recommend a new API over revamping `Proxy` itself.
https://github.com/forax/hidden_proxy would be a good prototype that serves
most needs of the current Proxy API (except a few, like invoking default
superinterface method).

The third parameter of defineProxy() is a lambda which is called for each method that can be overriden, either toString/equals/hashCode but also any default methods,
if the lambda return true, the method is overriden, otherwise the default implementation is used.

With hidden classes, I don't see much value in leaving
the new API's produced instance in separate modules; what we have for hidden
classes should be fine for the most part.

Imo the main obstacle is still the handling of serialization. The annotations
are serializable, but currently hidden classes do not work with serialization
at all and must use `writeReplace` and `readResolve`. And how to migrate
annotations off proxies without breaking serialization is still a question as
well. Maybe we can upgrade invocation handlers to allow them to declare custom
`readResolve` logic for the proxy to facilitate migration away. How the new API
will treat serialization is still undetermined.

yes, i suppose that like with lambdas, we need a special overload of defineProxy that automatically implements writeReplace() and use a specific class SerializableProxy (mirroring how SerializableLambda works).

@liach
Copy link
Member Author

liach commented Apr 19, 2022

The third parameter of defineProxy() is a lambda which is called for each method that can be overriden, either toString/equals/hashCode but also any default methods,
if the lambda return true, the method is overriden, otherwise the default implementation is used.

Not quite, I mean calling default implementations in the super interface, consider:

interface Root { void cleanUp(); }
interface FeatureOne extends Root { default void cleanUp() { /* do something */ } }
interface FeatureTwo extends Root { default void cleanUp() { /* do something */ } }

My proxy implements both feature one and two, but in your API, there is no way for my cleanUp to call both FeatureOne.super.cleanUp(); and FeatureTwo.super.cleanUp();. You should probably expose the lookup in your linker too, especially that you enabled nest access and you can just use that lookup to resolve, say, an implementation static method in the proxy user class.

Also the "delegate" in your API would significantly benefit from https://bugs.openjdk.java.net/browse/JDK-8282798 (#7744), too.

@mlbridge
Copy link

mlbridge bot commented Apr 19, 2022

Mailing list message from Remi Forax on core-libs-dev:

----- Original Message -----

From: "liach" <duke at openjdk.java.net>
To: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "security-dev" <security-dev at openjdk.java.net>
Sent: Tuesday, April 19, 2022 3:31:24 AM
Subject: Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes

On Mon, 18 Apr 2022 20:42:48 GMT, Remi Forax <forax at univ-mlv.fr> wrote:

The third parameter of defineProxy() is a lambda which is called for each method
that can be overriden, either toString/equals/hashCode but also any default
methods,
if the lambda return true, the method is overriden, otherwise the default
implementation is used.

Not quite, I mean calling default implementations in the super interface,
consider:

interface Root { void cleanUp(); }
interface FeatureOne extends Root { default void cleanUp() { /* do something */
} }
interface FeatureTwo extends Root { default void cleanUp() { /* do something */
} }

My proxy implements both feature one and two, but in your API, there is no way
for my `cleanUp` to call both `FeatureOne.super.cleanUp();` and
`FeatureTwo.super.cleanUp();`. You should probably expose the lookup in your
linker too, especially that you enabled nest access and you can just use that
lookup to resolve, say, an implementation static method in the proxy user
class.

yes, you are right, i should send the Lookup too.

Also the "delegate" in your API would significantly benefit from
https://bugs.openjdk.java.net/browse/JDK-8282798 (#7744), too.

I don't think i need the carrier API, the idea is to have only one field in the proxy, this field can be a value type (exactly a primitive class) in the future, so being expanded into multiple fields by the VM at runtime.

@DasBrain
Copy link
Member

DasBrain commented Apr 19, 2022

About deserializing Proxies, this is currently done in 3 steps:

  1. Finding the class
  2. Allocating a new instance and invoke the constructor of the first non-serializable superclass
  3. Setting the fields of the instance

Only step 2 is problematic here:

  1. For a proxy, the class is serialized using a TC_PROXYCLASSDESC. when deserialized it invokes Proxy.getProxyClass (

    Class<?> proxyClass = Proxy.getProxyClass(
    ).
    For this step, it doesn't matter if Proxy.getProxyClass returns a normal class or a hidden class.

  2. Allocating and calling the constructor:
    This part is currently implemented by spinning a class. The generated bytecode looks more or less like this:

     anew ProxyClass
     invokespecial Object.<init>()
    

    The big lie here is that methods and constructors are different - but constructors are just methods, and the verifier makes sure that a constructor is called only once, exactly once, and that uninitialized instances don't escape.

    This doesn't work for hidden classes - as the hidden class can not be named in an other class.
    But MethodHandles can do this. Here is one of my old prototypes, based on a prototype for implementing reflection using MethodHandles from Mandy Chung: DasBrain/jdk@ae45c5d...fbb0a63

  3. Setting the fields uses deep reflection.
    But the hidden class themself doesn't have any fields - just it's superclass java.lang.reflect.Proxy.h.
    This is not a problem if the class can be instantiated at all (see step 2).

@liach
Copy link
Member Author

liach commented Apr 19, 2022

Fixing deserialization is not particularly hard. A longer term goal is to declare a readResolve for proxies (presumably by extra methods on invocation handlers) to convert serialization result to something else

@liach
Copy link
Member Author

liach commented Apr 29, 2022

/label remove security

@openjdk openjdk bot removed the security security-dev@openjdk.org label Apr 29, 2022
@openjdk
Copy link

openjdk bot commented Apr 29, 2022

@liach
The security label was successfully removed.

@liach
Copy link
Member Author

liach commented Apr 29, 2022

So, after reading the updated valhalla documentation, namely after the expert group decides to represent identity vs value with flags without touching inheritance (so saves serialization breakage when there's no spurious IdentityObject) and that LambdaMetafactory will reject identity or value interfaces per Value Objects JEP, I wonder about the future of dynamic proxies as well. I expect proxies to reject identity or value interfaces like LambdaMetafactory, and we most likely need a new API, like Remi's, if we wish to support identity/value interfaces.

Also for deserialization, since we have Unsafe.allocateInstance, we might alternatively replace the anew bytecode with such a call for the native serialization constructor if we do serialize and deserialize any hidden class like proxies, without touching the security part.

@bridgekeeper
Copy link

bridgekeeper bot commented May 27, 2022

@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@liach liach closed this May 27, 2022
@liach liach deleted the feature/hidden-proxy branch May 23, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org csr Pull request needs approved CSR before integration rfr Pull request is ready for review
3 participants