Review Request: 8179025: Exclude deployment modules from FieldSetAccessibleTest.java and VerifyJimage.java
Webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8179025/webrev.00/index.html These tests failed due to IAE when loading types from the deployment modules which are expected to be defined when running with javaws or plugin. This revises the tests to exclude these modules to remove the tests from the problem list. In the long term, we should look into some way not to link in these modules in the image. This patch also updates JdkQualifiedExportTest.java test to take out the exception for deployment modules to have qualified exports to upgradeable modules. thanks Mandy
hg: jigsaw/jake/jdk: SecurityManager should not record packages of modules defined to app loader
Changeset: b38f70bbe88d Author:alanb Date: 2017-04-21 20:12 +0100 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/b38f70bbe88d SecurityManager should not record packages of modules defined to app loader ! src/java.base/share/classes/java/lang/SecurityManager.java
hg: jigsaw/jake/jdk: Load SecurityManager's non-exported packages when needed rather than eagerly
Changeset: 8d25210fce7b Author:alanb Date: 2017-04-21 17:57 +0100 URL: http://hg.openjdk.java.net/jigsaw/jake/jdk/rev/8d25210fce7b Load SecurityManager's non-exported packages when needed rather than eagerly ! src/java.base/share/classes/java/lang/Module.java ! src/java.base/share/classes/java/lang/SecurityManager.java ! src/java.base/share/classes/java/lang/System.java ! src/java.base/share/classes/jdk/internal/misc/JavaLangAccess.java ! src/java.base/share/classes/jdk/internal/module/Modules.java
Re: Using lookups in place of reflection
On 21/04/17 11:57, Alan Bateman wrote: > The check in checkUnprivilegedlookupClass isn't new, I think it's a > defense-in-depth check that goes back to JDK 7 to catch usages of full > power lookups in fully privileged code. There was recent discussion on > core-libs-dev about this, prompted by work that Paul Sandoz was doing to > retrofit LockSupport and TLR to use privateLookupIn. My recollection is > that he relaxed the check to allow the target class be Thread or a class > in j.u.concurrent and the more general question on whether this check is > needed was kicked down the road. I'm sure John Rose or Paul will jump in > on this thread. I'd be very pleased if they did :-) > BTW: None of the modules defined to the boot loader open packages so I > wouldn't expect too many people will run into this. You run into in the > example because java.net.HttpURLConnection is in java.base and defined > to the boot loader. I guess another possible scenario would be someone > opening a package in a core module with `--add-opens` and the raiding > party uses privateLookupIn. Ah, well, actually ... my agent can run into this even when java.base does not open a package. Byteman will open a package to its own private module if it wants to access protected/private members of classes in that otherwise unexported package. Of course, it only does so to do the specific, targetted accesses specified in rules provided by whoever installed the agent. It does not make the handles (never mind the packages) available to non-agent code. So, strictly ... I agree you are correct when you note that this is something that will mostly only affect a small number of people -- it only limits the options available to agent developers. Of course, that doesn't mean it won't have a knock-on effect for users of those agent (perhaps a rather larger commmunity :-). regards, Andrew Dinn --- Senior Principal Software Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Re: RFR: 8177845: Need a mechanism to load Graal
There has been some discussion about whether we want to update Graal in the JDK at this late stage. The main (only?) risk is a regression in the AOT tool. If we don't update Graal from upstream, then the qualified exports from JVMCI to jdk.internal.vm.compiler cannot be removed in JDK 9. Note that in addition to updating Graal to remove the qualified exports, there would also need to be changes in the relevant make files to add --add-exports options when compiling Graal and jaotc as they use the dynamically exported JVMCI packages. I have an updated hotspot patch that adapts Graal to the JVMCI API changes: http://cr.openjdk.java.net/~dnsimon/8177845/hotspot.02/ Note that this patch does not include the changes removing use of JDK internal API from Graal. Cherry picking those upstream Graal changes would be more work than simply doing a complete update from upstream Graal. As I see it, there are 2 options here: 1. Go with the current webrev (including hotspot.02 patch) and live with the qualified exports. 2. Go with the current webrev (including hotspot.02 patch) and create a follow up bug to update Graal from upstream, perform the relevant make file changes and remove the qualified exports. I've tested the current webrev (including hotspot.02 patch) against both upstream Graal and with jprt. -Doug > On 20 Apr 2017, at 20:50, Doug Simonwrote: > > I've had to update the webrev again to support selection of a "null" compiler > (i.e. one that raises an > exception upon a compilation request) and added -Djvmci.Compiler=null to a > large number of JVMCI jtreg > tests to prevent Graal being selected and initialized by the JVMCI compiler > auto-selection mechanism. > Initializing Graal will currently fail with errors (see stack trace below) > until Graal is updated to > the version compatible with the JVMCI API changes. > > In addition to resolving the compatibility issue, explicitly selecting the > "null" compiler for these > tests better isolates them from parts of the runtime they are not aiming to > test. > > org.graalvm.compiler.debug.GraalError: java.lang.ClassCastException: > java.base/java.util.ImmutableCollections$MapN cannot be cast to > java.base/java.util.Properties > at > jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.getSavedProperties(HotSpotGraalCompilerFactory.java:217) > at > jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.initializeOptions(HotSpotGraalCompilerFactory.java:138) > at > jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.onSelection(HotSpotGraalCompilerFactory.java:95) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCICompilerConfig.getCompilerFactory(HotSpotJVMCICompilerConfig.java:104) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.(HotSpotJVMCIRuntime.java:290) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.(HotSpotJVMCIRuntime.java:65) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime$DelayedInit.(HotSpotJVMCIRuntime.java:73) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.runtime(HotSpotJVMCIRuntime.java:83) > at jdk.internal.vm.ci/jdk.vm.ci.runtime.JVMCI.initializeRuntime(Native > Method) > at jdk.internal.vm.ci/jdk.vm.ci.runtime.JVMCI.(JVMCI.java:58) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.runtime(HotSpotJVMCIRuntime.java:82) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotVMConfig.config(HotSpotVMConfig.java:41) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotResolvedJavaMethodImpl.getHolder(HotSpotResolvedJavaMethodImpl.java:92) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotResolvedJavaMethodImpl.fromMetaspace(HotSpotResolvedJavaMethodImpl.java:110) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.CompilerToVM.asResolvedJavaMethod(Native > Method) > at > jdk.internal.vm.ci/jdk.vm.ci.hotspot.CompilerToVMHelper.asResolvedJavaMethod(CompilerToVMHelper.java:185) > at > compiler.jvmci.common.CTVMUtilities.getResolvedMethod(CTVMUtilities.java:59) > at > compiler.jvmci.common.CTVMUtilities.getResolvedMethod(CTVMUtilities.java:64) > at > compiler.jvmci.compilerToVM.AllocateCompileIdTest.runSanityCorrectTest(AllocateCompileIdTest.java:125) > at java.base/java.util.ArrayList.forEach(ArrayList.java:1378) > at > compiler.jvmci.compilerToVM.AllocateCompileIdTest.main(AllocateCompileIdTest.java:71) > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > at > java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > at java.base/java.lang.reflect.Method.invoke(Method.java:563) >
Re: Using lookups in place of reflection
On 21/04/2017 11:04, Andrew Dinn wrote: Being a good OpenJDK citizen I have attempted to make my agent use lookups in jdk9 in place of using reflection. This is mostly working fine. However, I have run across a rather arbitrary limitation (not just my conclusion -- the jdk source code acknowledges it as such) and am wondering whether there will be any chance of remedying it before (or after) jdk9 release. The check in checkUnprivilegedlookupClass isn't new, I think it's a defense-in-depth check that goes back to JDK 7 to catch usages of full power lookups in fully privileged code. There was recent discussion on core-libs-dev about this, prompted by work that Paul Sandoz was doing to retrofit LockSupport and TLR to use privateLookupIn. My recollection is that he relaxed the check to allow the target class be Thread or a class in j.u.concurrent and the more general question on whether this check is needed was kicked down the road. I'm sure John Rose or Paul will jump in on this thread. BTW: None of the modules defined to the boot loader open packages so I wouldn't expect too many people will run into this. You run into in the example because java.net.HttpURLConnection is in java.base and defined to the boot loader. I guess another possible scenario would be someone opening a package in a core module with `--add-opens` and the raiding party uses privateLookupIn. -Alan.
Using lookups in place of reflection
Being a good OpenJDK citizen I have attempted to make my agent use lookups in jdk9 in place of using reflection. This is mostly working fine. However, I have run across a rather arbitrary limitation (not just my conclusion -- the jdk source code acknowledges it as such) and am wondering whether there will be any chance of remedying it before (or after) jdk9 release. The problem (I'll come to the motivation as to /why/ it is a problem in a second) is this method of class java.lang.invoke.MethodHandles.Lookup private static void checkUnprivilegedlookupClass(Class lookupClass, int allowedModes) { String name = lookupClass.getName(); if (name.startsWith("java.lang.invoke.")) throw newIllegalArgumentException("illegal lookupClass: "+lookupClass); // For caller-sensitive MethodHandles.lookup() disallow lookup from // restricted packages. This a fragile and blunt approach. // TODO replace with a more formal and less fragile mechanism // that does not bluntly restrict classes under packages within // java.base from looking up MethodHandles or VarHandles. if (allowedModes == FULL_POWER_MODES && lookupClass.getClassLoader() == null) { if ((name.startsWith("java.") && !name.equals("java.lang.Thread") && !name.startsWith("java.util.concurrent.")) || (name.startsWith("sun.") && !name.startsWith("sun.invoke."))) { throw newIllegalArgumentException("illegal lookupClass: " + lookupClass); } } } This is indeed a 'blunt approach' -- I'm not sure about the fragility and it would be good to know what motivated this specific choice of blunt instrument and what might constitute a more honed instrument. What makes this so egregious is that this check method is invoked from method Lookup.privateLookupIn() which, before instituting the check, ensures, amongst other things, that the target class is open to the module of the Lookup's 'caller' class. What that adds up to is this: the the original Lookup cannot be used to access protected or private fields or methods of java.* or sun.* classes using a method handle reflection /can/ be used to access those same fields or methods That seems like a rather perverse incentive to stick with reflection. Now, why am I so concerned about this detail? Well, as an example, here is a snippet of a Byteman rule used to test HttpURLConnection code: RULE java-net: Java HttpUrlConnection connect entry CLASS ^java.net.HttpURLConnection METHOD connect() HELPER io.opentracing.contrib.agent.OpenTracingHelper AT ENTRY IF !$0.connected && retrieveSpan($0) == null && !ignore($0) DO associateSpan($0, getTracer().buildSpan($0.getRequestMethod()); ... ENDRULE I won't provide a full account of what this rule does but basically it defines code for the agent to inject into the connect() method of java.net.HttpURLConnection and its subclasses (the latter indicated by the ^). The purpose is to track the connection state so operation can be validated at end of the test run. HELPER class OpenTracingHelper is just a POJO that defines methods retrieveSpan(), ignore(), buildSpan() etc which construct and maintain this tracking info n.b. there are several more rules, defined on other methods of HttpURLConnection, which update the Span object. A few more specifics are called for. $0 identifies the target instance for the connect() call i.e. an instance of HttpURLConnection or one of its subclasses. In practice the rule code actually gets injected in method connect() of the underlying implementation class sun.net.www.protocol.http.HttpURLConnection so at runtime that will be the type of $0. The critical expression to attend to is the first term in the IF condition '$0.connected'. This reads boolean field 'connected' defined by superclass java.net.URLConnection of class HttpURLConnection. A test of this field is needed because it is only appropriate to track connection information for a URLConnection which has actually been connected. Unfortunately, field 'connected' is protected. Worse, none of the classes in the URLConnection hierarchy provide an accessor for it. So, the only way to implement this test/rule is to strong-arm a read of the protected field. The Byteman agent has to apply one or other form of leverage -- either reflection or a getter method handle. In jdk8 it successfully reads the field using reflection. In jdk9 it tries to behave well and use a getter but that is ruled out by the rather arbitrary check above. Other rules run into problems with fields/methods of the target class java.net.HttpURLConnection. So, what am I asking: Is there a clearer rationale for this check which might lead to a less restrictive variant? Can I haz it in jdk9 pleeze? regards, Andrew Dinn --- Senior Principal Software Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael