Review Request: 8179025: Exclude deployment modules from FieldSetAccessibleTest.java and VerifyJimage.java

[Mandy Chung]

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

[alan . bateman]

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

[alan . bateman]

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

[Andrew Dinn]

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

[Doug Simon]

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 Simon  wrote:
> 
> 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

[Alan Bateman]

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

[Andrew Dinn]

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