Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
On Sun, 17 Apr 2022 16:17:30 GMT, 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.
>
> 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.
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`
(https://github.com/openjdk/jdk/blob/13fb1eed52f1a9152242119969a9d4a0c0627513/src/java.base/share/classes/java/io/ObjectInputStream.java#L891).
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.()
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:
https://github.com/dasbrain/jdk/compare/ae45c5de7fc635df4dc86b764405158c245d2765...fbb0a63436f696a85e7039c0e109c379dfa1edce
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).
-
PR: https://git.openjdk.java.net/jdk/pull/8278
Re: RFR: 8283237: CallSite should be a sealed class
On Wed, 16 Mar 2022 13:09:30 GMT, liach wrote: > Change `CallSite` to a sealed class, as `CallSite` is an abstract class which > does not allow direct subclassing by users per its documentation. Since I > don't have a JBS account, I posted the content for the CSR in a GitHub Gist > at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and wish someone > can submit a CSR for me. Marked as reviewed by jkuhn (Author). - PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8280377: MethodHandleProxies does not correctly invoke default methods with varags [v4]
On Tue, 25 Jan 2022 21:35:27 GMT, Mandy Chung wrote: >> The MethodHandle of a default method should be made as a fixed arity method >> handle because it is invoked via Proxy's invocation handle with a non-vararg >> array of arguments. On the other hand, the >> `InvocationHandle::invokeDefault` method was added in Java 16 to invoke a >> default method of a proxy instance. This patch simply converts the >> implementation to call `InvocationHandle::invokeDefault` instead. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > add comment My question was for when a library wants to implement something similar to `MethodHandleProxies.asInterfaceInstace`, and for example supports Interfaces with more than a single abstract method. Currently, such a library would also have to call `ReflectAccess.invokeDefault`, as the interface may not be accessible by the `InvocationHandler` (as it could be the case with `MethodHandleProxies.asInterfaceInstace` But this might be a discussion for an other time. Patch looks good. - PR: https://git.openjdk.java.net/jdk/pull/7185
Re: RFR: 8280377: MethodHandleProxies does not correctly invoke default methods with varags [v2]
On Mon, 24 Jan 2022 23:03:49 GMT, Mandy Chung wrote:
>> The MethodHandle of a default method should be made as a fixed arity method
>> handle because it is invoked via Proxy's invocation handle with a non-vararg
>> array of arguments. On the other hand, the
>> `InvocationHandle::invokeDefault` method was added in Java 16 to invoke a
>> default method of a proxy instance. This patch simply converts the
>> implementation to call `InvocationHandle::invokeDefault` instead.
>
> Mandy Chung has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - revert MethodHandlesProxiesTest change
> - Add new regression test
> - Should not perform access check on the interface as specified
Changes requested by jkuhn (Author).
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 206:
> 204: if (isDefaultMethod(method)) {
> 205: // no additional access check is performed
> 206: return JLRA.invokeDefault(proxy, method, null,
> args);
Isn't the argument order `..., args, caller`?
src/java.base/share/classes/java/lang/reflect/ReflectAccess.java line 134:
> 132: throws Throwable {
> 133: return Proxy.invokeDefault(proxy, method, args, caller);
> 134: }
What about other, non-jdk code that wishes to implement a similar thing - make
a proxy for an arbitrary interface and handle default methods by invoking them?
-
PR: https://git.openjdk.java.net/jdk/pull/7185
Re: RFR: 8280377: MethodHandleProxies does not correctly invoke default methods with varags
On Fri, 21 Jan 2022 22:49:38 GMT, Mandy Chung wrote: > The MethodHandle of a default method should be made as a fixed arity method > handle because it is invoked via Proxy's invocation handle with a non-vararg > array of arguments. On the other hand, the `InvocationHandle::invokeDefault` > method was added in Java 16 to invoke a default method of a proxy instance. > This patch simply converts the implementation to call > `InvocationHandle::invokeDefault` instead. When testing this patch from a named module and not-exported package, I get the following exception: Exception in thread "main" java.lang.reflect.UndeclaredThrowableException at jdk.proxy1/com.sun.proxy.jdk.proxy1.$Proxy0.toString(Unknown Source) at test.openjdk/test.openjdk.ProxyTest.main(ProxyTest.java:22) Caused by: java.lang.IllegalAccessException: class java.lang.invoke.MethodHandleProxies$1 (in module java.base) cannot access interface test.openjdk.ProxyTest$Test (in module test.openjdk) because module test.openjdk does not export test.openjdk to module java.base at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:394) at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:674) at java.base/java.lang.reflect.InvocationHandler.invokeDefault(InvocationHandler.java:278) at java.base/java.lang.invoke.MethodHandleProxies$1.invoke(MethodHandleProxies.java:202) ... 2 more [My test code](https://gist.github.com/DasBrain/60dbc1c9075b15635d9c87e8295f1c1a). Running without the patch results in the wrong output for default varargs methods, so this is a regression. - PR: https://git.openjdk.java.net/jdk/pull/7185
Re: Looking for a starter task. Maybe JDK-8253396 (Please add `not()` method to `java.util.function.BiPredicate`)?
Hi and welcome. I would not consider JDK-8253396 (Please add `not()` method to `java.util.function.BiPredicate`) a good starter bug. Generally, any bug that tries to add new APIs are hard, and require a lot of time and effort. While the implementation of java.util.function.BiPredicate.not() is probably trivial, doing this task requires much more work: * There should be some discussion if this feature is needed - this mailing list is the right place for that. * The method needs a well written specification, in form of a javadoc comment. * Tests need to be written to ensure conformance with the specification. This is my current understanding on how to add new APIs. If I am wrong, please correct me. I would not recommend adding a new API as a starter bug. From my limited experience, great starter bugs are bugs where the wrong exception is thrown - they are easy to fix, but still require you to write additional tests. But if you find a sponsor who helps you implement JDK-8253396, then go for it. - Johannes On 20-Mar-21 7:31, Suren Nihalani wrote: Hi, I am new openjdk (I've been using java for 7 years but I am new to contributing to openjdk!). I was looking for interesting starter tasks to help out with. JDK-8253396 looked like an easy to implement candidate. I am open to other suggestions too (feel free to little r as well)! The contribution guide suggested I socialize my change before I code it up. Seems like the implementation would be straightforward and similar to Predicate.java. Are folks okay with this change? Thanks for looking into this!
Re: RFR: 8263508: Remove dead code in MethodHandleImpl
On Fri, 12 Mar 2021 13:27:39 GMT, Claes Redestad wrote: > Remove unused methods. LGTM - Marked as reviewed by jkuhn (Author). PR: https://git.openjdk.java.net/jdk/pull/2969
Re: JDK-8262003: Class.arrayType should not throw IllegalArgumentException
Thanks Mandy. Will amend the CSR to cover TypeDescriptor.OfField::arrayType as well, and create a patch. Where should I put the tests? Didn't find any tests that exercise Class::arrayType in test/jdk/java/lang/. Any wishes for tests that I should add w.r.t. Class::arrayType? - Johannes On 24-Feb-21 20:54, Mandy Chung wrote: Hi Johannes, Changing Class::arrayType to throw ISA makes sense to me. I think `TypeDescriptor.ofField::arrayType` spec should also be updated to throw ISA to follow what JVM checks for array dimension because it's a static constraint check [1] (rather than a resolution error) for `anewarray` instruction to create an array no more than 255 dimensions. The CSR looks okay. I agree that the compatibility risk is low. I'd like to review PR along with CSR together. Mandy [1] https://docs.oracle.com/javase/specs/jvms/se15/html/jvms-4.html#jvms-4.9.1 On 2/23/21 6:38 AM, Johannes Kuhn wrote: I want to learn about writing a CSR, and need a sponsor teaching me the ropes. Bug: https://bugs.openjdk.java.net/browse/JDK-8262003 Currently, Class.arrayType() will throw an IllegalArgumentException if the maximum number of dimensions will be exceeded. Throwing an IllegalArgumentException from a method that doesn't take an argument is, at least, strange. Therefore I would like to update the specification to allow this method to throw an IllegalStateException, similar to what ClassDesc.arrayType() does. The current plan is: * Create a CSR detailing the spec changes: https://bugs.openjdk.java.net/browse/JDK-8262211 * Surround the code with a try-catch block. Checking for the error case is hard, and somewhat rare, so I think this is appropriate. * Copy the @throws javadoc line from ClassDesc.arrayType to Class.arrayType But there are a few questions I would love to get the answer on: * Both Class.arrayType() and ClassDesc.arrayType() are specified by TypeDescriptor.OfField. Should the specification of TypeDescriptor.OfField.arrayType() changed as well? * Is the draft of my CSR fine? Should I add some details, or omit things? Rephrase things? In advance, thanks for any support and feedback on this. - Johannes
JDK-8262003: Class.arrayType should not throw IllegalArgumentException
I want to learn about writing a CSR, and need a sponsor teaching me the ropes. Bug: https://bugs.openjdk.java.net/browse/JDK-8262003 Currently, Class.arrayType() will throw an IllegalArgumentException if the maximum number of dimensions will be exceeded. Throwing an IllegalArgumentException from a method that doesn't take an argument is, at least, strange. Therefore I would like to update the specification to allow this method to throw an IllegalStateException, similar to what ClassDesc.arrayType() does. The current plan is: * Create a CSR detailing the spec changes: https://bugs.openjdk.java.net/browse/JDK-8262211 * Surround the code with a try-catch block. Checking for the error case is hard, and somewhat rare, so I think this is appropriate. * Copy the @throws javadoc line from ClassDesc.arrayType to Class.arrayType But there are a few questions I would love to get the answer on: * Both Class.arrayType() and ClassDesc.arrayType() are specified by TypeDescriptor.OfField. Should the specification of TypeDescriptor.OfField.arrayType() changed as well? * Is the draft of my CSR fine? Should I add some details, or omit things? Rephrase things? In advance, thanks for any support and feedback on this. - Johannes
Re: JDK-6824466 java.lang.reflect.Method should use java.lang.invoke.MethodHandle
I now implemented a prototype for JDK-6824466: [1] The goal I set here was to remove the old AccessorGenerator. The good news: - It works - all tier1 tests pass, except one that requires the old implementation. - A lot of code has been removed. And even more could be removed. The bad news: - I hit a performance regression [2]. The JMH tests were written by Peter Levart [3]. As much as I don't like the old AccessorGenerator, I have to say that it is very optimized. If you have any ideas on how the performance could be improved, or how I could find that out where the performance drops, let me know. - Johannes PS.: I played around with adding some @Stable and @ForceInline annotations, and got a big performance improvement for the const cases (~9 ns/op). But that came with other performance regressions for the non-const cases. [1]: https://github.com/openjdk/jdk/pull/2457 [2]: https://gist.github.com/DasBrain/09b37510955b14f0ad82313df8d8de37#gistcomment-3623292 [3]: http://cr.openjdk.java.net/~plevart/jdk-dev/6824466_MHReflectionAccessors/
Re: RFR: 8261254: Initialize charset mapping data lazily
On Sun, 7 Feb 2021 19:08:18 GMT, Claes Redestad wrote:
> This patch refactor JDK internal charsets to initialize charset mapping data
> lazily when needed via holder classes. This means both a startup improvement
> in some cases, and possible throughput improvements for all DoubleByte-based
> Charsets.
>
> Testing: tier1-3
Looks good to me.
src/java.base/share/classes/java/lang/ModuleLayer.java line 936:
> 934: for (Module m : nameToModule.values()) {
> 935: servicesCatalog.register(m);
> 936: }
Seems to be unrelated, but it's not a bad change.
-
Marked as reviewed by jkuhn (Author).
PR: https://git.openjdk.java.net/jdk/pull/2449
Re: RFR: 8013527: calling MethodHandles.lookup on itself leads to errors [v2]
On Wed, 3 Feb 2021 18:40:15 GMT, Mandy Chung wrote: >> JDK-8013527: calling MethodHandles.lookup on itself leads to errors >> JDK-8257874: MethodHandle injected invoker doesn't have necessary private >> access >> >> Johannes Kuhn is also a contributor to this patch. >> >> A caller-sensitive method can behave differently depending on the identity >> of its immediate caller. If a method handle for a caller-sensitive method is >> requested, this resulting method handle behaves as if it were called from an >> instruction contained in the lookup class. The current implementation >> injects >> a trampoline class (aka the invoker class) which is the caller class invoking >> such caller-sensitive method handle. It works in all CSMs except >> `MethodHandles::lookup` >> because the caller-sensitive behavior depends on the module of the caller >> class, >> the class loader of the caller class, the accessibility of the caller class, >> or >> the protection domain of the caller class. The invoker class is a hidden >> class >> defined in the same runtime package with the same protection domain as the >> lookup class, which is why the current implementation works for all CSMs >> except >> `MethodHandles::lookup` which uses the caller class as the lookup class. >> >> Two issues with current implementation: >> 1. The invoker class only has the package access as the lookup class. It >> cannot >> access private members of the lookup class and its nest members. >> >> The fix is to make the invoker class as a nestmate of the lookup class. >> >> 2. `MethodHandles::lookup` if invoked via a method handle produces a >> `Lookup` >> object of an injected invoker class which is a bug. >> >> There are two alternatives: >> - define the invoker class with the lookup class as the class data such that >> `MethodHandles::lookup` will get the caller class from the class data if >> it's the injected invoker >> - Johannes' proposal [1]: detect if an alternate implementation with an >> additional >> trailing caller class parameter is present, use the alternate >> implementation >> and bind the method handle with the lookup class as the caller class >> argument. >> >> There has been several discussions on the improvement to support caller >> sensitive >> methods for example the calling sequences and security implication. I have >> looked at how each CSM uses the caller class. The second approach (i.e. >> defining an alternate implementation for a caller-sensitive method taking >> an additional caller class parameter) does not work for non-static non-final >> caller-sensitive method. In addition, it is not ideal to pollute the source >> code to provide an alternatve implementation for all 120+ caller-sensitive >> methods >> whereas the injected invoker works for all except `MethodHandles::lookup`. >> >> I propose to use both approaches. We can add an alternative implementation >> for >> a caller-sensitive method when desirable such as `MethodHandles::lookup` in >> this PR. For the injected invoker case, one could extract the original >> lookup >> class from class data if needed. >> >> test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSM.java ensures that >> no new non-static non-final caller-sensitive method will be added to the JDK. >> I extend this test to catch that non-static non-final caller-sensitive method >> cannot have the alternate implementation taking the additional caller class >> parameter. >> >> This fix for JDK-8013527 is needed by the prototype for JDK-6824466 I'm >> working on. >> >> [1] >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-January/073184.html > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Support MethodHandles::lookup called via Method::invoke via MethodHandle Looks good. - Marked as reviewed by dasbr...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/2367
Re: RFR: 8013527: calling MethodHandles.lookup on itself leads to errors [v2]
On Wed, 3 Feb 2021 18:34:55 GMT, Mandy Chung wrote: >> You are right, got it confused with the future use. >> >> With this fix, MethodHandle -> Method.invoke -> MethodHandles.lookup() will >> still return a lookup on the injected invoker. >> I somehow missed that this is not part of the fix, but for the future use. > > `MethodHandle -> Method.invoke -> MethodHandles.lookup() ` is a corner case > that can be fixed easily using the class data approach. See the new commit. The security issue I mentioned was in an other branch, method-invoke. I used commit https://github.com/mlchung/jdk/commit/4a3c914f1b46cf84b42f6b6bc19d421955faac3f (i.e. before strengthening the injected invoker checks) to test the [my exploit](https://gist.github.com/DasBrain/4dda6cc3a13e1636afe17e6a02ec3d12). (Yes, full sandbox escape.) I hope the same is not possible with the nestmate requirement. PS.: Hidden Class -> MethodHandle -> Method.invoke -> MethodHandles might break due to mangling of the hidden class name for the injected invoker. Will write a test. - PR: https://git.openjdk.java.net/jdk/pull/2367
Re: RFR: 8013527: calling MethodHandles.lookup on itself leads to errors
On Wed, 3 Feb 2021 17:25:04 GMT, Mandy Chung wrote: >> Only `Lookup` with the original access can access `MethodHandles.classData`. >> A hidden class `HC$$InjectedInvoker/0x1234` can access private members of >> another class `C` in the same nest but not `C`'s class data. >> >> I don't follow which previous commit you refer to more dangerous. Please >> elaborate. I don't see any security concern with class data. > > Last night, I had a second thought that the fix for these two JBS issues does > not need the class data. It's more for a future use. I plan to revise it > and drop class data in this fix anyway. You are right, got it confused with the future use. With this fix, MethodHandle -> Method.invoke -> MethodHandles.lookup() will still return a lookup on the injected invoker. I somehow missed that this is not part of the fix, but for the future use. - PR: https://git.openjdk.java.net/jdk/pull/2367
Re: RFR: 8013527: calling MethodHandles.lookup on itself leads to errors
On Wed, 3 Feb 2021 01:50:36 GMT, Mandy Chung wrote:
> JDK-8013527: calling MethodHandles.lookup on itself leads to errors
> JDK-8257874: MethodHandle injected invoker doesn't have necessary private
> access
>
> Johannes Kuhn is also a contributor to this patch.
>
> A caller-sensitive method can behave differently depending on the identity
> of its immediate caller. If a method handle for a caller-sensitive method is
> requested, this resulting method handle behaves as if it were called from an
> instruction contained in the lookup class. The current implementation injects
> a trampoline class (aka the invoker class) which is the caller class invoking
> such caller-sensitive method handle. It works in all CSMs except
> `MethodHandles::lookup`
> because the caller-sensitive behavior depends on the module of the caller
> class,
> the class loader of the caller class, the accessibility of the caller class,
> or
> the protection domain of the caller class. The invoker class is a hidden
> class
> defined in the same runtime package with the same protection domain as the
> lookup class, which is why the current implementation works for all CSMs
> except
> `MethodHandles::lookup` which uses the caller class as the lookup class.
>
> Two issues with current implementation:
> 1. The invoker class only has the package access as the lookup class. It
> cannot
> access private members of the lookup class and its nest members.
>
> The fix is to make the invoker class as a nestmate of the lookup class.
>
> 2. `MethodHandles::lookup` if invoked via a method handle produces a `Lookup`
> object of an injected invoker class which is a bug.
>
> There are two alternatives:
> - define the invoker class with the lookup class as the class data such that
> `MethodHandles::lookup` will get the caller class from the class data if
> it's the injected invoker
> - Johannes' proposal [1]: detect if an alternate implementation with an
> additional
> trailing caller class parameter is present, use the alternate implementation
> and bind the method handle with the lookup class as the caller class
> argument.
>
> There has been several discussions on the improvement to support caller
> sensitive
> methods for example the calling sequences and security implication. I have
> looked at how each CSM uses the caller class. The second approach (i.e.
> defining an alternate implementation for a caller-sensitive method taking
> an additional caller class parameter) does not work for non-static non-final
> caller-sensitive method. In addition, it is not ideal to pollute the source
> code to provide an alternatve implementation for all 120+ caller-sensitive
> methods
> whereas the injected invoker works for all except `MethodHandles::lookup`.
>
> I propose to use both approaches. We can add an alternative implementation
> for
> a caller-sensitive method when desirable such as `MethodHandles::lookup` in
> this PR. For the injected invoker case, one could extract the original lookup
> class from class data if needed.
>
> test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSM.java ensures that
> no new non-static non-final caller-sensitive method will be added to the JDK.
> I extend this test to catch that non-static non-final caller-sensitive method
> cannot have the alternate implementation taking the additional caller class
> parameter.
>
> This fix for JDK-8013527 is needed by the prototype for JDK-6824466 I'm
> working on.
>
> [1]
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-January/073184.html
Thanks Mandy.
Looks good, except the possibility for an attacker to teleport within the same
nest.
src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1205:
> 1203: Class invokerClass = new Lookup(targetClass)
> 1204: .makeHiddenClassDefiner(name,
> INJECTED_INVOKER_TEMPLATE, Set.of(NESTMATE))
> 1205: .defineClass(true, targetClass);
Using the target class directly could lead to some unintended problems.
An attacker can define it's own hidden class as nestmate and with a name ending
in `$$InjectedInvoker`.
This allows the attacker to "teleport" into a nestmate with full privileges.
An attacker could then access `MethodHandles.classData`.
Suggested remedy: Create a holder that is only visible to `java.lang.invoke`:
/* package-private */ static class OriginalCallerHolder {
final Class originalCaller;
OriginalCallerHolder(Class originalCaller) {
this.originalCaller = originalCaller;
}
}
As this type is only visible inside `java.lang.invoke`, it can't be created
without hacking into `java.lang.invoke`, at which point all bets are
Re: JDK-6824466 java.lang.reflect.Method should use java.lang.invoke.MethodHandle
Thanks Alan.
For using MethodHandles and @CS methods: As far as I remember,
MethodHandle is also skipped on a stack walk for security. In my
prototype (where I did always use MethodHandles for Method.invoke)
MethodHandles.class.findStatic("lookup").invoke(null) did return a
Lookup with the right lookup class.
So, MethodHandles as implementation works - if all classes on the stack
extend MethodAccessorImpl or MethodHandle.
I did a prototype in the past to use an overload for @CS methods [1]
While C2 could link to the overload directly, this is not needed in my
prototype.
The roadblock I hit was that Method.invoke is itself @CS, so either the
Method.invoke overload will have to inject a frame for the caller or all
@CS methods need to have their overload. (And the overload for
Method.invoke needs it's own intrinsic, so it's skipped during security
stack walk.)
The other problem with regard to @CS is Thread.getContextClassLoader().
This method can be virtually called. But a private overload would bypass
the any override.
Currently, java.lang.invoke will bind the caller if the called method is
named "getContextClassLoader" and the method is in an interface.
Things get interesting if you consider this interface:
interface GetCCLObject {Object getContextClassLoader();}
javac will now add a bridge method if you extend Thread and implement
this interface - which calls the @CS method. And that class is now the
caller.
Not exactly sure what my conclusion is, but I tend to "if you implement
such an interface, you are out of luck".
(That's the point my head starts to hurt.)
In the end - Loom has an other implementation of JDK-6824466. Makes 4.
Interesting is that they all have been born out of different
requirements - so it seems indeed to be a good idea.
In my prototype I did fell into the trap that I tried too much at once.
And @CS is a really complex topic that might be attractive to somehow
squeeze into JDK-6824466. But I'm not convinced that this is a good idea.
- Johannes
[1]:
https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-January/073184.html
On 02-Feb-21 11:07, Alan Bateman wrote:
On 01/02/2021 15:19, Johannes Kuhn wrote:
:
Thanks Rémi.
The problem here is that (according to Peter Levart tests), the cold
use of a MHMethodAccessor is 11x as expensive as using the native
accessor.
In some way, it sill makes sense to keep the native accessor around,
at least during startup. On the other hand, I heard that there have
been some improvements on the AppCDS w.r.t MethodHandles, but I'm out
of the loop there.
An other problem with completely replacing the native accessors is
some bootstrap problem - if certain flags are set (some of them are
used to build the CDS), then the java.lang.invoke infrastructure tries
to write stuff to System.out. But to initialize System.out, the
charset has to be constructed, which relies on reflection.
I can see some potential in improving the initialization of the
standard charset, but it's out of the scope here.
Apropos scope: I try to narrow the scope down to "replace
MethodAccessorGenerator with accessors that delegate to MethodHandles".
Once this is done, then additional improvements can be made -
including some that take loom into account. Or adding some
@Stable/@ForceInline stuff so the JIT can better optimize reflective
calls.
But currently (didn't look into the loom source) there are only two
strategies: Native accessor (bad for loom) or generated (bytecode
spinning, doesn't work with hidden classes). So, with loom and hidden
classes you get the worst of both worlds. MethodHandles solve that.
As things stands, virtual threads don't call through the VM because of
the potential for blocking with a native trampoline frame on the stack.
The bytecode generator is still used for @CS methods but otherwise it
uses method handles. Hidden classes should be okay although we don't
have tests for that yet. In any case, this will all be replaced once
Mandy's work is further along. Core reflection usages during initPhase2
can be dealt it by deferral until the module system is fully initialized
(as Mandy suggests) as using invoke count would be too fragile. For @CS
methods you will see linked issues where the suggestion to link to an
overloaded method that takes the caller class has been suggested. I also
suspect there will be some C2 work needed.
-Alan
Re: JDK-6824466 java.lang.reflect.Method should use java.lang.invoke.MethodHandle
Thanks Mandy. Yes, the 3 prototypes I mentioned were yours, Peter Levart's, and my own. (My prototype is at https://github.com/DasBrain/jdk/tree/reflection-MHaccessors - I hit an boostrap problem, build a workaround that I don't really like, so I stopped working on that.) I agree on your goals, they are the same as mine. For the constructor story, well, that is a prerequisite for the hidden proxy classes. I'll take an other look at your implementation, and maybe open a pull request against your repo. Current approach is to keep the scope small - just replacing the old generated accessors with method handle based ones. Even this "small" change is IMHO worthwhile: * Getting rid of the old MethodAccessorGenerator (I don't want to maintain it, and if I had to, then moving to ASM would be a step forward) * Better performance on calling methods from hidden classes. (Loom doesn't like native frames on the stack) * Prerequisite for further enchantments, such as hidden proxy classes, alternatives for reflective @CS and possible performance improvements using @Stable/@ForceInline. It's just - it's an old bug, and other people already did try some stuff on it. So I try to learn from their experience and try to understand what roadblocks they hit first. No need to repeat the mistakes others already made. - Johannes On 01-Feb-21 18:37, Mandy Chung wrote: Hi Johannes, I believe you are aware of the prototype I'm working on: https://github.com/mlchung/jdk/tree/method-invoke My prototype so far replaces method and fields but not constructors yet. You are welcome to contribute. My main motivation of doing this is to get rid of the old clunky bytecode generator and core reflection will be built atop on method handles. This would greatly simplify the work to add support for a new feature for example Valhalla primitive classes only in method handles. I plan to keep the native method accessor for startup use (or switch to method handles when module system is initialized). Mandy On 2/1/21 6:50 AM, Johannes Kuhn wrote: While implementing a prototype for JDK-8242888 (Convert dynamic proxy to hidden classes) I came across the problem that hidden classes can't be mentioned in the constant pool, breaking the constructor for serialization. To remedy that problem, I used a MHConstructorAccessor which delegates the actual work to MethodHandles - not unlike what JDK-6824466 suggests. As there has been previous work in that area, (I am aware of at least 3 independently developed prototypes for that bug/enchantment) I would like to ask a few questions around it: 1) What are the challenges? From the bug I could infer, that it's cold start is slower than NativeMethodAccessor, but still faster than the generated (bytecode spinning) accessors. 2) Are there any roadblocks that prevent replacing the MethodAccessorGenerator with accessors that use MethodHandles? From my limited tests, it appears to work well enough. 3) Should I try to implement it? From my POV, replacing MethodAccessorGenerator with accessors that delegate to MethodHandles has a few benefits: * Support for hidden classes. (Currently fallback to native accessors) * Removal of MethodAccessorGenerator (which is old and clunky) - Johannes
Re: JDK-6824466 java.lang.reflect.Method should use java.lang.invoke.MethodHandle
On 01-Feb-21 15:58, Remi Forax wrote: - Mail original - De: "Johannes Kuhn" À: "core-libs-dev" Envoyé: Lundi 1 Février 2021 15:50:51 Objet: JDK-6824466 java.lang.reflect.Method should use java.lang.invoke.MethodHandle While implementing a prototype for JDK-8242888 (Convert dynamic proxy to hidden classes) I came across the problem that hidden classes can't be mentioned in the constant pool, breaking the constructor for serialization. To remedy that problem, I used a MHConstructorAccessor which delegates the actual work to MethodHandles - not unlike what JDK-6824466 suggests. As there has been previous work in that area, (I am aware of at least 3 independently developed prototypes for that bug/enchantment) I would like to ask a few questions around it: 1) What are the challenges? From the bug I could infer, that it's cold start is slower than NativeMethodAccessor, but still faster than the generated (bytecode spinning) accessors. 2) Are there any roadblocks that prevent replacing the MethodAccessorGenerator with accessors that use MethodHandles? From my limited tests, it appears to work well enough. 3) Should I try to implement it? From my POV, replacing MethodAccessorGenerator with accessors that delegate to MethodHandles has a few benefits: * Support for hidden classes. (Currently fallback to native accessors) * Removal of MethodAccessorGenerator (which is old and clunky) Hi Johannes, the native accessors doesn't play with loom (a C stack frame in the middle of the Java stack can not be moved) so it doesn't seem to be a good idea to rely on the native accessors for hidden classes. - Johannes Rémi Thanks Rémi. The problem here is that (according to Peter Levart tests), the cold use of a MHMethodAccessor is 11x as expensive as using the native accessor. In some way, it sill makes sense to keep the native accessor around, at least during startup. On the other hand, I heard that there have been some improvements on the AppCDS w.r.t MethodHandles, but I'm out of the loop there. An other problem with completely replacing the native accessors is some bootstrap problem - if certain flags are set (some of them are used to build the CDS), then the java.lang.invoke infrastructure tries to write stuff to System.out. But to initialize System.out, the charset has to be constructed, which relies on reflection. I can see some potential in improving the initialization of the standard charset, but it's out of the scope here. Apropos scope: I try to narrow the scope down to "replace MethodAccessorGenerator with accessors that delegate to MethodHandles". Once this is done, then additional improvements can be made - including some that take loom into account. Or adding some @Stable/@ForceInline stuff so the JIT can better optimize reflective calls. But currently (didn't look into the loom source) there are only two strategies: Native accessor (bad for loom) or generated (bytecode spinning, doesn't work with hidden classes). So, with loom and hidden classes you get the worst of both worlds. MethodHandles solve that. - Johannes
JDK-6824466 java.lang.reflect.Method should use java.lang.invoke.MethodHandle
While implementing a prototype for JDK-8242888 (Convert dynamic proxy to hidden classes) I came across the problem that hidden classes can't be mentioned in the constant pool, breaking the constructor for serialization. To remedy that problem, I used a MHConstructorAccessor which delegates the actual work to MethodHandles - not unlike what JDK-6824466 suggests. As there has been previous work in that area, (I am aware of at least 3 independently developed prototypes for that bug/enchantment) I would like to ask a few questions around it: 1) What are the challenges? From the bug I could infer, that it's cold start is slower than NativeMethodAccessor, but still faster than the generated (bytecode spinning) accessors. 2) Are there any roadblocks that prevent replacing the MethodAccessorGenerator with accessors that use MethodHandles? From my limited tests, it appears to work well enough. 3) Should I try to implement it? From my POV, replacing MethodAccessorGenerator with accessors that delegate to MethodHandles has a few benefits: * Support for hidden classes. (Currently fallback to native accessors) * Removal of MethodAccessorGenerator (which is old and clunky) - Johannes
Integrated: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base"
On Fri, 8 Jan 2021 09:38:40 GMT, Johannes Kuhn wrote: > Simple fix - one line change: > https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. > > Most of the changes come from an additional test that fails without this fix: > > Error: Unable to load main class somelib.test.TestMain in module somelib > java.lang.IllegalAccessError: superclass access check failed: class > somelib.test.TestMain (in module somelib) cannot access class > java.lang.Object (in module java.base) because module java.base does not > export java.lang to module somelib > > As described in JDK-8259395. > You can view the output of the test without patch here: > https://github.com/DasBrain/jdk/runs/1668078245 > > Thanks to @AlanBateman for pointing out the right fix. This pull request has now been integrated. Changeset: cf942081 Author:Johannes Kuhn Committer: Alan Bateman URL: https://git.openjdk.java.net/jdk/commit/cf942081 Stats: 454 lines in 11 files changed: 453 ins; 0 del; 1 mod 8259395: Patching automatic module with additional packages re-creates module without "requires java.base" Reviewed-by: attila, alanb - PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v6]
> Simple fix - one line change: > https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. > > Most of the changes come from an additional test that fails without this fix: > > Error: Unable to load main class somelib.test.TestMain in module somelib > java.lang.IllegalAccessError: superclass access check failed: class > somelib.test.TestMain (in module somelib) cannot access class > java.lang.Object (in module java.base) because module java.base does not > export java.lang to module somelib > > As described in JDK-8259395. > You can view the output of the test without patch here: > https://github.com/DasBrain/jdk/runs/1668078245 > > Thanks to @AlanBateman for pointing out the right fix. Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Renamed tests, to make it more clear what they should test. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2000/files - new: https://git.openjdk.java.net/jdk/pull/2000/files/15a057d9..87b4a8ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2000=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2000=04-05 Stats: 26 lines in 1 file changed: 0 ins; 0 del; 26 mod Patch: https://git.openjdk.java.net/jdk/pull/2000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000 PR: https://git.openjdk.java.net/jdk/pull/2000
Integrated: 8259922 MethodHandles.collectArguments does not throw IAE if pos is outside the arity range
On Wed, 20 Jan 2021 18:29:00 GMT, Johannes Kuhn wrote: > Add explicit range check to `MethodHandles.collectArgumentsCheck`. > Added test case that exercises all cases. > > This is a behavioral change, from throwing an unspecified exception to > throwing an IllegalArgumentException, as specified. > No spec change needed, as the IllegalArgumentException is already specified > to be thrown in those cases. > > Feel free to suggest a better place for the tests. This pull request has now been integrated. Changeset: bf5e8015 Author:Johannes Kuhn Committer: Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/bf5e8015 Stats: 86 lines in 2 files changed: 86 ins; 0 del; 0 mod 8259922: MethodHandles.collectArguments does not throw IAE if pos is outside the arity range Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk/pull/2171
Re: RFR: 8259922 MethodHandles.collectArguments does not throw IAE if pos is outside the arity range [v2]
On Fri, 22 Jan 2021 00:48:51 GMT, Johannes Kuhn wrote: >> Looks good. What tests have you ran? > > On the latest commit, only my own test. > On the previous commit (4f74e2d) I did run the full tier1. > > Currently a full test tier1 run on my machine and github actions is in > progress. Tests did run successfully on my machine (win 64) and github actions. Let me know if there should be additional tests that I should run. - PR: https://git.openjdk.java.net/jdk/pull/2171
Re: RFR: 8259922 MethodHandles.collectArguments does not follow its documentation [v2]
On Fri, 22 Jan 2021 00:44:12 GMT, Mandy Chung wrote: >> Johannes Kuhn has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Implement suggestions by Mandy Chung. > > Looks good. What tests have you ran? On the latest commit, only my own test. On the previous commit (4f74e2d) I did run the full tier1. Currently a full test tier1 run on my machine and github actions is in progress. - PR: https://git.openjdk.java.net/jdk/pull/2171
Re: RFR: 8259922 MethodHandles.collectArguments does not follow its documentation [v2]
> Add explicit range check to `MethodHandles.collectArgumentsCheck`. > Added test case that exercises all cases. > > This is a behavioral change, from throwing an unspecified exception to > throwing an IllegalArgumentException, as specified. > No spec change needed, as the IllegalArgumentException is already specified > to be thrown in those cases. > > Feel free to suggest a better place for the tests. Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Implement suggestions by Mandy Chung. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2171/files - new: https://git.openjdk.java.net/jdk/pull/2171/files/4f74e2df..f01fefaa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2171=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2171=00-01 Stats: 179 lines in 3 files changed: 82 ins; 95 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2171.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2171/head:pull/2171 PR: https://git.openjdk.java.net/jdk/pull/2171
Re: RFR: 8259922 MethodHandles.collectArguments does not follow its documentation
On Thu, 21 Jan 2021 22:54:56 GMT, Mandy Chung wrote:
>> Add explicit range check to `MethodHandles.collectArgumentsCheck`.
>> Added test case that exercises all cases.
>>
>> This is a behavioral change, from throwing an unspecified exception to
>> throwing an IllegalArgumentException, as specified.
>> No spec change needed, as the IllegalArgumentException is already specified
>> to be thrown in those cases.
>>
>> Feel free to suggest a better place for the tests.
>
> test/jdk/java/lang/invoke/8259922/TestMethodHandlesCollectArgs.java line 37:
>
>> 35: import static org.testng.Assert.*;
>> 36:
>> 37: public class TestMethodHandlesCollectArgs {
>
> I suggest to rename this test in
> `test/jdk/java/lang/invoke/MethodHandlesCollectArgsTest.java` matching
> existing convention and `CollectArgsTest.java` is also fine with me. The bug
> ID is already in @bug and I find the directory with bug ID adds noise.
Yeah, still learning where to put tests.
-
PR: https://git.openjdk.java.net/jdk/pull/2171
RFR: 8259922 MethodHandles.collectArguments does not follow its documentation
Add explicit range check to `MethodHandles.collectArgumentsCheck`. Added test case that exercises all cases. This is a behavioral change, from throwing an unspecified exception to throwing an IllegalArgumentException, as specified. No spec change needed, as the IllegalArgumentException is already specified to be thrown in those cases. Feel free to suggest a better place for the tests. - Commit messages: - Fix tailing whitespace in in MethodHandles.java - Fix copyright yeahr in MethodHandles.java - Fix JDK-8259922 - Fix JDK-8259922. Changes: https://git.openjdk.java.net/jdk/pull/2171/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2171=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8259922 Stats: 100 lines in 2 files changed: 99 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2171.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2171/head:pull/2171 PR: https://git.openjdk.java.net/jdk/pull/2171
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v5]
> Simple fix - one line change: > https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. > > Most of the changes come from an additional test that fails without this fix: > > Error: Unable to load main class somelib.test.TestMain in module somelib > java.lang.IllegalAccessError: superclass access check failed: class > somelib.test.TestMain (in module somelib) cannot access class > java.lang.Object (in module java.base) because module java.base does not > export java.lang to module somelib > > As described in JDK-8259395. > You can view the output of the test without patch here: > https://github.com/DasBrain/jdk/runs/1668078245 > > Thanks to @AlanBateman for pointing out the right fix. Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Fix comment, and missing newline in module-info.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/2000/files - new: https://git.openjdk.java.net/jdk/pull/2000/files/fc3b2ac0..15a057d9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2000=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2000=03-04 Stats: 7 lines in 5 files changed: 0 ins; 2 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/2000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000 PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v4]
> Simple fix - one line change: > https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. > > Most of the changes come from an additional test that fails without this fix: > > Error: Unable to load main class somelib.test.TestMain in module somelib > java.lang.IllegalAccessError: superclass access check failed: class > somelib.test.TestMain (in module somelib) cannot access class > java.lang.Object (in module java.base) because module java.base does not > export java.lang to module somelib > > As described in JDK-8259395. > You can view the output of the test without patch here: > https://github.com/DasBrain/jdk/runs/1668078245 > > Thanks to @AlanBateman for pointing out the right fix. Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Implement more tests. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2000/files - new: https://git.openjdk.java.net/jdk/pull/2000/files/7c2ed088..fc3b2ac0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2000=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2000=02-03 Stats: 388 lines in 10 files changed: 316 ins; 54 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/2000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000 PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v3]
On Tue, 19 Jan 2021 16:14:51 GMT, Alan Bateman wrote: >>> >>> >>> This issue requires a Reviewer from someone working in this area. Please do >>> not sponsor or integrate until that review has been done. >> >> Ok, increased the number of required reviewers to 2. >> Hope that was the right move, as I don't see any other way to undo >> /integrate. > > Finally getting back to this. The update to ModulePatcher.java is good. Test > coverage needs discussion. There are four scenarios where test coverage is > lacking: > > 1. automatic module on the module path, patched to override or augment > classes/resources > 2. automatic module on the module path, patched to add new packages > 3. automatic module as the initial module, patched to override or augment > classes/resources > 4. automatic module as the initial module, patched to add new packages > > The patch adds automatic/PatchTest.java so it's adding test coverage for 4. > We should probably rename it to leave room for the other tests, or else > create it so that additional test coverage can be added. I assume the test > was copied from another test as there are a few left overs, e.g. `@modules > java.script` but the test does not use this module. I don't want to expand > the scope of this PR too much but I think we should at least cover 3 and 4 in > the test. Thanks @AlanBateman, I now implemented a few more tests. They should cover all four cases you described. - PR: https://git.openjdk.java.net/jdk/pull/2000
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
Thanks Mandy.
Yes, @CS is a complicated beast.
I also implemented a part of JDK-6824466 for my "proxies should use
hidden classes prototype". (Only for the "constructor for
serialization", as hidden classes can't be mentioned in the constant pool.)
For the @CS method that can be called virtually -
Thread.getContextClassLoader, I thought about those two interfaces:
interface GetCCLClassLoader {ClassLoader getContextClassLoader(); }
interface GetCCLObject {Object getContextClassLoader(); }
Insight: If a class extends Thread and implements GetCCLObject, javac
will add a bridge method - which is the caller now. Have to think more
about what this actually means.
The entire topic is very complex - but my current believe is that
JDK-6824466 is a basic building block for a lot of other work in that
area. It also has quite a few prototypes that have been developed
independently - suggesting that it is indeed a basic building block.
I did a quick look though your prototype, one question I could not
answer was "what prevents me from naming my hidden class
Something$$InjectedInvoker?".
I will try to dig deeper into that, sure. I don't think that there will
be any fully satisfying solution in the next months. And then I have to
convince people that those changes don't expose any security issues -
which will be quite hard.
But if you have some starter bugs that I could fix, let me know, might
help to get some reputation and familiarity with the entire process.
Thank you for your time listening to my ideas, I appreciate it :).
- Johannes
On 18-Jan-21 22:52, Mandy Chung wrote:
Hi Johannes,
There has been a couple of the prototypes regarding @CS methods (Alan
did one and I did another) in the context of JDK-6824466. There are lots
of security consideration regarding @CS methods. You are welcome to go
deeper in that area. If you are looking for starter bugs to fix, that
won't be a quick patch.
I also came up with a patch for JDK-8013527 when working on
JDK-6824466. It's buried in
https://github.com/openjdk/jdk/compare/master...mlchung:method-invoke. I
will extract that fix and post a PR on my proposed fix.
Mandy
On 1/16/21 10:07 AM, Johannes Kuhn wrote:
After digging though the JBS, I found this comment from John Rose[1].
I like the general idea, but I don't think it's necessary to use a
native method as stub. Instead it could be written like this:
class Class {
@CallerSensitive
@ForceInline
public static Class forName(String name) {
return forName(name, Reflection.getCallerClass());
}
private static Class forName(String name, Class caller) {
// original implementation
}
}
By doing this, binding to the caller could be done by returning a
MethodHandle that actually calls the private method - with the lookup
class injected as argument (MethodHandles.insertArguments).
Problem are methods that could be virtually invoked
(getContextClassLoader). Therefore it might be useful to keep the old
binding logic around. It also reduces the number of places where this
change has to be done - it's only required for the
hyper-@CallerSensitive methods.
I will try to write a prototype that demonstrates that this approach
is feasible.
- Johannes
[1]:
https://bugs.openjdk.java.net/browse/JDK-8020968?focusedCommentId=13611844=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13611844
On 09-Dec-20 21:09, Johannes Kuhn wrote:
On 09-Dec-20 19:44, Mandy Chung wrote:
On 12/8/20 6:02 PM, Johannes Kuhn wrote:
There are a lot of things to consider when trying to fix JDK-8013527.
Exactly in particular security implication! What is clear is that
the expected lookup class should not be the injected class. The
key message here is that we can't fix JDK-8257874 until we fix
JDK-8013527 unfortunately.
Mandy
Yeah, if JDK-8013527 is fixed it might fix JDK-8257874 as a byproduct.
If Lookup.lookup() can determine the original caller, then
Field.set*/Method.invoke could do the same.
Special care has to be taken that no other class could spoof such an
injected invoker.
Too complicated for me :). JDK-8013527 needs a sound design to
approach fixing it IMHO.
- Johannes
Trying to fix JDK-8013527 - 1st Prototype
JDK-8013527[1] has somehow become the umbrella bug for "Using MethodHandles to call caller sensitive methods leads to interesting results". To recap: A caller sensitive method knows about who called it, and can behave differently when called from an other context. Examples are: Class.forName, MethodHandles.lookup, Method.invoke... A MethodHandle on the other hand should not be caller sensitive. To archive this, a MethodHandle will "bind" the lookup class as caller for caller sensitive methods. This is currently done by injecting a hidden class (InjectedInvoker" that acts as a trampoline for calling caller sensitive methods. This injected invoker shares many properties of the original caller: Same ClassLoader, same Module, same Package, same ProtectionDomain, but it's not the same class or a nestmate of it. For caller sensitive methods that do look at more than just the injected invoker, this leads to "unexpected" results when called through a MethodHandle: * MethodHandles.lookup() returns a full privileged lookup for the injected invoker. * jlr.Field.get*/set*, jlr.Constructor.newInstance, jlr.Method.invoke may fail with an IllegalAccessException if the target is private. See JDK-8257874[2]. --- After reading one of John Rose's comments[3], I thought that this might be a way to solve this general problem. So I implemented some of it here[4]. The basic idea is that there is a private overload of the caller sensitive method which accepts the caller as a tailing argument. The good news: * tier1 Tests pass. * ((Lookup) lookup.findStatic(MethodHandles.class, "lookup", MethodType.methodType(Lookup.class)).invokeExact()).lookupClass() == lookup.lookupClass(); * JDK-8257874 can't be reproduced with Field.* or Constructor. * Performance is likely better. (InjectedInvoker collects all arguments into an Object[].) The bad news: * If you use a MethodHandle to call Method.invoke for a caller sensitive method, then you can still observe the injected invoker. --- Moving forward, there are 3 ways: 1. Do nothing. Won't fix any bug. 2. Use the current prototype, and accept Method.invoke is odd when calling it through a MethodHandle. 3. Go all in: * Require **every** caller sensitive method to have a private overload. * Method.invoke will also use that private overload. The problems with the 3rd approach are: * What about methods that can be called virtually? (Thread.getContextClassLoader()) * Requires a few changes to MethodAccessor. Maybe implementing JDK-6824466[5] first? * What about methods that do stack walks? I have to think more about the problems listed above - but maybe you have some input that could help me on that. - Johannes [1]: https://bugs.openjdk.java.net/browse/JDK-8013527 [2]: https://bugs.openjdk.java.net/browse/JDK-8257874 [3]: https://bugs.openjdk.java.net/browse/JDK-8020968?focusedCommentId=13611844=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13611844 [4]: https://github.com/openjdk/jdk/pull/2117/files [5]: https://bugs.openjdk.java.net/browse/JDK-6824466
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
After digging though the JBS, I found this comment from John Rose[1].
I like the general idea, but I don't think it's necessary to use a
native method as stub. Instead it could be written like this:
class Class {
@CallerSensitive
@ForceInline
public static Class forName(String name) {
return forName(name, Reflection.getCallerClass());
}
private static Class forName(String name, Class caller) {
// original implementation
}
}
By doing this, binding to the caller could be done by returning a
MethodHandle that actually calls the private method - with the lookup
class injected as argument (MethodHandles.insertArguments).
Problem are methods that could be virtually invoked
(getContextClassLoader). Therefore it might be useful to keep the old
binding logic around. It also reduces the number of places where this
change has to be done - it's only required for the
hyper-@CallerSensitive methods.
I will try to write a prototype that demonstrates that this approach is
feasible.
- Johannes
[1]:
https://bugs.openjdk.java.net/browse/JDK-8020968?focusedCommentId=13611844=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13611844
On 09-Dec-20 21:09, Johannes Kuhn wrote:
On 09-Dec-20 19:44, Mandy Chung wrote:
On 12/8/20 6:02 PM, Johannes Kuhn wrote:
There are a lot of things to consider when trying to fix JDK-8013527.
Exactly in particular security implication! What is clear is that the
expected lookup class should not be the injected class. The key
message here is that we can't fix JDK-8257874 until we fix JDK-8013527
unfortunately.
Mandy
Yeah, if JDK-8013527 is fixed it might fix JDK-8257874 as a byproduct.
If Lookup.lookup() can determine the original caller, then
Field.set*/Method.invoke could do the same.
Special care has to be taken that no other class could spoof such an
injected invoker.
Too complicated for me :). JDK-8013527 needs a sound design to approach
fixing it IMHO.
- Johannes
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v3]
On Mon, 11 Jan 2021 14:13:47 GMT, Alan Bateman wrote: > > > This issue requires a Reviewer from someone working in this area. Please do > not sponsor or integrate until that review has been done. Ok, increased the number of required reviewers to 2. Hope that was the right move, as I don't see any other way to undo /integrate. - PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v3]
On Mon, 11 Jan 2021 14:06:12 GMT, Attila Szegedi wrote: >> Johannes Kuhn has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add a missing pesky newline. > > Marked as reviewed by attila (Reviewer). Thanks @szegedi for reviewing this. - PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v3]
> Simple fix - one line change: > https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. > > Most of the changes come from an additional test that fails without this fix: > > Error: Unable to load main class somelib.test.TestMain in module somelib > java.lang.IllegalAccessError: superclass access check failed: class > somelib.test.TestMain (in module somelib) cannot access class > java.lang.Object (in module java.base) because module java.base does not > export java.lang to module somelib > > As described in JDK-8259395. > You can view the output of the test without patch here: > https://github.com/DasBrain/jdk/runs/1668078245 > > Thanks to @AlanBateman for pointing out the right fix. Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Add a missing pesky newline. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2000/files - new: https://git.openjdk.java.net/jdk/pull/2000/files/73ac7fde..7c2ed088 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2000=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2000=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000 PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base" [v2]
> Simple fix - one line change: > https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. > > Most of the changes come from an additional test that fails without this fix: > > Error: Unable to load main class somelib.test.TestMain in module somelib > java.lang.IllegalAccessError: superclass access check failed: class > somelib.test.TestMain (in module somelib) cannot access class > java.lang.Object (in module java.base) because module java.base does not > export java.lang to module somelib > > As described in JDK-8259395. > You can view the output of the test without patch here: > https://github.com/DasBrain/jdk/runs/1668078245 > > Thanks to @AlanBateman for pointing out the right fix. Johannes Kuhn has updated the pull request incrementally with one additional commit since the last revision: Move tests to test/jdk/tools/launcher/modules/patch/automatic. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2000/files - new: https://git.openjdk.java.net/jdk/pull/2000/files/bf715a8c..73ac7fde Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2000=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2000=00-01 Stats: 176 lines in 4 files changed: 114 ins; 61 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000 PR: https://git.openjdk.java.net/jdk/pull/2000
Re: RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base"
On Sun, 10 Jan 2021 15:14:31 GMT, Alan Bateman wrote: >> Simple fix - one line change: >> https://openjdk.github.io/cr/?repo=jdk=2000=00#sdiff-0. >> >> Most of the changes come from an additional test that fails without this fix: >> >> Error: Unable to load main class somelib.test.TestMain in module somelib >> java.lang.IllegalAccessError: superclass access check failed: class >> somelib.test.TestMain (in module somelib) cannot access class >> java.lang.Object (in module java.base) because module java.base does not >> export java.lang to module somelib >> >> As described in JDK-8259395. >> You can view the output of the test without patch here: >> https://github.com/DasBrain/jdk/runs/1668078245 >> >> Thanks to @AlanBateman for pointing out the right fix. > > test/jdk/jdk/modules/scenarios/automaticmodules/RunWithAutomaticModules.java > line 186: > >> 184: } >> 185: >> 186: /** > > The update to ModulePatcher is fine, that was an oversight that was missed > because it's a real corner to have an automatic module be the initial module > and patch it to add additional packages at the same time. > > The tests for --patch-module are in jdk/tools/launcher/modules/patch. I was > planning to add tests there for patching automatic modules when I created the > issue (I didn't know you were going to create a PR for it). Choosing the right place for the test was one of the harder parts. I did choose automaticmodules because it already has most of the necessary machinery already in place. But I will move the tests to jdk/tools/launcher/modules/patch. And making a PR was a shortcut reaction - "Hey, I could do this". - PR: https://git.openjdk.java.net/jdk/pull/2000
RFR: JDK-8259395 Patching automatic module with additional packages re-creates module without "requires java.base"
Simple fix, complex test case. Thanks to @AlanBateman for pointing out the right fix. - Commit messages: - * Minor cleanup. - * Fix Whitespace V2 - * Fix whitespace - * Fix JDK-8259395 - test now passes - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8259395 - * Add failing tests for JDK-8259395 Changes: https://git.openjdk.java.net/jdk/pull/2000/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2000=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8259395 Stats: 142 lines in 4 files changed: 140 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2000.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2000/head:pull/2000 PR: https://git.openjdk.java.net/jdk/pull/2000
Re: java.io.Console (was: Is SharedSecrets thread-safe?)
This brings up some stuff I wanted to mention for some time: * Console.cs is one of the fields projects like JRuby hack into (at least in the past). My guess is that they handle encodings in Ruby, and not using the Java facilities for that. The fact that it is also exported as shared secret for some unrelated stuff (sun.security.util) shows that there might be a need for a supported, public API. * java.io.Console has a nice API - but it only works (at least on Windows) if there is indeed a console attached. It usually doesn't work inside an IDE. Is there an interest to allow IDEs to create their own Console implementation, exposed through SPI? I would volunteer to write the spec and a patch, but there should be some general interest for this - as someone has to sponsor that. (And as a fair warning: It would be my first bigger contribution to OpenJDK.) - Johannes On 04-Jan-21 22:48, Peter Levart wrote: Hello 99206970363698485155, Thanks for these pointers. I would prefer to untangle those knots one at a time, if you don't mind, since some of them touch other parts of code while this change to SharedSecrets is pretty straightforward and localized. Regards, Peter On 12/31/20 6:25 PM, some-java-user-99206970363698485...@vodafonemail.de wrote: Hello Peter, the changes look good, however there might be more to consider: - `jdk.internal.access.SharedSecrets.getJavaLangRefAccess()` Might be good to add a comment there or for `java.lang.ref.Reference` that it is (hopefully?) initialized during JVM start-up. Similar to the comment for `AccessibleObject` which states that it is initialized in "initPhase1". Currently without special knowledge about JVM start-up, it is not obvious that this construct is safe. - `java.io.Console.cons` This is pretty brittle. It is currently only thread-safe because the only accessor is `System.console()` which has its own synchronization. However, I doubt that the author was aware that `Console.cons` on its own is not thread-safe. In general, why is there lazy initialization twice? Once in `System.console()` and then in the accessor for `Console.cons`. In my opinion *only* `java.io.Console` should have the lazy initialization logic (with proper synchronization!) and make sure that only one `Console` instance exists. - `jdk.internal.access.JavaIOAccess.charset()` The implementation of this one is extremely brittle. While it documents that the `Password` class calling it will make sure that `System.console()` has been called before, in that `Password` class there is not such notice, so it could easily happen that someone breaks this at a later point. Additionally the field `Console.cs` it accesses is not `final`, making it even more brittle. In my opinion there should be two changes: 1. Change `JavaIOAccess.charset()` -> `charset(Console)`, which then accesses the charset from that Console argument. This enforces that the user of the access already has the Console initialized and indirectly establishes the happens-before relationship for `Console.cs`. 2. The Console class should have the following fields `final`: readLock, writeLock, reader, out, pw, formatter, cs For `cs` this would merely require introducing a local variable. In general `sun.security.util.Password.convertToBytes(char[])` is problematic because it makes passwords unportable when one OS uses a different terminal encoding than another. The cleaner backward compatible solution might be to simply block all non-ASCII chars (i.e. throw exception for them)? This would break for all existing users which used non-ASCII chars or where their terminal has an encoding not based on ASCII, but these users might already have different problems due to the existing implementation. - `jdk.internal.access.SharedSecrets.setJavaAWTAccess(JavaAWTAccess)` Might have to establish a happens-before relationship for `getJavaAWTAccess()` because caller appears to expect a `JavaAWTAccess` in case respective class has been loaded. On a side note here: The implementation of that access appears to contain redundant code: https://github.com/openjdk/jdk/blob/8435f0daf2413a4c17578dd288e093fe006b3880/src/java.desktop/share/classes/sun/awt/AppContext.java#L866 The null check there makes no sense because `ecx` is always null at that point. - `jdk.internal.access.SharedSecrets.setJavaAWTFontAccess(JavaAWTFontAccess)` This seems to be rather brittle as well because its callers check whether the access has already been initialized. Additionally this seems to be more complicated than it has to be: It appears to be simpler to have `SharedSecrets` initialize the access by initializing only `NumericShaper` (or `TextAttribute`, ultimately does not matter which class from that package is used) when `getJavaAWTFontAccess()` is called. The
Re: RFR: 8198540: Dynalink leaks memory when generating type converters
On Sat, 2 Jan 2021 14:45:51 GMT, Attila Szegedi wrote: > _(NB: For this leak to occur, an application needs to be either creating and > discarding linkers frequently, or creating and discarding class loaders > frequently while creating Dynalink type converters for classes in them. > Neither is typical usage, although they can occur in dynamic code loading > environments such as OSGi.)_ > > I'll explain this one in a bit more detail. Dynalink's creates and caches > method handles for language-specific conversions between types. Each > conversion is thus characterized by a `Class` object representing the type > converted from, and a `Class` object representing the type converted to, thus > they're keyed by a pair of `(Class, Class)`. Java API provides the excellent > `ClassValue` class for associating values with a single class, but it lacks > the – admittedly more niche – facility for associating a value with a pair of > classes. I originally solved this problem using something that looks like a > `ClassValue, T>>`. I say "looks like" because Dynalink has a > specialized `ClassMap` class that works like `Map, T>` but it also > goes to some pains to _not_ establish strong references from a class loader > to either its children or to unrelated class loaders, for obvious reasons. > > Surprisingly, the problem didn't lie in there, though. The problem was in the > fact that `TypeConverterFactory` used `ClassValue` objects that were its > non-static anonymous inner classes, and further the values they computed were > also of non-static anonymous inner classes. Due to the way `ClassValue` > internals work, this led to the `TypeConverterFactory` objects becoming > anchored in a GC root of `Class.classValueMap` through the synthetic `this$0` > reference chains in said inner classes… talk about non-obvious memory leaks. > (I guess there's a lesson in there about not using `ClassValue` as an > instance field, but there's a genuine need for them here, so…) > > … so the first thing I did was I deconstructed all those anonymous inner > classes into named static inner classes, and ended up with something that > _did_ fix the memory leak (yay!) but at the same time there was a bunch of > copying of constructor parameters from outer classes into the inner classes, > the whole thing was very clunky with scary amounts of boilerplate. I felt > there must exist a better solution. > > And it exists! I ended up replacing the `ClassValue>` construct > with a ground-up implementation of `BiClassValue`, representation of > lazily computed values associated with a pair of types. This was the right > abstraction the `TypeConverterFactory` code needed all along. > `BiClassValue` is also not implemented as an abstract class but rather it > is a final class and takes a `BiFunction, Class, T>` for > computation of its values, thus there's never a risk of making it an > instance-specific inner class (well, you still need to be careful with the > bifunction lambda…) It also has an even better solution for avoiding strong > references to child class loaders. > > And that's why I'm not submitting here the smallest possible point fix to the > memory leak, but rather a slightly larger one that architecturally fixes it > the right way. > > There are two test classes, they test subtly different scenarios. > `TypeConverterFactoryMemoryLeakTest` tests that when a `TypeConverterFactory` > instance becomes unreachable, all method handles it created also become > eligible for collection. `TypeConverterFactoryRetentionTests` on the other > hand test that the factory itself won't prevent class loaders from being > collected by virtue of creating converters for them. Just a small suggestion using `MethodHandles.empty` instead of `MethodHandles.constant().asType().dropArguments()`. test/jdk/jdk/dynalink/TypeConverterFactoryRetentionTests.java line 65: > 63: // Never meant to be invoked, just a dummy MH that conforms > to the expected type. > 64: MethodHandle result = MethodHandles.constant(Object.class, > null).asType(MethodType.methodType(targetType)); > 65: result = MethodHandles.dropArguments(result, 0, sourceType); Suggestion: MethodHandle result = MethodHandles.empty(MethodType.methodType(targetType, sourceType)); test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java line 70: > 68: // Never meant to be invoked, just a dummy MH that conforms > to the expected type. > 69: MethodHandle result = MethodHandles.constant(Object.class, > null).asType(MethodType.methodType(targetType)); > 70: result = MethodHandles.dropArguments(result, 0, sourceType); Suggestion: MethodHandle result = MethodHandles.empty(MethodType.methodType(targetType, sourceType)); - PR: https://git.openjdk.java.net/jdk/pull/1918
Re: RFR: 8198540: Dynalink leaks memory when generating type converters
On Sat, 2 Jan 2021 14:45:51 GMT, Attila Szegedi wrote:
> _(NB: For this leak to occur, an application needs to be either creating and
> discarding linkers frequently, or creating and discarding class loaders
> frequently while creating Dynalink type converters for classes in them.
> Neither is typical usage, although they can occur in dynamic code loading
> environments such as OSGi.)_
>
> I'll explain this one in a bit more detail. Dynalink's creates and caches
> method handles for language-specific conversions between types. Each
> conversion is thus characterized by a `Class` object representing the type
> converted from, and a `Class` object representing the type converted to, thus
> they're keyed by a pair of `(Class, Class)`. Java API provides the excellent
> `ClassValue` class for associating values with a single class, but it lacks
> the – admittedly more niche – facility for associating a value with a pair of
> classes. I originally solved this problem using something that looks like a
> `ClassValue, T>>`. I say "looks like" because Dynalink has a
> specialized `ClassMap` class that works like `Map, T>` but it also
> goes to some pains to _not_ establish strong references from a class loader
> to either its children or to unrelated class loaders, for obvious reasons.
>
> Surprisingly, the problem didn't lie in there, though. The problem was in the
> fact that `TypeConverterFactory` used `ClassValue` objects that were its
> non-static anonymous inner classes, and further the values they computed were
> also of non-static anonymous inner classes. Due to the way `ClassValue`
> internals work, this led to the `TypeConverterFactory` objects becoming
> anchored in a GC root of `Class.classValueMap` through the synthetic `this$0`
> reference chains in said inner classes… talk about non-obvious memory leaks.
> (I guess there's a lesson in there about not using `ClassValue` as an
> instance field, but there's a genuine need for them here, so…)
>
> … so the first thing I did was I deconstructed all those anonymous inner
> classes into named static inner classes, and ended up with something that
> _did_ fix the memory leak (yay!) but at the same time there was a bunch of
> copying of constructor parameters from outer classes into the inner classes,
> the whole thing was very clunky with scary amounts of boilerplate. I felt
> there must exist a better solution.
>
> And it exists! I ended up replacing the `ClassValue>` construct
> with a ground-up implementation of `BiClassValue`, representation of
> lazily computed values associated with a pair of types. This was the right
> abstraction the `TypeConverterFactory` code needed all along.
> `BiClassValue` is also not implemented as an abstract class but rather it
> is a final class and takes a `BiFunction, Class, T>` for
> computation of its values, thus there's never a risk of making it an
> instance-specific inner class (well, you still need to be careful with the
> bifunction lambda…) It also has an even better solution for avoiding strong
> references to child class loaders.
>
> And that's why I'm not submitting here the smallest possible point fix to the
> memory leak, but rather a slightly larger one that architecturally fixes it
> the right way.
>
> There are two test classes, they test subtly different scenarios.
> `TypeConverterFactoryMemoryLeakTest` tests that when a `TypeConverterFactory`
> instance becomes unreachable, all method handles it created also become
> eligible for collection. `TypeConverterFactoryRetentionTests` on the other
> hand test that the factory itself won't prevent class loaders from being
> collected by virtue of creating converters for them.
test/jdk/jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java line 69:
> 67: public GuardedInvocation convertToType(Class sourceType,
> Class targetType, Supplier lookupSupplier) {
> 68: // Never meant to be invoked, just a dummy MH that conforms
> to the expected type.
> 69: MethodHandle result = MethodHandles.constant(Object.class,
> null).asType(MethodType.methodType(targetType));
Suggestion:
MethodHandle result = MethodHandles.zero(targetType);
test/jdk/jdk/dynalink/TypeConverterFactoryRetentionTests.java line 64:
> 62: public GuardedInvocation convertToType(Class sourceType,
> Class targetType, Supplier lookupSupplier) {
> 63: // Never meant to be invoked, just a dummy MH that conforms
> to the expected type.
> 64: MethodHandle result = MethodHandles.constant(Object.class,
> null).asType(MethodType.methodType(targetType));
Suggestion:
MethodHandle result = MethodHandles.zero(targetType);
-
PR: https://git.openjdk.java.net/jdk/pull/1918
Re: Is SharedSecrets thread-safe?
It's a bit more complicated - after all we are talking about the memory
model and class loading.
There are some shared secrets that can be loaded later (e.g. AWT), when
Threads are there.
The field is only set in one single place inside the JDK, so we are
talking about 3 scenarios:
* The class is not yet loaded - the field is null, call
ensureClassInitialized - which executes the class initializer on the
current thread. A subsequent read of the field must be visible - as
actions performed in the thread appear to be sequential for that thread.
* The class has been successfully loaded, everything relating to that is
visible.
* The class is currently initialized by an other thread:
* The first load of the field yields null - ensureClassInitialized
blocks until the class has been loaded.
* The first load yields something different than null.
The question now is: can the second load yield the previous value?
That is: can a field revert to it's original value?
Other fields may not be visible yet (except if they have been frozen).
So the big question is: can the second read return null after the
first has found it to be a non-null value?
After reading the "Double checked locking is broken"[1] declaration
again - it says that it does work for primitive values that can't be
teared. The problem seems to be that the field of the referenced objects
are not yet constructed.
The good news: the implementations of the shared secrets don't have
fields. The better news: reference assignment also can't be teared.
- Johannes
[1]: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
On 29-Dec-20 18:53, Hans Boehm wrote:
On Tue, Dec 29, 2020 at 5:56 AM Johannes Kuhn <mailto:i...@j-kuhn.de>> wrote:
>
> Depends on what `initialize()` is.
>
> If it (at least) reads a volatile field, then the compiler can't reorder
> the second read before the first.
>
> - Johannes
I disagree with this claim. I have no idea whether concurrency is
possible here, so it may not matter. See below.
>
> On 29-Dec-20 14:42,
some-java-user-99206970363698485...@vodafonemail.de
<mailto:some-java-user-99206970363698485...@vodafonemail.de>
> wrote:
> > Hello,
> > the class `jdk.internal.access.SharedSecrets` provides getter
methods which all look similar to this:
> > ```
> > if (static_field == null) {
> > initialize();
> > }
> > return static_field;
> > ```
If static_field is not volatile, and set concurrently, then the first
read of static_field may return non-null and the second null, without
initialize() even being executed. The Java memory model does not prevent
reordering of non-volatile reads from the same field (for good reason).
Even if initialize() is executed and performs a volatile read, this
reasoning doesn't hold. The initial static_field read may be delayed
past the volatile read inside the conditional and hence, at least
theoretically, past the second read. Control dependencies don't order
reads, either in Java, or in modern weakly-ordered architectures with
branch prediction. This doesn't matter if initialize() sets static_field.
This all assumes that having two threads call initialize() is OK.
Java code with data races is extremely tricky and rarely correct.
Hans
Re: Is SharedSecrets thread-safe?
Depends on what `initialize()` is.
If it (at least) reads a volatile field, then the compiler can't reorder
the second read before the first.
- Johannes
On 29-Dec-20 14:42, some-java-user-99206970363698485...@vodafonemail.de
wrote:
Hello,
the class `jdk.internal.access.SharedSecrets` provides getter methods which all
look similar to this:
```
if (static_field == null) {
initialize();
}
return static_field;
```
However, neither the static fields are `volatile` nor are the getter methods
synchronized. So if my
understanding of the Java Memory Model is correct, the compiler is free to
reorder the two static
field reads. So it is in theory possible that the first read yields a
non-`null` value, but the
second read yields a `null` value which leads to incorrect behavior (for
further reading [1]).
It is probably rather unlikely that this actually happens because
`SharedSecrets` is in most cases
accessed from static initializers (which are only run once) and because not
many classes access the
same `SharedSecrets` fields.
Is this analysis correct or did I forget to consider parts of the Memory Model
logic, or is there
some JVM magic I am missing?
Kind regards
[1]
https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-benign-is-resilient
Re: JDK-8229959/JDK-8242888 Prototype for j.l.r.Proxy with hidden classes & class data
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 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
JDK-8229959/JDK-8242888 Prototype for j.l.r.Proxy with hidden classes & class data
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
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
On 09-Dec-20 19:44, Mandy Chung wrote: On 12/8/20 6:02 PM, Johannes Kuhn wrote: There are a lot of things to consider when trying to fix JDK-8013527. Exactly in particular security implication! What is clear is that the expected lookup class should not be the injected class. The key message here is that we can't fix JDK-8257874 until we fix JDK-8013527 unfortunately. Mandy Yeah, if JDK-8013527 is fixed it might fix JDK-8257874 as a byproduct. If Lookup.lookup() can determine the original caller, then Field.set*/Method.invoke could do the same. Special care has to be taken that no other class could spoof such an injected invoker. Too complicated for me :). JDK-8013527 needs a sound design to approach fixing it IMHO. - Johannes
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
On 09-Dec-20 1:16, Mandy Chung wrote: On 12/8/20 5:07 AM, Johannes Kuhn wrote: Not sure if I read this correctly as "please share some example of code that tries to do that" or "please share code that you write to fix that". So I do both. I was curious to find out what old software attempts to change static final fields via reflection and why they do that. This leads to various unpredictable issues. Looks like you want to get old software to work and have to hack their static final fields!! Just do a quick search for "NoSuchFieldException modifiers". A lot of code does it, my impression is that often someone tried to be clever - just because they can. Setting static final fields does not work [1]. It probably never really did. It usually seems to work - but there is no guarantee that it actually does (like undefined behavior). Field::setXXX will throw IAE on static final fields and non-static fields declared on a record or hidden class too. The example works with Java 8 and 11 - it "tricks" the JIT into constant folding a static final field, so the subsequent change is not reflected. For the nestmate security consideration, the following invariants should already hold: * To call a @CallerSensitive method, the Lookup needs to have full privilege access (Lookup.hasFullPrivilegeAccess()) -> Injected Invokers are only created for full privilege lookups. * The injected invoker is in the same runtime package and has the same protection domain. * It is already possible to obtain a Lookup for the injected invoker by calling MethodHandles.lookup() through a MethodHandle (yikes) [5]. I am concerned with this. I am inclined to say we need to fix JDK-8013527 if we fix this issue. This means, we only have to consider what additional privileges the injected invoker gets if it is also a nestmate of the lookup class. I don't see any problem with that, as you already have a full privilege lookup when the injected invoker is created. - Johannes PS.: JDK-8013527 is mildly related - as the @CallerSensitive methods in java.lang.reflect are "hyper-sensitive". This fix will get JDK-8013527 passed the IAE but the lookup class of the resulting Lookup object is the hidden class which is still incorrect. Mandy A quick history about JDK-8013527 (and JDK-8207834 - which has the same root cause, but I wrote an comment on that): In Java 8, the injected invoker class had a name starting with "java.lang.invoke.". Creating a lookup for a class that starts with that name or binding the caller for such a class is explicitly blocked. In Java 9 the name of the injected invoker has changed - from this point on, the name of the injected invoker did depend on the package of the lookup class / caller. **It is already possible** to obtain a Lookup for the injected invoker by calling MethodHandles.lookup() through a MethodHandle. It doesn't throw an IAE anymore. **Since Java 9.** There are a lot of things to consider when trying to fix JDK-8013527. The first problem is: How do you determine the original class? It doesn't work with just using the nest host - as this might be different from the original class. Using the name might also not work - as the original class could be a hidden class itself. So, the only real solution is to attach that information (a reference to the lookup class) to the injected invoker. Then we have to detect that the target class is an injected invoker - and replace it with the actual lookup class. And this comes with some cost - either the job is done by Reflection.getCallerClass() - which would fix any other problem of that kind - or any hyper-sensitive @CallerSensitive method has to do this on it's own. (Luckily there are only a handful @CallerSensitive methods - and only a few are hyper-sensitive.) Unfortunately, I don't know enough about hotspot to determine if it is possible to change the "owner class" of some code. Also, garbage collection... I don't know enough about those systems. I only look at the stuff from the Java / bytecode side. In the end, I'm not sure if fixing l.lookupClass() == ((Lookup) l.findStatic(MethodHandles.class, "lookup", MethodType.methodType(Lookup.class)).invokeExact()).lookupClass() is worth that effort. - Johannes
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
On 08-Dec-20 5:40, Mandy Chung wrote: Thanks David. I was about to create one. This is indeed a gap in injecting this trampoline class for supporting @CallerSensitive methods. The InjectedInvoker ensures that the InjectedInvoker class has the same class loader as the lookup class. W.r.t. your patch, it seems okay but I have to consider and think through the security implication carefully. You mentioned "Some old software loves to set static final fields through reflection" - can you share some example libraries about this? This is really ugly hack!! Mandy Not sure if I read this correctly as "please share some example of code that tries to do that" or "please share code that you write to fix that". So I do both. Setting static final fields does not work [1]. It probably never really did. It usually seems to work - but there is no guarantee that it actually does (like undefined behavior). JPEXS [2] for example used that for it's configuration. Also some old Minecraft Forge version (a Minecraft mod loader / mod API) depends on this. Example use [3], but they do really ugly things. So, I said I develop agents to get old stuff running on a current Java version. Why? Fun, I guess. I also learn a lot a about what are the main comparability problems with newer Java versions. Pros of writing an agent: * I don't need the source code. * I don't need to setup a build environment with all dependencies, lombok and who knows what else is required. In all, it's faster for me. And I then have a list of problems - and how they can be solved. I did publish my JPESX agent here [4]. But yeah, it's an ugly hack. For the nestmate security consideration, the following invariants should already hold: * To call a @CallerSensitive method, the Lookup needs to have full privilege access (Lookup.hasFullPrivilegeAccess()) -> Injected Invokers are only created for full privilege lookups. * The injected invoker is in the same runtime package and has the same protection domain. * It is already possible to obtain a Lookup for the injected invoker by calling MethodHandles.lookup() through a MethodHandle (yikes) [5]. This means, we only have to consider what additional privileges the injected invoker gets if it is also a nestmate of the lookup class. I don't see any problem with that, as you already have a full privilege lookup when the injected invoker is created. - Johannes PS.: JDK-8013527 is mildly related - as the @CallerSensitive methods in java.lang.reflect are "hyper-sensitive". [1]: https://gist.github.com/DasBrain/25c6738610c517ee375aacc86ffebd0c [2]: https://github.com/akx/jpexs-decompiler/blob/6c998254b9d5bdce80be1b92d34836820ee31a1d/libsrc/ffdec_lib/src/com/jpexs/decompiler/flash/configuration/Configuration.java#L869 [3]: https://github.com/Chisel-Team/Chisel/blob/1.12/dev/src/main/java/team/chisel/common/init/ChiselBlocks.java#L18 [4]: https://github.com/DasBrain/jpex/tree/master/src/pw/dasbrain/jpexs/agent [5]: https://gist.github.com/DasBrain/142cb8ef9cc16e7469ac519790119e07
A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
Let's start with the reproducer:
public class NestmateBug {
private static int field = 0;
public static void main(String[] args) throws Throwable {
Lookup l = MethodHandles.lookup();
Field f = NestmateBug.class.getDeclaredField("field");
MethodHandle mh = l.findVirtual(Field.class, "setInt",
methodType(void.class, Object.class, int.class));
int newValue = 5;
mh.invokeExact(f, (Object) null, newValue);
}
}
This throws a IAE in the last line:
class test.se15.NestmateBug$$InjectedInvoker/0x000800bb5840 (in
module test.se15) cannot access a member of class test.se15.NestmateBug
(in module test.se15) with modifiers "private static"
at
java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:385)
at
java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:693)
at java.base/java.lang.reflect.Field.checkAccess(Field.java:1096)
at java.base/java.lang.reflect.Field.setInt(Field.java:976)
at test.se15/test.se15.NestmateBug.main(NestmateBug.java:17)
The reason for this behaviour is:
* Field.setInt (without setAccessible) requires that the caller class is
in the same nest for private members.
* Field.setInt is @CallerSensitive
* MethodHandles will bind to the caller by injecting an invoker for
@CallerSensitive methods.
* The injected invoker is NOT a nestmate of the original lookup class.
* The access check of Field.setInt fails - as the injected invoker is
now the caller.
This is important because:
* Some old software loves to set static final fields through reflection.
* To do that, it usually hacks into Field.modifiers. Which is filtered
iirc since Java 12.
* I write Java agents to fix such things - removing final from static
fields, and intercept Class.getDeclaredField and Field.setInt by
replacing it with an invokedynamic instruction.
* The original target is passed as a MethodHandle bootstrap argument. In
the case of Field.setInt, it is guarded with a
MethodHandles.guardWithTest() - calling the original target if the Field
is not my canary.
Suggested fix:
* Make the injected invoker a nestmate of lookup class.
I did prepare a fix for that [1], but AFAIK the bug first needs to be
filled in the bug tracker.
And I don't have an account.
- Johannes
[1]:
https://github.com/DasBrain/jdk/commit/3c4bb20c8e4cd9086e934128e5cb085a5cfbdb94
Can sun.misc.Unsafe.throwException() be removed?
From my understanding, Unsafe.throwException throws the passed
Throwable, more commonly known as "snaky throws".
There are other ways to archive the same result, one commonly found is
by (ab)using generics:
public static void sneakyThrows(Throwable t)
throws T {
throw (T) t;
}
(This will generate an unchecked warning, but that can be silenced with
the right annotation.)
Is there any reason why this method still needs to exist?
If not, could it be removed?
- Johannes
Re: RFR: 8247532: Records deserialization is slow
On 14-Jun-20 18:28, Peter Levart wrote: Hi, I noticed that deserializing records (new preview Java feature in JDK14 and JDK15) is slow compared to equivalent classical classes. I created a JMH benchmark [1] to se how it compares (ran it on JDK14): Benchmark Mode Cnt Score Error Units RecordSerializationBench.deserializeClasses avgt 10 31.911 ± 0.218 us/op RecordSerializationBench.deserializeClasses:·gc.alloc.rate avgt 10 815.106 ± 5.545 MB/sec RecordSerializationBench.deserializeClasses:·gc.alloc.rate.norm avgt 10 40921.903 ± 0.615 B/op RecordSerializationBench.deserializeClasses:·gc.churn.G1_Eden_Space avgt 10 839.522 ± 191.195 MB/sec RecordSerializationBench.deserializeClasses:·gc.churn.G1_Eden_Space.norm avgt 10 42153.661 ± 9682.799 B/op RecordSerializationBench.deserializeClasses:·gc.churn.G1_Survivor_Space avgt 10 0.117 ± 0.492 MB/sec RecordSerializationBench.deserializeClasses:·gc.churn.G1_Survivor_Space.norm avgt 10 5.835 ± 24.447 B/op RecordSerializationBench.deserializeClasses:·gc.count avgt 10 21.000 counts RecordSerializationBench.deserializeClasses:·gc.time avgt 10 17.000 ms RecordSerializationBench.deserializeRecords avgt 10 531.333 ± 3.094 us/op RecordSerializationBench.deserializeRecords:·gc.alloc.rate avgt 10 346.511 ± 1.997 MB/sec RecordSerializationBench.deserializeRecords:·gc.alloc.rate.norm avgt 10 289637.193 ± 6.894 B/op RecordSerializationBench.deserializeRecords:·gc.churn.G1_Eden_Space avgt 10 359.773 ± 191.116 MB/sec RecordSerializationBench.deserializeRecords:·gc.churn.G1_Eden_Space.norm avgt 10 300657.838 ± 159724.346 B/op RecordSerializationBench.deserializeRecords:·gc.churn.G1_Survivor_Space avgt 10 0.007 ± 0.012 MB/sec RecordSerializationBench.deserializeRecords:·gc.churn.G1_Survivor_Space.norm avgt 10 6.020 ± 9.910 B/op RecordSerializationBench.deserializeRecords:·gc.count avgt 10 9.000 counts RecordSerializationBench.deserializeRecords:·gc.time avgt 10 14.000 ms ...not only it is it about 16x slower, it also produces 7x garbage. I checked the code and it is not very optimal. It matches the record component names with object stream fields in O(n^2) way. It uses method handles but binds arguments of canonical constructor each time an instance of record is constructed. So I tried to optimize it [2] and with that patch on top of JDK16 the benchmark produces the following results: Benchmark Mode Cnt Score Error Units RecordSerializationBench.deserializeClasses avgt 10 31.049 ± 0.235 us/op RecordSerializationBench.deserializeClasses:·gc.alloc.rate avgt 10 837.614 ± 6.326 MB/sec RecordSerializationBench.deserializeClasses:·gc.alloc.rate.norm avgt 10 40921.931 ± 0.666 B/op RecordSerializationBench.deserializeClasses:·gc.churn.G1_Eden_Space avgt 10 867.743 ± 251.373 MB/sec RecordSerializationBench.deserializeClasses:·gc.churn.G1_Eden_Space.norm avgt 10 42405.532 ± 12403.301 B/op RecordSerializationBench.deserializeClasses:·gc.churn.G1_Survivor_Space avgt 10 0.126 ± 0.478 MB/sec RecordSerializationBench.deserializeClasses:·gc.churn.G1_Survivor_Space.norm avgt 10 6.113 ± 23.268 B/op RecordSerializationBench.deserializeClasses:·gc.count avgt 10 22.000 counts RecordSerializationBench.deserializeClasses:·gc.time avgt 10 19.000 ms RecordSerializationBench.deserializeRecords avgt 10 33.588 ± 0.394 us/op RecordSerializationBench.deserializeRecords:·gc.alloc.rate avgt 10 500.033 ± 5.871 MB/sec RecordSerializationBench.deserializeRecords:·gc.alloc.rate.norm avgt 10 26425.293 ± 0.759 B/op RecordSerializationBench.deserializeRecords:·gc.churn.G1_Eden_Space avgt 10 512.772 ± 288.112 MB/sec RecordSerializationBench.deserializeRecords:·gc.churn.G1_Eden_Space.norm avgt 10 27090.499 ± 15175.280 B/op RecordSerializationBench.deserializeRecords:·gc.churn.G1_Survivor_Space avgt 10 0.134 ± 0.496 MB/sec RecordSerializationBench.deserializeRecords:·gc.churn.G1_Survivor_Space.norm avgt 10 7.128 ± 26.526 B/op RecordSerializationBench.deserializeRecords:·gc.count avgt 10 13.000 counts RecordSerializationBench.deserializeRecords:·gc.time avgt 10 17.000 ms ...so here the speed is comparable and it even produces less garbage. I created an issue [3]. So WDYT? Since this is still a preview feature in JDK15, is it possible to squeeze it into JDK15? Regards, Peter [1] http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/RecordSerializationBench.java [2]
Re: JPackage with a modular application fails to load record class files even with --enable-preview
Relevant StackOverlow question: https://stackoverflow.com/questions/61504956/records-in-jlinked-application-throws-exception It talks about JLink, but the error message is the same, so I expect the same root cause: The ASM version shipped with JDK 14 doesn't support records yet. It is already fixed for JDK 15. - Johannes On 09-Jun-20 16:40, Andy Herrick wrote: So when you use the jpackage option "--java-options --enable-preview" it appears to work with a non-modular jar, but not with a modular jar ? I think I can create a testcase for that. Have you tried it with the jpackage from a preview release of JDK15 (https://jdk.java.net/15/) ? Can you look at the runtime in the app-image of the failing case, and see if the preview module is there ? Perhaps the problem is the way jpackage invokes jlink. A final thing you can try is run jlink yourself, and use the resulting runtime-image in jpackage args. /Andy On 6/7/2020 9:07 AM, Markus Jevring wrote: I have this app, which uses Java 14 with –enable-preview and records. If I turn it in to a normal non-modular jar, it works with both java -jar –enable-preview myapp.jar, and with a native image built with jpackage. However, when i turn it into a modular jar, I can still launch it with java –enable-preview –module-path ... –module ..., but when I try to create a native image using jpackage usign the same modular flags, launching it fails. I get a dialog box saying “Failed to launch JVM”, and if I enable –win-console when I build it and relaunch it, it tells me this: Exception in thread "main" java.lang.ClassFormatError: Invalid constant pool index 31 for name in Record attribute in class file net/jevring/frequencies/v2/input/KeyTimings$Note at java.base/java.lang.ClassLoader.defineClass2(Native Method) at java.base/java.lang.ClassLoader.defineClass(Unknown Source) at java.base/java.security.SecureClassLoader.defineClass(Unknown Source) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(Unknown Source) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassInModuleOrNull(Unknown Source) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(Unknown Source) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(Unknown Source) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(Unknown Source) at java.base/java.lang.ClassLoader.loadClass(Unknown Source) at frequencies/net.jevring.frequencies.v2.input.KeyTimings.(Unknown Source) at frequencies/net.jevring.frequencies.v2.Thief.(Unknown Source) at frequencies/net.jevring.frequencies.v2.Main.main(Unknown Source) Note, above, is a record. I tried moving it out to a separate class, and I still got the same error. When I converted Note to a normal class, it started complaining about the next record it cound. I know that it’s picking up the –enable-preview java option, because when I omit that, it complains about the class file version being wrong. Since I can run it just fine using “java -jar”, and it works fine under all other circumstances, I must conclude that there’s something wrong with the intersection of modular jars and jpackage. I’ve googled around plenty, and I haven’t found anything that might help me. I’m not sure if this is a call for help or a bug report, though I’d like to consider it the latter =) I put what I had on a branch: https://bitbucket.org/jevring/frequencies/branch/java-modules I realize it’s not a small self-contained example that replicates the issue, and for that I’m sorry. If someone is interested about this, I could try to make one.
JDK-8229959: Current status and thoughts about JDK-8242888
Hi,
half a year ago, I proposed a patch that implements JDK-8229959: Convert
proxy class to use constant dynamic [2].
**Background**: java.lang.reflect.Proxy creates a class file at runtime
that extends j.l.r.Proxy and implements the passed interfaces.
Methods invoked on that proxy are passed to a
j.l.r.InvocationHandler.invoke. One of it's argument is a j.l.r.Method.
This Method is currently cached in a private static final field in the
generated proxy class.
Remi Forax suggested to use a constant dynamic instead.
Last time I was not sure about the scope of my proposal, and when
discussion drifted to related topics, I did not know how to handle that.
After some thought, the scope of this is only changing the shape of the
generated class file. Those are NOT in the scope:
* Changes to a public API.
* Changes on how the generated class file is loaded.
Especially JDK-8242888: Convert dynamic proxy to hidden classes [3] is
not in scope. But see below for some thoughts on it.
-
To avoid changing the specification, I decided to put the bootstrap
method for the constant dynamic into the generated class file.
My first approach was a bootstrap method that is equivalent to this:
private static Method $proxy$bootstrap(Lookup l, String ignored,
Class jlrMethod, MethodHandle mh) {
return (Method) lookup.revealDirect(mh).reflectAs(jlrMethod);
}
One of the responses was to measure the impact, and I wrote some JMH
tests - which did run that code JITed.
It is much more likely that this code is executed in the interpreter
through.
Brian Goetz then suggested to write a bootstrap method per proxy mehod,
and calling Class::getMethod in them.
After some tries, I found a way to parameterize it, using MethodType as
carrier.
This avoids encoding the primitive types as yet an other constant dynamic.
Example bootstrap method:
private static Method $proxy$bootstrap(Lookup l, String name, Class
jlrMethod, Class owner, MethodType mt) {
return owner.getMethod(name, mt.parameterArray());
}
This approach seemed to work - until I did look into the interaction
with an installed SecurityManager.
Class::getMethod can throw a SecurityException if the method belongs to
an interface that is in a package not exported by the JDK.
In particular, it breaks j.l.i.MethodHandleProxies.wrapperInstanceTarget
when a SecurityManager is installed. [4]
(A separate change could be made to fix that particular case, but I
consider it just to be a symptom.)
This breaks because Class::getMethod is called at a later point - when
untrusted code is on the stack.
--
Where can this go from here?
* Wrap the call to Class::getMethod in an AccessController.doPrivileged
block.
While possible, this would add even more overhead.
* Go back to revealDirect/reflectAs.
This does not work because Lookup::revealDirect will check the
package access.
* Use hidden classes, as suggested by JDK-8242888.
A promising approach, but has a few unsolved problems.
What about hidden classes?
It's possible to pass some ClassData to a hidden class when defining it.
Which can be used to pass the Methods.
But Hidden Classes need some more requirements, as outlined by Mandy in
JDK-8242888:
* Serialization: No change necessary.
Proxy classes (as determined by Proxy.isProxyClass) are written as a
proxy descriptor, containing a list of all implemented interfaces.
Deserialization is fine if Proxy.h can be updated even if the
instance is implemented by a hidden class.
* A lookup in the same package as the proxy is needed.
This is a problem for not-yet-existing packages. It's possible to
inject a "package-witness" or shim.
* Protection domain: Proxy is defined to have a null ProtectionDomain.
This is currently necessary for Class.getMethod.
If the methods are passed using ClassData, then that requirement is
no longer needed.
An other option is to grant the package witness a null ProtectionDomain.
As conclusion, JDK-8229959 can only be implemented if it either uses
AccessController.doPrivileged or use hidden classes with ClassData.
- Johannes
[1]: https://bugs.openjdk.java.net/browse/JDK-8229959
[2]:
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-November/063595.html
[3]: https://bugs.openjdk.java.net/browse/JDK-8242888
[4]: https://gist.github.com/DasBrain/37855876743fed7828c7690603aa3039
Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version
Thanks.
Noted that readInt can read outside of the byte array when it is shorter
than 4 bytes, throwing an ArrayIndexOutOfBoundsException.
Same applies for readUnsignedShort.
Test cases for malformedClassFiles:
new byte[3], new byte[] {0xCA, 0xFE, 0xBA, 0xBE, 0x00}
- Johannes
On 29-May-20 21:23, Mandy Chung wrote:
It's fixed. Thanks
Mandy
On 5/28/20 4:35 PM, Johannes Kuhn wrote:
Hi,
just noticed a small thing:
The magic is 4 bytes, but readUnsignedShort reads two bytes.
- Johannes
On 28-May-20 19:25, Mandy Chung wrote:
On 5/28/20 6:55 AM, Alan Bateman wrote:
On 28/05/2020 05:13, Mandy Chung wrote:
Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/
I modify this patch to check the class file version and throws CFE
if unsupported before creating ClassReader. This also fixes
JDK-8245061 that it reads the value of `this_class` as a constant
(as ASM will throw an exception if it's not a constant) and also
ensures that `this_class` is a CONSTANT_Class_info by checking the
descriptor string from Type.
I don't want to complicate this further but the --enable-preview
handling in the VM doesn't seem to expose an internal property that
allows code in the libraries know if preview feature are enabled or
not. I was assuming that isSupportedClassFileVersion would need to
check that.
The runtime will ensure if --enable-preview is set if a class file
with minor is loaded. I will prefer to keep
VM::isSupportedClassFileVersion to validate the given major/minor
version. At runtime, it will fail when such class file is loaded if
preview is not enabled.
I'll add a comment to describe that.
Will readUnsignedShort throw AIOBE if the offset is beyond the length?
Good catch. I add the check to throw CFE.
+ private static int readUnsignedShort(byte[] bytes, int
offset) {
+ if (offset >= bytes.length) {
+ throw new ClassFormatError("Invalid ClassFile
structure");
+ }
+ return ((bytes[offset] & 0xFF) << 8) |
(bytes[offset + 1] & 0xFF);
+ }
Also are you sure about "& 0xCAFEBABE", I assumed it would just
check the magic number is equal to that.
It should check ==. Fixed.
thanks
Mandy
Re: RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version
Hi,
just noticed a small thing:
The magic is 4 bytes, but readUnsignedShort reads two bytes.
- Johannes
On 28-May-20 19:25, Mandy Chung wrote:
On 5/28/20 6:55 AM, Alan Bateman wrote:
On 28/05/2020 05:13, Mandy Chung wrote:
Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/
I modify this patch to check the class file version and throws CFE
if unsupported before creating ClassReader. This also fixes
JDK-8245061 that it reads the value of `this_class` as a constant
(as ASM will throw an exception if it's not a constant) and also
ensures that `this_class` is a CONSTANT_Class_info by checking the
descriptor string from Type.
I don't want to complicate this further but the --enable-preview
handling in the VM doesn't seem to expose an internal property that
allows code in the libraries know if preview feature are enabled or
not. I was assuming that isSupportedClassFileVersion would need to
check that.
The runtime will ensure if --enable-preview is set if a class file
with minor is loaded. I will prefer to keep
VM::isSupportedClassFileVersion to validate the given major/minor
version. At runtime, it will fail when such class file is loaded if
preview is not enabled.
I'll add a comment to describe that.
Will readUnsignedShort throw AIOBE if the offset is beyond the length?
Good catch. I add the check to throw CFE.
+ private static int readUnsignedShort(byte[] bytes, int
offset) {
+ if (offset >= bytes.length) {
+ throw new ClassFormatError("Invalid ClassFile
structure");
+ }
+ return ((bytes[offset] & 0xFF) << 8) | (bytes[offset
+ 1] & 0xFF);
+ }
Also are you sure about "& 0xCAFEBABE", I assumed it would just check
the magic number is equal to that.
It should check ==. Fixed.
thanks
Mandy
Mismatch between spec and implementation of DataInputStream.readFully(byte[], int, int)
Found a small mismatch between the specification of
DataInputStream.readFully(byte[] b, int off, int len) and its
implementation.
In particular, it doesn't throw an IndexOutOfBoundsException if offset
is negative and len is 0.
Reproducer below.
I suggest changing the specification of this method to allow that behavior.
This change should also affect DataInput, where care has to be taken
that both behaviors are legal.
- Johannes
import java.io.ByteArrayInputStream;
import java.io.DataInputStream;
import java.io.IOException;
public class DISBug {
public static void main(String[] args) throws IOException {
try (var bais = new ByteArrayInputStream(new byte[0]);
var dis = new DataInputStream(bais)) {
// From the spec:
// throws IndexOutOfBoundsException - if off is negative,
len is negative, or len is greater than b.length - off.
byte[] b = new byte[0];
int off = -1; // This is not valid
int len = 0;
dis.readFully(b, off, len);
throw new AssertionError("readFully did not throw");
} catch (IndexOutOfBoundsException expected) {
// Ignore, this exception is expected
}
}
}
Re: JDK-8226810: An other case and a small change suggestion
On 11-May-20 10:59, Alan Bateman wrote:
On 10/05/2020 15:47, Johannes Kuhn wrote:
:
After the discussion with Naoto, I would like to change the one line
to strcpy(ret + 2, "1252")
diff --git a/src/java.base/windows/native/libjava/java_props_md.c
b/src/java.base/windows/native/libjava/java_props_md.c
--- a/src/java.base/windows/native/libjava/java_props_md.c
+++ b/src/java.base/windows/native/libjava/java_props_md.c
@@ -73,6 +73,7 @@
LOCALE_IDEFAULTANSICODEPAGE,
ret+2, 14) == 0) {
codepage = 1252;
+ strcpy(ret + 2, "1252");
} else {
codepage = atoi(ret+2);
}
I have already signed the OCA.
This would be my first patch. Is there anything else you need?
I will take a further look into GetLocaleInfo, and try to find a
model that works for me (currently thinking about blackbox, can
return anything from [1]),
and then discussing how to handle all those cases.
It's not clear how conditions are necessary for GetLocaleInfo to fail
but good to fix this code path. We should probably create a new issue
in JIRA for this as it's not clear that it is the same issue as
JDK-8226810. We've been chasing JDK-8226810 for some time but have not
been able to find a system where it duplicates. The reports so far
suggests is Windows Chinese but most of the people that report the
issue to not respond to questions so we can't be sure. One of the
reports (see my comment in the bug from June 2019) suggests the code
page is 54936, useful information as GB18030 is not in java.base. Once
your fix is in then I think we should try to improve the exception
message rather than NPE. There may be some of these cases where it
should default to UTF-8.
-Alan.
I agree.
Looking through the JDK code, the IMHO best way to create a hook for
this is Charset.defaultCharset().
Instead of using GetPropertyAction, it could retrieve
System.getProperties and do a null check there. This has probably the
least impact on performance as this code path is only executed once.
Indeed, it could (theoretically) emit a warning/debug message there with
debug information and return sun.nio.cs.UTF_8.INSTANCE as fallback
without setting defaultCharset.
But that is a hard problem. I don't really like adding a warning to java
startup, even if it would not work before just to get debug information.
On the other hand, just continuing as if nothing did happen could hide
further bugs. (Also, how would I get the debug info from the Java side,
as properties are not yet initialized.)
I wrote a small C program which just outputs the result of
GetLocaleInfo(GetSystemDefaultLCID(), LOCALE_IDEFAULTANSICODEPAGE, ...).
I was not able to influence that result - on my machine it's always 1252.
My current understanding of codepages is basically: They are like encodings.
They come in two variants (?): ANSI and non-ANSI. The ansi codepages use
ANSI for bytes between 0 and 127.
Each system, application or thread (wtf? see CP_THREAD_ACP) can have a
different current codepage.
Some windows API functions (the *A variants) use the current codepage
(as returned by GetACP) to translate the string from/to unicode.
GetACP returns the codepage identifier for the operating system. (No
mention of thread in the doc for that, so why is there even CP_THREAD_ACP?)
It's a rabbit hole.
If someone knows about a way to influence the result of
GetLocaleInfo(GetSystemDefaultLCID(), LOCALE_IDEFAULTANSICODEPAGE, ...),
then at least reproducing the error would become possible.
- Johannes
Re: JDK-8226810: An other case and a small change suggestion
On 09-May-20 1:27, Brent Christian wrote:
On 5/7/20 5:50 AM, Alan Bateman wrote:
On 07/05/2020 12:37, Johannes Kuhn wrote:
:
In the end, I don't know what causes the bug, or how I can replicate
it.
I think I did find a likely suspect.
>
Good sleuthing. I don't what the conditions are for GetLocaleInfo to
fail but it does look like that would return possibly non-terminated
garbage starting with "CP" so we should at least fix that.
I agree. I can file a bug and sponsor this small change.
-Brent
Thank you.
After the discussion with Naoto, I would like to change the one line to
strcpy(ret + 2, "1252")
diff --git a/src/java.base/windows/native/libjava/java_props_md.c
b/src/java.base/windows/native/libjava/java_props_md.c
--- a/src/java.base/windows/native/libjava/java_props_md.c
+++ b/src/java.base/windows/native/libjava/java_props_md.c
@@ -73,6 +73,7 @@
LOCALE_IDEFAULTANSICODEPAGE,
ret+2, 14) == 0) {
codepage = 1252;
+ strcpy(ret + 2, "1252");
} else {
codepage = atoi(ret+2);
}
I have already signed the OCA.
This would be my first patch. Is there anything else you need?
I will take a further look into GetLocaleInfo, and try to find a model
that works for me (currently thinking about blackbox, can return
anything from [1]),
and then discussing how to handle all those cases.
- Johannes
[1]:
https://docs.microsoft.com/en-us/windows/win32/intl/code-page-identifiers
Re: JDK-8226810: An other case and a small change suggestion
Thanks. I think strcpy(ret+2, "1252") vs. strcpy(ret, "Cp1252") is a just matter of style. I would prefer the later, as it makes the intent clear. But not my call. Do you have any info how I can change the detected codepage there? I wrote a small C program that basically just does that part and printf it. In my limited tests (windows likes to require a restart after each configuration change) I did not find a way to influence that. An other thing to consider is if Cp65001 should be treated as UTF-8 in that function? (As said before, locale is not my expertise. Can that function with that LCSID even return 65001?) I can see how things go wrong if it returns 65001 as locale, so... could be a safe change? (I'm sure that things break if that function returns 65001.) Then there is the other part: The mismatch between the comment in jni_util.c/newSizedStringJava and the implementation on the Java side. There is no fallback to iso-8859-1. If new String(byte[]) is called before the system properties are installed, then this will lead to a NullPointerException. And there is a code path that leads to exactly that - newPlatformString is called from the initialization of the properties. [1] And if the encoding returned by the windows function is not supported, then it will call new String(byte[]) - during system property initialization. - Johannes [1]: https://hg.openjdk.java.net/jdk/jdk/file/d40d865753fb/src/java.base/share/native/libjava/System.c#l207 On 08-May-20 18:27, naoto.s...@oracle.com wrote: Ditto. Good catch! I am not sure the fix would address the issue in 8226810 (cannot confirm it either, as my Windows box is at my office where I cannot enter at the moment :-), but this definitely looks like a bug. I would change the additional line to "strcpy(ret+2, "1252");" as Cp is copied in the following switch. Naoto On 5/7/20 5:50 AM, Alan Bateman wrote: On 07/05/2020 12:37, Johannes Kuhn wrote: : In the end, I don't know what causes the bug, or how I can replicate it. I think I did find a likely suspect. Good sleuthing. I don't what the conditions are for GetLocaleInfo to fail but it does look like that would return possibly non-terminated garbage starting with "CP" so we should at least fix that. The issue in JDK-8226810 might be something else. One of the submitters to that issue did engage and provided enough information to learn that the locale is zh_CN and also reported that it was failing for GB18030. GB18030 is not in java.base so that at least explained that report. -Alan
JDK-8226810: An other case and a small change suggestion
I just saw a question on StackOverflow [1] which reports to have the
same issue as in JDK-8226810 [2] - with the same stack trace.
While I could not reproduce the issue, I tried to understand which code
path could lead to that outcome.
While doing so I did come across getEncodingInternal in java_props_md.c [3]
The path when GetLocaleInfo in line 72 fails just sets the codepage to 1252.
The following switch doesn't have a branch for codepage 1252, so the
default branch is used.
The first two elements of the char array are set to "Cp", while the rest
of that char array is still uninitialized.
This (partially) uninitialized array is then returned.
This seems wrong.
I would therefore either change codepage to 0 (resulting in a default
encoding of UTF-8) or also copy the "Cp1252" to the char buffer.
(More when I follow this code path):
In InitializeEncoding [4] this invalid encoding is then set to
jnuEncoding, and fastEncoding to NO_FAST_ENCODING.
A call to JNU_NewStringPlatform will then result in a call to
newSizedStringJava [5], where jnuEncodingSupported is called.
This calls java.nio.charset.Charset.isSupported(jnuEncoding) and caches
the result - I assume it returns false. Or throws.
This means new String(byte[]) is called.
Here comes the puzzler: java.lang.StringCoding.decode(byte[],int,int)
calls Charset.getDefaultCharset() as first action.
- and does not fall back to iso-8859-1 as the comment in
newSizedStringJava suggests.
Charset.getDefaultCharset() then tries to read the properties - which
are currently in the process to be initialized - and fails with a
NullPointerException.
In the end, I don't know what causes the bug, or how I can replicate it.
I think I did find a likely suspect.
I therefore would like to propose the following patch:
diff --git a/src/java.base/windows/native/libjava/java_props_md.c
b/src/java.base/windows/native/libjava/java_props_md.c
--- a/src/java.base/windows/native/libjava/java_props_md.c
+++ b/src/java.base/windows/native/libjava/java_props_md.c
@@ -73,6 +73,7 @@
LOCALE_IDEFAULTANSICODEPAGE,
ret+2, 14) == 0) {
codepage = 1252;
+ strcpy(ret, "Cp1252");
} else {
codepage = atoi(ret+2);
}
Because this path is not taken on my system, I can't test if this change
affects anything. Tests run as before.
But I still think that it is The Right Thing To Do.
Maybe logging the GetLastError too - but it's very early in the VM's
startup.
I did consider calling GetLocaleInfo twice - with NULL as buffer and 0
as length - which returns the required size of the buffer.
But that would have some implication on the startup time.
I could also check GetLastError and try again with a bigger buffer - but
then the question arises: Can the VM handle such a charset?
Johannes
[1]:
https://stackoverflow.com/questions/61650372/jdk-14-0-1-error-occurred-during-initialization-of-vm-java-lang-nullpointerexcep
[2]: https://bugs.openjdk.java.net/browse/JDK-8226810
[3]:
https://hg.openjdk.java.net/jdk/jdk/file/d40d865753fb/src/java.base/windows/native/libjava/java_props_md.c#l64
[4]:
https://hg.openjdk.java.net/jdk/jdk/file/d40d865753fb/src/java.base/share/native/libjava/jni_util.c#l773
[5]:
https://hg.openjdk.java.net/jdk/jdk/file/d40d865753fb/src/java.base/share/native/libjava/jni_util.c#l661
Re: Some Classes with a public void close() don't implement AutoCloseable
updated the code, similar output. Disregard org/w3c/dom/html/HTMLDocument
https://gist.github.com/DasBrain/df25a69acf564dcb5f2241cc065be2ed
About java.xml.stream: A XMLStreamReader is for example constructed with:
BufferedReader br = ...;
XMLStreamReader xsr =
XMLInputFactory.newDefaultFactory().createXMLEventReader(br);
For my IDE this would look like XMLStreamReader wraps the
BufferedReader, so it won't issue a warning if I don't close br.
But the Javadoc for XMLStreamReader.close() states:
> Frees any resources associated with this Reader. This method does not
close the underlying input source.
If those classes would implement AutoCloseable, people and IDEs might
wrongly assume that it would close the BufferedReader.
Which would result in code like this:
try (var xsr =
XMLInputFactory.newDefaultFactory().createXMLEventReader(Files.newBufferedReader(path,
StandardCharsets.UTF_8))) { ... }
And have a resource leak. Incidentally, that is the similar to the code
I tried to write when I saw that XMLStreamReader has a public void
close() method.
So in my opinion, to let XMLStreamReader implement AutoCloseable, the
specification of the close() method should be changed.
The same applies for the other XML Stream/Event Reader/Writer.
Thanks,
Johannes
On 16-Apr-20 9:28, Alan Bateman wrote:
On 16/04/2020 01:28, Johannes Kuhn wrote:
:
I did not restrict my enumeration to public and exported types - it
was easier not to do that and I'm lazy.
Most of the candidates that you've identified are JDK internal classes
and there are several non-public classes in exported packages. There
are also a few cases where the sub-classes of the likely candidate are
showing up too (e.g. all the public sub-classes of
java.util.logging.Handler). I skimmed through your list and filtered
it down to the public classes in exported packages to following:
com/sun/jdi/connect/spi/Connection
java/awt/SplashScreen
java/util/logging/Handler
javax/naming/Context
javax/naming/NamingEnumeration
javax/naming/ldap/StartTlsResponse
javax/smartcardio/CardChannel
javax/sql/PooledConnection
javax/swing/ProgressMonitor
javax/xml/stream/XMLEventReader
javax/xml/stream/XMLEventWriter
javax/xml/stream/XMLStreamReader
javax/xml/stream/XMLStreamWriter
As Joe points out, some of these may have been looked at already. I
was surprised to see the java.xml.stream reader/writers so it might be
worth digging into those to see why they were not retrofitted.
-Alan.
Re: Some Classes with a public void close() don't implement AutoCloseable
Hi Joe, thanks for all the info. As I'm not yet familiar with the way how OpenJDK is developed, mind if I ask what steps would have to be taken to add Closeable to javax.naming.Context? The first thing would be to create a patch, test it, run the tests. Then the change has to be proposed, and reviewed. You can argue that this might waste the time of the reviewers with such a small patch. Is there something else that would need to be done? I have read http://openjdk.java.net/contribute/ and signed the OCA, but on the other hand, it's such a small change that most already know how the patch will look. I am not even sure if my time would be spend wisely. The motivation would be to do it to learn how to contribute. Anyway, thank you for your time and consideration. This just some stuff I found, then wrote some code, got some results and did not know what I can or should do with that. Thanks, Johannes On 16-Apr-20 3:15, Joe Darcy wrote: Hi Johannes, FYI, back during Project Coin during JDK 7, there were various systematic efforts to review types with a "close" method in the JDK and retrofit AutoCloseable where appropriate: JDK-6911261: Project Coin: Retrofit Automatic Resource Management (ARM) support onto platform APIs https://bugs.openjdk.java.net/browse/JDK-6911261 JDK-6963723: Project Coin: Retrofit more JDK classes for ARM https://bugs.openjdk.java.net/browse/JDK-6963723 If you'll excuse the bad import into another blogging system, the process of finding the candidate types with an annotation processor and doing the retrofitting was discussed in a blog entry from that time: https://blogs.oracle.com/darcy/project-coin:-bringing-it-to-a-closeable More recently fixed in JDK 14, JDK-8203036: com.sun.net.httpserver.HttpExchange should implement AutoCloseable https://bugs.openjdk.java.net/browse/JDK-8203036 and as of August 2019, closed as will not fix given lack of interest: JDK-7057061: Support autocloseable for javax.naming.Context https://bugs.openjdk.java.net/browse/JDK-7057061 Thanks, -Joe On 4/15/2020 5:28 PM, Johannes Kuhn wrote: After failing to wrap a XMLStreamReader in a try-with-resources after discovering it's close() method, I thought about checking what other classes have a public void close() method in the JDK but don't implement AutoCloseable. So I wrote a small program that enumerates all classes present in in the jrt image with a public void close() method that does not directly or indirectly implement AutoCloseable: https://gist.github.com/DasBrain/8d50e02e35654870e2c2c00bf3f79954 (Output & Code, Code requires JDK 14 with preview features and ASM) As I am not an expert in any of those areas where classes showed up, and I don't know if it was an oversight, or if there is a good reason for not implementing AutoCloseable. Implementing AutoCloseable is a double edged sword, a few IDEs warn about resource leaks based on that interface. And treat an AutoCloseable passed to methods who return an AutoCloseable as the return value wraping the parameter. I did look at a few classes: * javax.xml.stream.XMLStreamReader:Javadoc: "Frees any resources associated with this Reader. This method does not close the underlying input source." Better not implement it? * javax.naming.Context: Javadoc: "This method releases this context's resources immediately, instead of waiting for them to be released automatically by the garbage collector." Implement AutoCloseable? * java.util.logging.Handler: Lifecycle seems to be managed by a LogManager. Better not implement AutoCloseable? * jdk.internal.jrtfs.ExplodedImage: class is package private, overrides close() from SystemImage while increasing visibility to public. I did not restrict my enumeration to public and exported types - it was easier not to do that and I'm lazy. This touches many unrelated areas, but David Holmes suggested that I take it to core-libs-dev. I know, I just did some static code analysis, but it seems that there might be some low hanging fruits. And for public classes I wish to have at least some rationale why a class with a public void close() method doesn't implement AutoCloseable. Thanks, Johannes
Some Classes with a public void close() don't implement AutoCloseable
After failing to wrap a XMLStreamReader in a try-with-resources after discovering it's close() method, I thought about checking what other classes have a public void close() method in the JDK but don't implement AutoCloseable. So I wrote a small program that enumerates all classes present in in the jrt image with a public void close() method that does not directly or indirectly implement AutoCloseable: https://gist.github.com/DasBrain/8d50e02e35654870e2c2c00bf3f79954 (Output & Code, Code requires JDK 14 with preview features and ASM) As I am not an expert in any of those areas where classes showed up, and I don't know if it was an oversight, or if there is a good reason for not implementing AutoCloseable. Implementing AutoCloseable is a double edged sword, a few IDEs warn about resource leaks based on that interface. And treat an AutoCloseable passed to methods who return an AutoCloseable as the return value wraping the parameter. I did look at a few classes: * javax.xml.stream.XMLStreamReader:Javadoc: "Frees any resources associated with this Reader. This method does not close the underlying input source." Better not implement it? * javax.naming.Context: Javadoc: "This method releases this context's resources immediately, instead of waiting for them to be released automatically by the garbage collector." Implement AutoCloseable? * java.util.logging.Handler: Lifecycle seems to be managed by a LogManager. Better not implement AutoCloseable? * jdk.internal.jrtfs.ExplodedImage: class is package private, overrides close() from SystemImage while increasing visibility to public. I did not restrict my enumeration to public and exported types - it was easier not to do that and I'm lazy. This touches many unrelated areas, but David Holmes suggested that I take it to core-libs-dev. I know, I just did some static code analysis, but it seems that there might be some low hanging fruits. And for public classes I wish to have at least some rationale why a class with a public void close() method doesn't implement AutoCloseable. Thanks, Johannes Classes found: com/sun/jdi/connect/spi/Connection com/sun/jndi/dns/BaseNameClassPairEnumeration com/sun/jndi/dns/DnsClient com/sun/jndi/dns/DnsContext com/sun/jndi/dns/Resolver com/sun/jndi/ldap/AbstractLdapNamingEnumeration com/sun/jndi/ldap/LdapCtx com/sun/jndi/ldap/LdapReferralContext com/sun/jndi/ldap/LdapSchemaCtx com/sun/jndi/ldap/ext/StartTlsResponseImpl com/sun/jndi/rmi/registry/BindingEnumeration com/sun/jndi/rmi/registry/NameClassPairEnumeration com/sun/jndi/rmi/registry/RegistryContext com/sun/jndi/toolkit/dir/ContextEnumerator com/sun/jndi/toolkit/dir/HierMemDirCtx com/sun/jndi/toolkit/dir/HierMemDirCtx$BaseFlatNames com/sun/jndi/toolkit/dir/LazySearchEnumerationImpl com/sun/jndi/toolkit/url/GenericURLContext com/sun/media/sound/AudioFloatFormatConverter$AudioFloatInputStreamChannelMixer com/sun/media/sound/AudioFloatFormatConverter$AudioFloatInputStreamResampler com/sun/media/sound/AudioFloatInputStream com/sun/media/sound/AudioFloatInputStream$BytaArrayAudioFloatInputStream com/sun/media/sound/AudioFloatInputStream$DirectAudioFloatInputStream com/sun/media/sound/ModelAbstractOscillator com/sun/media/sound/ModelDirector com/sun/media/sound/ModelOscillatorStream com/sun/media/sound/ModelStandardDirector com/sun/media/sound/ModelStandardIndexedDirector com/sun/media/sound/RIFFWriter$RandomAccessByteWriter com/sun/media/sound/RIFFWriter$RandomAccessFileWriter com/sun/media/sound/RIFFWriter$RandomAccessWriter com/sun/media/sound/SoftAbstractResampler$ModelAbstractResamplerStream com/sun/media/sound/SoftMainMixer com/sun/media/sound/SoftMixingDataLine$AudioFloatInputStreamResampler com/sun/media/sound/SoftMixingMainMixer com/sun/media/sound/SoftMixingSourceDataLine$NonBlockingFloatInputStream com/sun/naming/internal/VersionHelper$InputStreamEnumeration com/sun/org/apache/xerces/internal/impl/XMLStreamFilterImpl com/sun/org/apache/xerces/internal/impl/XMLStreamReaderImpl com/sun/org/apache/xerces/internal/xinclude/XIncludeTextReader com/sun/org/apache/xml/internal/serializer/EmptySerializer com/sun/org/apache/xml/internal/serializer/SerializationHandler com/sun/org/apache/xml/internal/serializer/SerializerBase com/sun/org/apache/xml/internal/serializer/ToHTMLSAXHandler com/sun/org/apache/xml/internal/serializer/ToUnknownStream com/sun/org/apache/xml/internal/serializer/WriterChain com/sun/tools/javac/api/JavacTaskPool$ReusableContext$ReusableJavaCompiler com/sun/tools/javac/file/JavacFileManager$1 com/sun/tools/javac/file/JavacFileManager$ArchiveContainer com/sun/tools/javac/file/JavacFileManager$Container com/sun/tools/javac/file/JavacFileManager$DirectoryContainer com/sun/tools/javac/file/JavacFileManager$JRTImageContainer com/sun/tools/javac/file/Locations com/sun/tools/javac/main/JavaCompiler com/sun/tools/javac/processing/JavacProcessingEnvironment$DiscoveredProcessors
Re: [PATCH] Simplification proposal regarding TYPE-field initialization in primitive wrappers
Hi,
I saw Remi's reply after I sent my reply.
The questions I asked were just to confirm that my suspicion (int.class gets
compiled into Integer.TYPE) were true - I am currently on mobile, so I couldn't
test it myself.
In short, my mail doesn't provide any additional value, and if I had seen
Remi's reply, I wouldn't have sent mine.
Best regards,
Johannes
On November 26, 2019 10:10:02 PM GMT+01:00, "Сергей Цыпанов"
wrote:
>Hello,
>
>> I suspect that it will print null, because to my knowledge, int.class
>will be compiled to a getstatic Integer.TYPE, so your test basically
>tests if Integer.TYPE == Integer.TYPE.
>
>right, Remi pointed this out in his response.
>
>> Also, do the tier1 tests succeed with your patch?
>
>I'm going to do that as far as my work station is fixed, hope tomorrow.
>Apparently, something is going to go wrong :) Anyway, I'll notify about
>results when this is done.
>
>Regards,
>Sergey Tsypanov
>
>
>26.11.2019, 21:57, "Johannes Kuhn" :
>> On November 26, 2019 8:29:06 PM GMT+01:00, "Сергей Цыпанов"
> wrote:
>>> Hello,
>>>
>>> while using java.lang.Integer.TYPE I've found out that currently
>>> wrapper classes for primitives initialize their own TYPE-fields by
>>> calling native method java.lang.Class.getPrimitiveClass().
>>>
>>> This can be simplified by changing existing declaration (here for
>>> java.lang.Integer)
>>>
>>> @SuppressWarnings("unchecked")
>>> public static final Class TYPE = (Class)
>>> Class.getPrimitiveClass("int");
>>>
>>> to
>>>
>>> public static final Class TYPE = int.class;
>>>
>>> This is likely to improve start-up time as Class.getPrimitiveClass()
>is
>>> called 9 times (8 primitive type wrappers + java.lang.Void), after
>the
>>> change this method is not called at all and thus can be removed
>along
>>> with its backing C++ code on VM-side. The fields are initialized
>purely
>>> on Java side.
>>>
>>> I've verified correctness of the substitution with this test run on
>JDK
>>> 11
>>>
>>> @Test
>>> void name() {
>>> assert void.class == Void.TYPE;
>>> assert boolean.class == Boolean.TYPE;
>>> assert byte.class == Byte.TYPE;
>>> assert char.class == Character.TYPE;
>>> assert short.class == Short.TYPE;
>>> assert int.class == Integer.TYPE;
>>> assert long.class == Long.TYPE;
>>> assert float.class == Float.TYPE;
>>> assert double.class == Double.TYPE;
>>> }
>>>
>>> The patch is attached.
>>>
>>> Regards,
>>> Sergey Tsypanov
>>
>> Hi,
>> a few questions:
>> What does System.out.println(int.class); with your patch print? "int"
>or "null"?
>>
>> I suspect that it will print null, because to my knowledge, int.class
>will be compiled to a getstatic Integer.TYPE, so your test basically
>tests if Integer.TYPE == Integer.TYPE.
>>
>> Also, do the tier1 tests succeed with your patch?
>>
>> With best regards,
>> Johannes
Re: New candidate JEP: 370: Foreign-Memory Access API
Hi, Just had the chance to look at the proposal, and I have a small suggestion: MemorySegmenent::mapFromPath should state that it can throw a SecurityException if a SecurityManager is installed in the javadoc. The description can be copied from FileChannel::open. With best regards, Johannes
Re: [PATCH] Simplification proposal regarding TYPE-field initialization in primitive wrappers
On November 26, 2019 8:29:06 PM GMT+01:00, "Сергей Цыпанов"
wrote:
>Hello,
>
>while using java.lang.Integer.TYPE I've found out that currently
>wrapper classes for primitives initialize their own TYPE-fields by
>calling native method java.lang.Class.getPrimitiveClass().
>
>This can be simplified by changing existing declaration (here for
>java.lang.Integer)
>
>@SuppressWarnings("unchecked")
>public static final Class TYPE = (Class)
>Class.getPrimitiveClass("int");
>
>to
>
>public static final Class TYPE = int.class;
>
>This is likely to improve start-up time as Class.getPrimitiveClass() is
>called 9 times (8 primitive type wrappers + java.lang.Void), after the
>change this method is not called at all and thus can be removed along
>with its backing C++ code on VM-side. The fields are initialized purely
>on Java side.
>
>I've verified correctness of the substitution with this test run on JDK
>11
>
>@Test
>void name() {
> assert void.class == Void.TYPE;
> assert boolean.class == Boolean.TYPE;
> assert byte.class == Byte.TYPE;
> assert char.class == Character.TYPE;
> assert short.class == Short.TYPE;
> assert int.class == Integer.TYPE;
> assert long.class == Long.TYPE;
> assert float.class == Float.TYPE;
> assert double.class == Double.TYPE;
>}
>
>The patch is attached.
>
>Regards,
>Sergey Tsypanov
Hi,
a few questions:
What does System.out.println(int.class); with your patch print? "int" or "null"?
I suspect that it will print null, because to my knowledge, int.class will be
compiled to a getstatic Integer.TYPE, so your test basically tests if
Integer.TYPE == Integer.TYPE.
Also, do the tier1 tests succeed with your patch?
With best regards,
Johannes
Re: Looking for Sponsor: JDK-8229959 Convert proxy class to use constant dynamic
On 11/23/2019 10:40 PM, Brian Goetz wrote:
Finally, we can benchmark the current approach against the LDC
approach on a per-Method basis. The LDC approach may well be doing
more work per Method, so it's a tradeoff to determine whether
deferring that work is a win.
By this last bit, I mean JMH'ing:
Method m1() {
return Class.forName("java.lang.Object").getMethod("equals",
new Class[] { Class.forName("java.lang.Object") });
}
vs
Method m2() {
return bootstrap(... constant bootstrap args for above ...)
}
Thanks for the pointers, I did run one round on my machine - I don't
have a dedicated one - so, maybe treat it with a fine grain of salt.
The test code and results can be found here:
https://gist.github.com/DasBrain/501fd5ac1e2ade2e28347ec666299473
Benchmark Mode Cnt Score Error Units
ProxyFindMethod.getMethod thrpt 25 1505594.515 ±
42238.663 ops/s
ProxyFindMethod.lookupMethod thrpt 25 760530.602 ±
25074.003 ops/s
ProxyFindMethod.lookupMethodCachedMH thrpt 25 4327814.284 ±
103456.616 ops/s
I wrote three tests, the first one (getMethod) uses the
Class.forName().getMethod(), the second uses lookup.findVirtual to get
the MethodHandle every iteration, while the third one caches it in a
static final field.
My interpretation: If the method has to be looked up (and the VM has to
do that at least once), then the old getMethod call is faster.
I also suspect the Class.forName call to be optimized - on some old java
versions, iirc pre-1.5, Foo.class was compiled into
Class.forName("Foo"), so the JIT might do something there.
So, now I have to learn how to read assembly. Good.
Re: Looking for Sponsor: JDK-8229959 Convert proxy class to use constant dynamic
On 11/23/2019 6:09 PM, Brian Goetz wrote: Thanks for the examples. A few comments: - A reasonable place to consider putting the bootstrap is in Proxy itself. I am not sure that ConstantBootstraps is the right place (or anywhere in JLI for that matter) unless Method is retrofitted to implement Constable, which is problematic given that Methods are not constants. Yes, Method implementing Constable might not be the right thing, because Method is not immutable. Adding the bootstrap method to j.l.r.Proxy is a good alternative. - In the department of “future things that might be applied here” (Remi started it), this is a perfect application for lazy statics. But we don’t have those yet. What Remi and Mandy suggested is to pass the Method instances that we already use to spin the bytecode to the new class, which avoids an other round of method lookups. While I like the idea, it relies on on future things. I will try to write a prototype for that. - The main benefit here is that it makes proxy creation faster. However, given the amount of work that goes on at proxy creation (reflection, byte code spinning, class loading), its not clear how much of the cost is initializing the proxy class. Some benchmarking to establish the cost benefit here would be good. (If we are spinning the proxy at jlink time, then the benefit is probably greater.) Overall, the translation approach seems sound but I would suggest that we clarify the benefits a little bit before embarking. I just finished the first prototype, which can be found at https://gist.github.com/DasBrain/7766bedfbc8b76ba6a0ee66a54ba97ed - it contains a patch, the javap output of a generated proxy class with and without that patch, and the code I used to dump the proxy class. "make run-test-tier1" passes all JDK tests with the patch. I hope this will help the discussion a bit. The other benefit I talked about is: It makes to code easier, because it's less code and it's a greater locality. I know, this is hard to measure. Everything was spawned by a small sentence Remi wrote on this list: A follow up should to use constant dynamic (introduce in Java 11) to get the j.l.r.Method object instead of pre-calculating all of them in the static init block. The idea is that a ldc on a constant dynamic with bootstrap method that takes a MethodHandle as parameter (as boostrap argument) can return the corresponding Method by using Lookup.revealDirect + reflectAs. The bootstrap method can be added to j.l.i.ConstantBootstraps https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-August/061923.html But there were never a real discussion about the benefits yet. It's good that we have one now. As I am a novice JMH user, I don't trust myself to write good benchmarks that actually measure what I want to measure, not confirm what I would like it to be
Re: Looking for Sponsor: JDK-8229959 Convert proxy class to use constant dynamic
On 22.11.2019 22:41, Remi Forax wrote: i wonder if some codes in the wild rely on that ? I don't think some code does, but you never know. There is perhaps a better way, as part of the branch nestmates of valhalla, Mandy has added a way to transfer a live object to a Class when defining it, with that you can send all the Methods as an array and in the bootstrap method of a condy, access to the right method by extracting it from the array using an index. It will avoid the whole serialization/deserialization of Methods dance. That is a better idea - and could be done today using Unsafe.defineAnonymousClass - If it didn't need a host class. Here is an example: https://gist.github.com/forax/d0f0034190bc479b86ce977fb94ca176 I believe those changes are planned for Java 14 so will be integrated very soon. It looks like the class is defined using Lookup.defineHiddenClassWithClassData - in the current proxy implementation there is no lookup object. Also, I'm not sure if the defined proxy class needs to be findable. In short: I did not do an evaluation on what unspecified parts of a proxy can be changed without breaking someones cod
Re: Looking for Sponsor: JDK-8229959 Convert proxy class to use constant dynamic
Last mail got converted from HTML into plaintext, which dropped some
significant whit espace. So, again, doing it by hand this time.
On 22.11.2019 20:52, Brian Goetz wrote:
> For those that are not intimately familiar with the mechanics of
proxy class generation, it would be great to flesh this out with a code
example, that shows the proposed old and new translation. This would
make it easier for folks to evaluate the benefits and costs of the
approach, and possibly suggest improvements.
>
> On 11/21/2019 10:23 PM, Johannes Kuhn wrote:
>>
>> * What benefits would such a change have?
>> First, it allows us to drop the static final fields and the class
initializer for the generated proxy classes.
>> Second, it allows the code (both generated and generator) to be more
clear by moving the essential parts together.
>> Third, it might bring some performance improvement, because the
j.l.r.Method lookup is delayed to it's first use - which in some cases
might never happen.
>
Thanks for the suggestion. A proxy can be used to create an
implementation of one or more interfaces at run time.
I just let Java 13 dump a generated Proxy and decompiled it using JD.
This is the result:
package com.sun.proxy;
// imports ...;
public final class $Proxy0 extends Proxy implements Runnable {
private static Method m1;
private static Method m3;
private static Method m2;
private static Method m0;
public $Proxy0(InvocationHandler paramInvocationHandler) {
super(paramInvocationHandler); }
public final boolean equals(Object paramObject) {
try {
return ((Boolean)this.h.invoke(this, m1, new Object[] {
paramObject })).booleanValue();
} catch (Error|RuntimeException error) {
throw error;
} catch (Throwable throwable) {
throw new UndeclaredThrowableException(throwable);
}
}
public final void run() {
try {
this.h.invoke(this, m3, null);
return;
} catch (Error|RuntimeException error) {
throw error;
} catch (Throwable throwable) {
throw new UndeclaredThrowableException(throwable);
}
}
public final String toString() {
try {
return (String)this.h.invoke(this, m2, null);
} catch (Error|RuntimeException error) {
throw error;
} catch (Throwable throwable) {
throw new UndeclaredThrowableException(throwable);
}
}
public final int hashCode() {
try {
return ((Integer)this.h.invoke(this, m0, null)).intValue();
} catch (Error|RuntimeException error) {
throw error;
} catch (Throwable throwable) {
throw new UndeclaredThrowableException(throwable);
}
}
static {
try {
m1 = Class.forName("java.lang.Object").getMethod("equals", new
Class[] { Class.forName("java.lang.Object") });
m3 = Class.forName("java.lang.Runnable").getMethod("run", new
Class[0]);
m2 = Class.forName("java.lang.Object").getMethod("toString", new
Class[0]);
m0 = Class.forName("java.lang.Object").getMethod("hashCode", new
Class[0]);
return;
} catch (NoSuchMethodException noSuchMethodException) {
throw new NoSuchMethodError(noSuchMethodException.getMessage());
} catch (ClassNotFoundException classNotFoundException) {
throw new NoClassDefFoundError(classNotFoundException.getMessage());
}
}
}
The new implementation using ASM generates a similar class.
I intend to replace the the references to m0, m1... in the generated
code through an LDC instruction, and remove the code that generates the
fields and the static class initializer.
The resulting code would look a something like this:
package com.sun.proxy;
// imports ...;
public final class $Proxy0 extends Proxy implements Runnable {
public $Proxy0(InvocationHandler paramInvocationHandler) {
super(paramInvocationHandler); }
public final boolean equals(Object paramObject) {
try {
return ((Boolean)this.h.invoke(this, __LDC(__MethodHandle
(com.sun.proxy.$Proxy0::$bootstap, "_", Method.class,
__MethodHandle(java.lang.Object::equals)), new Object[] { paramObject
})).booleanValue();
} catch (Error|RuntimeException error) {
throw error;
} catch (Throwable throwable) {
throw new UndeclaredThrowableException(throwable);
}
}
// repeat for run(), hashCode() and equals(Object other)
private static final Method $bootstrap(Lookup lookup, String ignored,
Class clazz, MethodHandle mh) {
return lookup.revealDirect(mh).reflectAs(clazz, lookup);
}
}
I used __LDC as placeholder for the LDC bytecode instruction (limited to
constant dynamic), and __MethodHandle for a MethodHandle in the constant
pool.
This means the Method is only looked up when the proxy method is called
for the first time.
In the actual source code (the generating code), it means that the
following methods can be dropped:
P
Re: Looking for Sponsor: JDK-8229959 Convert proxy class to use constant dynamic
On 22.11.2019 20:52, Brian Goetz wrote:
For those that are not intimately familiar with the mechanics of proxy
class generation, it would be great to flesh this out with a code
example, that shows the proposed old and new translation. This would
make it easier for folks to evaluate the benefits and costs of the
approach, and possibly suggest improvements.
On 11/21/2019 10:23 PM, Johannes Kuhn wrote:
* What benefits would such a change have?
First, it allows us to drop the static final fields and the class
initializer for the generated proxy classes.
Second, it allows the code (both generated and generator) to be more
clear by moving the essential parts together.
Third, it might bring some performance improvement, because the
j.l.r.Method lookup is delayed to it's first use - which in some
cases might never happen.
Thanks for the suggestion. A proxy can be used to create an
implementation of one or more interfaces at run time.
I just let Java 13 dump a generated Proxy and decompiled it using JD.
This is the result:
packagecom.sun.proxy;
//imports...;
publicfinalclass$Proxy0extendsProxyimplementsRunnable{
privatestaticMethodm1;
privatestaticMethodm3;
privatestaticMethodm2;
privatestaticMethodm0;
public$Proxy0(InvocationHandlerparamInvocationHandler){super(paramInvocationHandler);}
publicfinalbooleanequals(ObjectparamObject){
try{
return((Boolean)this.h.invoke(this,m1,newObject[]{paramObject})).booleanValue();
}catch(Error|RuntimeExceptionerror){
throwerror;
}catch(Throwablethrowable){
thrownewUndeclaredThrowableException(throwable);
}
}
publicfinalvoidrun(){
try{
this.h.invoke(this,m3,null);
return;
}catch(Error|RuntimeExceptionerror){
throwerror;
}catch(Throwablethrowable){
thrownewUndeclaredThrowableException(throwable);
}
}
publicfinalStringtoString(){
try{
return(String)this.h.invoke(this,m2,null);
}catch(Error|RuntimeExceptionerror){
throwerror;
}catch(Throwablethrowable){
thrownewUndeclaredThrowableException(throwable);
}
}
publicfinalinthashCode(){
try{
return((Integer)this.h.invoke(this,m0,null)).intValue();
}catch(Error|RuntimeExceptionerror){
throwerror;
}catch(Throwablethrowable){
thrownewUndeclaredThrowableException(throwable);
}
}
static{
try{
m1=Class.forName("java.lang.Object").getMethod("equals",newClass[]{Class.forName("java.lang.Object")});
m3=Class.forName("java.lang.Runnable").getMethod("run",newClass[0]);
m2=Class.forName("java.lang.Object").getMethod("toString",newClass[0]);
m0=Class.forName("java.lang.Object").getMethod("hashCode",newClass[0]);
return;
}catch(NoSuchMethodExceptionnoSuchMethodException){
thrownewNoSuchMethodError(noSuchMethodException.getMessage());
}catch(ClassNotFoundExceptionclassNotFoundException){
thrownewNoClassDefFoundError(classNotFoundException.getMessage());
}
}
}
The new implementation using ASM generates a similar class.
I intend to replace the the references to m0, m1... in the generated
code through an LDC instruction, and remove the code that generates the
fields and the static class initializer.
The resulting code would look a something like this:
packagecom.sun.proxy;
//imports...;
publicfinalclass$Proxy0extendsProxyimplementsRunnable{
public$Proxy0(InvocationHandlerparamInvocationHandler){super(paramInvocationHandler);}
publicfinalbooleanequals(ObjectparamObject){
try{
return((Boolean)this.h.invoke(this,__LDC(__MethodHandle
(com.sun.proxy.$Proxy0::$bootstap, "_", Method.class,
__MethodHandle(java.lang.Object::equals)),newObject[]{paramObject})).booleanValue();
}catch(Error|RuntimeExceptionerror){
throwerror;
}catch(Throwablethrowable){
thrownewUndeclaredThrowableException(throwable);
}
}
// repeat for run(), hashCode() and equals(Object other)
private static final Method $bootstrap(Lookup lookup, String ignored,
Class clazz, MethodHandle mh) { return
lookup.revealDirect(mh).reflectAs(clazz, lookup); }
}
I used __LDC as placeholder for the LDC bytecode instruction (limited to
constant dynamic), and __MethodHandle for a MethodHandle in the constant
pool.
This means the Method is only looked up when the proxy method is called
for the first time.
In the actual source code (the generating code), it means that the
following methods can be dropped:
ProxyGenerator.generateStaticInitializer()
https://hg.openjdk.java.net/jdk/jdk/file/a2a921609481/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#l576
ProxyGenerator.ProxyMethod.codeFieldInitialization(MethodVisitor,
String)
https://hg.openjdk.java.net/jdk/jdk/file/a2a921609481/src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java#l576
It will require the addition of a new method, which creates the
$bootstrap method, and a change
ProxyGenerator.ProxyMethod.generateMethod to emit an LDC instruction
instead of a GETSTATIC.
The next step is to remove the now unused ProxyMethod.methodFieldName
and it's dependencies (constructor, th
Looking for Sponsor: JDK-8229959 Convert proxy class to use constant dynamic
Hi everyone, a few hours ago I signed the Oracle Contributor Agreement. I'd like to work on JDK-8229959: Convert proxy class to use constant dynamic. Without further ado, here are some questions and answers you may ask: * What are you exactly planing to do? I'd like to replace the GETSTATIC instruction with an LDC instruction in the current generated bytecode for the proxies. * What benefits would such a change have? First, it allows us to drop the static final fields and the class initializer for the generated proxy classes. Second, it allows the code (both generated and generator) to be more clear by moving the essential parts together. Third, it might bring some performance improvement, because the j.l.r.Method lookup is delayed to it's first use - which in some cases might never happen. * Are you able to write such a patch? Yes. I have some experience with the Java bytecode, ASM and ConstantDynamic. * But java.lang.reflect.Method is not immutable/persistent? That's not a question, but you are right. j.l.r.Method has setAccessible, modifying the Method instance. Still, the current implementation shares the same Method instance with all proxies with the same interfaces in the same ClassLoader. * What should the bootstrap method look like? The bootstrap method will take one additional argument, a direct MethodHandle to the to be resolved method, and should crack it using it's passed Lookup and then reflect it as Method. In other words, it should do `return lookup.revealDirect(mh).reflectAs(Method.class, lookup);` * Where should this bootstrap method reside? I'm glad you asked. After some thoughts, there are two promising places where I could put it: a) As private static method in the proxy itself. Benefits: No public API changes. Also allows easy migration to option b) if pursued later. Downsides: The bootstrap method will be duplicated into every proxy class. b) As a public static method in the java.* namespace, preferably in java.lang.invoke.ConstantBootstraps. Benefits: Only one single implementation in the JVM. Downside: Changes to the public API. Also leads to other questions like "Should j.l.r.Member implement Constable?" and "Should I add a subclass of j.l.c.DynamicConstantDesc for this bootstrap method?" I prefer option a), but I'm fine with both options. Please let me know what you think, or if you think of a better way. * What do I expect from my Sponsor? I expect my sponsor lo believe that this change will improve the OpenJDK. If consensus shows a preference for option b) above (bootstrap in j.l.i.ConstantBootstaps), a guidance what requests have to be made and where. In that case, I volunteer to write those requests and send them to my sponsor, who can (at their sole discretion) forward it to the relevant places. If you have any additional questions, please don't hesitate to answer to this mail. I will try to answer all questions to the best of my ability. With best regards, Johannes Kuhn
Re: RFR JDK-8233527: Update Lookup::hasPrivateAccess and Lookup::defineClass spec w.r.t. full power lookup
On 13.11.2019 20:07, Mandy Chung wrote:
Thank you for all those changes. It fixed two of my reported bugs
(JDK-8209005, JDK-8209078).
Thanks for filing these good reports. JDK-8173978 resolved the
issues reported by JDK-8209005 and JDK-8209078.
I'm happy I could help.
It also makes my suggested workaround for JDK-8230636 not longer
possible. Thanks for picking that up too.
I just have a question about the requirement of
MethodHandles.privateLookupIn: As it requires that the originating
lookup has both PRIVATE and MODULE access, it sounds a lot like full
privilege access. Should this be reworded?
The two bullets about the caller lookup object must have MODULE and
PRIVATE are important explanation for it to require such access.
Maybe I can add a bullet to say "The caller lookup object must have
full privilege access" and then move those two bullets as sub-bullets
under it.
What do you think?
This is a good idea, because full privilege access is a new concept, and
should be mentioned at the places where it is used.
Also, with the current specification, I don't see a way to abuse
jdk.unsupported's privileged access into java.base (which IMHO caused
the entire rework).
Thanks for looking at this.
Mandy
Shameless plug: I once asked on Stackoverflow if untrusted code running
with a SecurityManager and -illegal-access=deny but granting
|ReflectPermission("supressAccessChecks") |could escape the sandbox[1].
I found a way to escape the sandbox, which relies on the power of the
Lookup returned by privateLookupIn.
As this has been changed, I wonder if this escape was ever intended in
the first place. Are there any resources out there that can help answer
this question?
Thanks,
Johannes
[1]:
https://stackoverflow.com/questions/51712903/is-it-safe-to-grant-untrusted-code-supressaccesschecks-when-illegal-access-de
Re: RFR JDK-8233527: Update Lookup::hasPrivateAccess and Lookup::defineClass spec w.r.t. full power lookup
On 11.11.2019 22:23, Mandy Chung wrote: This is a follow-up of JDK-8226916. Lookup::hasPrivateAccess intends to test if this lookup is a full-power lookup; that is created by the original caller class calling MethodHandles::lookup. The current specification for Lookup::hasPrivateAccess returns true if the lookup modes contain PRIVATE but it does not check MODULE bit. This patch also clarifies the capabilities w.r.t PRIVATE access and full-power access. The security permission check is performed if the Lookup is not a full power lookup and therefore Lookup::defineClass spec should be updated to perform security permission check if the security manager is present and this lookup refuses access, consistent with other Lookup operations. http://cr.openjdk.java.net/~mchung/jdk14/8233527/webrev.02/ http://cr.openjdk.java.net/~mchung/jdk14/8233527/specdiff/ CSR: https://bugs.openjdk.java.net/browse/JDK-8233726 thanks Mandy Thank you for all those changes. It fixed two of my reported bugs (JDK-8209005, JDK-8209078). It also makes my suggested workaround for JDK-8230636 not longer possible. Thanks for picking that up too. I just have a question about the requirement of MethodHandles.privateLookupIn: As it requires that the originating lookup has both PRIVATE and MODULE access, it sounds a lot like full privilege access. Should this be reworded? Also, with the current specification, I don't see a way to abuse jdk.unsupported's privileged access into java.base (which IMHO caused the entire rework). thanks, Johannes
Re: [JDK-8209078] Unable to call default interface methods from named module
Hi,
thanks for your quick reply.
On 16/09/2018 11:15, Alan Bateman wrote:
> On 14/09/2018 20:34, Johannes Kuhn wrote:
>> :
>>
>> 1. Use Case:
>>
>> The most obvious use case is to to "implement" default methods in
>> instances created by j.l.r.Proxy. An implementation of
>> j.l.r.InvocationHandler that handles default methods could look like
>> this:
>>
>> public Object invoke(Object proxy, Method method, Object[] args)
>> throws Throwable {
>> if (method.isDefault()) {
>> MethodType mt = methodType(method.getReturnType(),
>> method.getParameterTypes());
>> Class declaringClass = method.getDeclaringClass();
>> Lookup l = MethodHandles.lookup();
>> MethodHandle mh = l.findSpecial(declaringClass,
>> method.getName(), mt, declaringClass)
>> .asFixedArity()
>> .bindTo(proxy);
>> return mh.invokeWithArguments(args);
>> }
>> // Handle the other methods
>> return null;
>> }
>>
>> Such a feature is very valuable so the InvocationHandler does not break
>> if an interface is evolved by adding new default methods.
> The common case will be a public interface in an exported package in
> which case the proxy will be generated in the unnamed module of the
> specified class loader. This means you can create a full-power lookup on
> the proxy class with:
>
> Class proxyClass = proxy.getClass();
> this.getClass().getModule().addReads(proxyClass.getModule());
> Lookup l = MethodHandles.privateLookupIn(proxyClass,
> MethodHandles.lookup());
>
> If you invoke findSpecial on this lookup then I would expect it should
> work.
Thank you, this works fine. Unless you don't have
j.l.r.ReflectPermission("suppressAccessChecks").
I try to avoid requiring additional permissions if possible, and code in
an unnamed module does not need privateLookupIn.
Also j.l.r.ReflectPermission("suppressAccessChecks") is considered a
dangerous permission that allows a full sandbox escape.
>
> There can be cases where the proxy is encapsulated (because the
> interfaces are not public or not in exported packages) but I assume you
> don't need to be concerned with those for now.
>
Right, I am not concerned about that now.
>> :
>>
>>
>> 3. Cause of the discrepancy between lookup class in named module vs
>> unnamed module
>>
>> As Mandy Chung observed:
>>
>>> IAE is thrown because the implementation calls Lookup::in teleporting
>>> to a Lookup of the special class which lost all access since the
>>> caller class and the requested lookup class are in different modules.
>> This behavior is documented in the Lookup.in() [6]:
>>
>>> * If the old lookup class is in a named module, and the new lookup
>>> class is in a different module M, then no members, not even public
>>> members in M's exported packages, will be accessible. The exception to
>>> this is when this lookup is publicLookup, in which case PUBLIC access
>>> is not lost.
>>> * If the old lookup class is in an unnamed module, and the new lookup
>>> class is a different module then MODULE access is lost.
>> This strikes me as a little bit odd, so I looked around for the
>> reasoning for this difference, and found an old discussion on jigsaw-dev
>> [7]. I especially found this mail from Alan Bateman [8] interesting,
>> where he gave a reasoning for this:
>>
>>> Preserved but perhaps with the (initially surprising) consequence that
>>> all access is lost when m(LC) is a named module and m(A) is a
>>> different module. This arises because the two modules may read very
>>> different sets of modules, the intersection cannot be expressed via a
>>> lookup class + modes.
>> While I agree that intersection might not be expressed via lookup class
>> + modes, I don't think that it is necessary to express that. Instead
>> don't allow any lookup on members in a different module if the MODULE
>> flag is not set.
>>
> Qualified exports complicated this, as does readability. JDK-8173978 [1]
> tracks the need to support checking the intersection.
>
I also had some thoughts about what semantics Lookup.in() can have.
A Lookup for class A can only be created in 3 ways:
* MethodHandles.lookup() called from A - full access
* MethodHandles.privateLookupIn() - full access
* Lookup.in() - drops modes depending on the distance.
The reasoning for Lookup.in() should always be:
** If a mode bit is set, then the o
[JDK-8209078] Unable to call default interface methods from named module
Hi everyone,
I recently reported JDK-8209078 "Unable to call default method from
interface in another module from named module" [1].
It is possible to call any default interface method directly using
MethodHandles.lookup().findSpecial(Interf.class, "defaultMethodName",
..., Intef.class).invoke(...);
This only works if the caller is in an unnamed module or Interf is in
the same module as the caller.
I now want to share some thoughts about this feature, and an idea how it
could be fixed, but this probably requires further discussion.
1. Use Case:
The most obvious use case is to to "implement" default methods in
instances created by j.l.r.Proxy. An implementation of
j.l.r.InvocationHandler that handles default methods could look like this:
public Object invoke(Object proxy, Method method, Object[] args)
throws Throwable {
if (method.isDefault()) {
MethodType mt = methodType(method.getReturnType(),
method.getParameterTypes());
Class declaringClass = method.getDeclaringClass();
Lookup l = MethodHandles.lookup();
MethodHandle mh = l.findSpecial(declaringClass,
method.getName(), mt, declaringClass)
.asFixedArity()
.bindTo(proxy);
return mh.invokeWithArguments(args);
}
// Handle the other methods
return null;
}
Such a feature is very valuable so the InvocationHandler does not break
if an interface is evolved by adding new default methods.
2. Documentation:
When I faced the problem above, I did a search on how to invoke default
methods on a proxy.
The above code was inspired by this mail [2] from John Rose, where he
mentions JDK-8130227 "JEP 274: Enhanced Method Handles" [3][4].
This is the only official documentation I could find on this feature.
Indeed, even the Javadoc for findSpecial [5] states that my code above
should not successfully run:
> Before method resolution, if the explicitly specified caller class is
> not identical with the lookup class, or if this lookup object does not
> have private access privileges, the access fails.
The specified caller class in the example above is the interface itself,
not the lookup class.
The documentation has to be at least changed to reflect this feature.
3. Cause of the discrepancy between lookup class in named module vs
unnamed module
As Mandy Chung observed:
> IAE is thrown because the implementation calls Lookup::in teleporting
> to a Lookup of the special class which lost all access since the
> caller class and the requested lookup class are in different modules.
This behavior is documented in the Lookup.in() [6]:
> * If the old lookup class is in a named module, and the new lookup
> class is in a different module M, then no members, not even public
> members in M's exported packages, will be accessible. The exception to
> this is when this lookup is publicLookup, in which case PUBLIC access
> is not lost.
> * If the old lookup class is in an unnamed module, and the new lookup
> class is a different module then MODULE access is lost.
This strikes me as a little bit odd, so I looked around for the
reasoning for this difference, and found an old discussion on jigsaw-dev
[7]. I especially found this mail from Alan Bateman [8] interesting,
where he gave a reasoning for this:
> Preserved but perhaps with the (initially surprising) consequence that
> all access is lost when m(LC) is a named module and m(A) is a
> different module. This arises because the two modules may read very
> different sets of modules, the intersection cannot be expressed via a
> lookup class + modes.
While I agree that intersection might not be expressed via lookup class
+ modes, I don't think that it is necessary to express that. Instead
don't allow any lookup on members in a different module if the MODULE
flag is not set.
But this is only a suggestion. It's up to you to decide how to deal with
this.
Please keep up the good work you are doing, and I hope this mail will
help you fixing this bug.
Thanks for your time,
Johannes Kuhn
[1]: https://bugs.openjdk.java.net/browse/JDK-8209078
[2]:
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2017-January/010741.html
[3]: https://bugs.openjdk.java.net/browse/JDK-8130227
[4]: http://openjdk.java.net/jeps/274
[5]:
https://docs.oracle.com/javase/10/docs/api/java/lang/invoke/MethodHandles.Lookup.html#findSpecial(java.lang.Class,java.lang.String,java.lang.invoke.MethodType,java.lang.Class)
[6]:
https://docs.oracle.com/javase/10/docs/api/java/lang/invoke/MethodHandles.Lookup.html#in(java.lang.Class)
[7]:
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2015-December/005751.html
[8]:
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2015-December/005768.html
