Thanks Mandy,
Appreciate your feedback, as always.
On 18-Dec-20 0:01, Mandy Chung wrote:
Hi Johannes,
I'm glad to see this prototype. Converting dynamic proxy to hidden
classes has a few challenges as JDK-8242888 describes.
1) the serialization specification w.r.t. dynamic proxies
Serialization is a must have, because annotation are implemented using
dynamic proxies.
But I made it work in my prototype.
The serialization format uses a special PROXYCLASSDESC - basically an
array of CLASSDESC of all implemented interfaces of the dynamic proxy.
It then asks j.l.r.Proxy for the proxy class that implements those
interfaces.
The issue I run in was the way the proxy class is instantiated.
ReflectionFactory::newConstructorForSerialization did spin a class that
referenced the expected type by it's binary name.
To avoid that, I created a MethodHandleConstructorAccessor, that just
delegates call to a MethodHandle. j.l.invoke internals are very powerful
and can do such shenanigans.
In the end, I think no specification change is necessary, at least for
that part. (Other parts may need one - the jshell test shows that such a
change is visible in the stack trace)
We need to look into the implication to the specification and whether
the default serialization mechanism should (or should not) support
dynamic proxies if it's defined as hidden classes.
2) 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)?
Frameworks don't always have the access to inject a class in a package
defined to a class loader. Dynamic proxies are a use case to determine
what functionality is needed to avoid spinning a shim class.
That topic certainly needs a lot more thought.
What are the requirements, what kind of frameworks exist, what do they
need...
For the dynamic proxy case: it just needs two unique packages per
ClassLoader. Names don't matter, as long as they don't clash with
anything else.
Currently they don't clash, because the specification says: don't name
your packages this way.
But a hidden class (and Lookup.defineClass) will always have package
private access to your package. This may or may not be intended by a
framework - especially if it compiles user code, like xerces or clojure.
3) protection domain
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 requires to
understand deeper the compatibility risks and what and how
applications/libraries depend on this behavior.
My anchor has a null PD, hidden classes share the that, so it should be
fine (for now). (Class::getProtectionDomain returns an all permission PD
if the class has a null PD - interesting. Looks like THAT is copied to
the hidden class??)
I think the original security reasoning for why proxies have a null PD
is fine goes something like this:
1. The code in the proxy is trusted, aside from <clinit> it will only
ever calls InvocationHandler.invoke.
2. The proxy itself should not contribute to the protection domains on
the stack - as it may sit between two privileged PDs.
The only sensible solution to keep 2. without a null PD is to use the PD
of the invocation handler - even if that PD is null.
I'm not sure I like the idea of having a class with null PD in a package
full of untrusted code, but that is already the case with a non-public
interface.
An other option is to use the PD of the caller - which gets *really*
interesting with serialization (Who is the caller now?).
That's all the feedbacks I can share so far. We need to have a clear
idea w.r.t. serialization spec and compatibility risk to existing code
w.r.t. protection domain and explore other options.
Compatibility risk is always hard to assess. But I don't think that
there is any change necessary for the serialization spec.
Otherwise - as only one new tests did fail - I would consider the risk
low. And the failing test was in jshell, which expected a stack frame
element from the proxy class. So, somewhat expected to break that.
W.r.t. AOT tests, AOT has been disabled in openjdk build [1] and that
may be the reason why these tests fail.
Thanks for the info. For now, I just ignore the failures.
Mandy
[1] https://github.com/openjdk/jdk/pull/960
On 12/17/20 2:07 PM, Johannes Kuhn wrote:
Now that class data support for hidden classes in master, I decided to
tackle JDK-8229959 again.
JDK-8229959: Convert proxy class to use constant dynamic [1]
JDK-8242888: Convert dynamic proxy to hidden classes [2]
The idea is simple: Define proxies as hidden classes, pass the methods
as class data, and access it from the Proxy with the
MethodHandles.classDataAt() BSM.
The current prototype[3] works - and aside from one test for jshell
that expects a stack trace element containing "jdk.proxy" as a stack
trace element (hidden classes, duh), no new tests did fail with "make
test-tier1".
Also, the aot tests fail ("link.exe not found"), if someone has some
instructions how to get them properly run on windows, I'm very
interested. But they are not new failures.
Problems I did run into:
I need a lookup for a class in a package to define a hidden class.
---------------------------
Solution: I inject an anchor class, obtain a lookup on it, and use
that. The anchor is reused for the same package, and named pkg +
".$ProxyAnchor".
I need to return both the class data & created bytecode back to Proxy
from ProxyGenerator
---------------------------
Solution: Added a record-like class that contains both bytecode as
well as the class data.
Serialization of proxies did break.
---------------------------
Serializing the proxy descriptor was not a problem - the problem is
how the constructor for serialization works.
It spins a MagicConstructorAccessor subclass - which requires for the
created class to have a binary name. Hidden classes don't have one.
Solution: MethodHandles don't have this limitation, so I hacked a
shared secret into JavaLangInvokeAccess to create such a MethodHandle,
and replaced the Reflection.generateConstructor() implementation to
use a ConstructorAccessor that delegates to that MethodHandle.
---------------------------
In all, this is a prototype - for now, I want some feedback on the
approach. Also a broader view - making it work is one thing.
Checking all the right boxes in all the different areas an other.
Some parts might still be a bit sloppy, but I want to know if there is
merit if I follow the current path and polish it up.
- Johannes
PS.: I decided to use a draft PR - as with my last approaches I had
problems when I did commit more stuff.
[1]: https://bugs.openjdk.java.net/browse/JDK-8229959
[2]: https://bugs.openjdk.java.net/browse/JDK-8242888
[3]: https://github.com/openjdk/jdk/pull/1830/files
