JDK-8229959/JDK-8242888 Prototype for j.l.r.Proxy with hidden classes & class data

[Johannes Kuhn]

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
>>
>