Agent should not open java.lang package to unnamed module of the application class loader

[Godin]

Fixes #1328

[Godin]

@marchof foreseeing your question about why this is still a draft 😉 because the code requires polishing and the addition of in-code comments, I plan to work on this a bit later. But of course, feel free to review to provide your suggestions/comments - as usual, they are more than welcome 🤗

[marchof]

First of all, thanks for digging deeper into this! I was too fast in closing this issue.

Just some high level remarks:

• Can we add a (integration) test case which demonstrates the original problem reported in Agent opens access to java.base to unnamed module #1328?
• Maybe add your comments and spec references from Agent opens access to java.base to unnamed module #1328.
• As the initialization is specific to the InjectedClassRuntime. I wonder whether the code should move there for better encapsulation. This would also allow to only create a class loader for Lookup and not for the overall InjectedClassRuntime class which has more aspects like bytecode generation. Then the PreMainwould look like this:

private static IRuntime createRuntime(final Instrumentation inst) throws Exception {
	if (InjectedClassRuntime.isSupported()) {
		return InjectedClassRuntime.createFor(inst);
	}
	return ModifiedSystemClassRuntime.createFor(inst, "java/lang/UnknownError");
}

[marchof]

Maybe for consistency and better readability this code can be extracted to a private method like getModule()

[Godin]

Can we add a (integration) test case

Sure.

First of all for such a test case we need a "stable" private JDK API 😆 e.g. Throwable.setCause mentioned in #1328 is not presented in JDK 11 - was added in JDK 12, so instead I propose to use

Class.forName("java.lang.Module").getDeclaredConstructors()[0].setAccessible(true);

because IMO less likely that any of the constructors of java.lang.Module will become public than the removal or change of signature of java.lang.Throwable.setCause

Secondly on 9 <= JDK < 16 we either need --illegal-access=deny or

class Example {
    public static void main(String[] args) throws Exception {
        System.out.println(Object.class.getModule().isOpen("java.lang", Example.class.getModule()));

        Class.forName("java.lang.Module").getDeclaredConstructors()[0].setAccessible(true);
    }
}

leads to

true
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by Main (file:/private/tmp/j/) to constructor java.lang.Module(java.lang.ModuleLayer,java.lang.ClassLoader,java.lang.module.ModuleDescriptor,java.net.URI)
WARNING: Please consider reporting this to the maintainers of Main
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

instead of

false
Exception in thread "main" java.lang.reflect.InaccessibleObjectException: Unable to make java.lang.Module(java.lang.ModuleLayer,java.lang.ClassLoader,java.lang.module.ModuleDescriptor,java.net.URI) accessible: module java.base does not "opens java.lang" to unnamed module @11739fa6
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
        at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
        at java.base/java.lang.reflect.Constructor.checkCanSetAccessible(Constructor.java:188)
        at java.base/java.lang.reflect.Constructor.setAccessible(Constructor.java:181)

--illegal-access=deny is not needed on JDK 16 because of https://openjdk.org/jeps/396, and not needed on JDK 17 because of https://openjdk.org/jeps/403, which unfortunately also states

We expect to remove the --illegal-access option entirely in a future release.

on JDK < 9 --illegal-access=deny leads to

Unrecognized option: --illegal-access=deny
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

so for simplicity I propose to test only JDK >= 16.

Finally to test the agent we need to

• either introduce a new type of tests
• or add it to jacoco-maven-plugin.test which are already slow
• or add it to org.jacoco.ant.test which for simplicity I propose to do

---

And I'll work on addressing other comments a bit later.

[marchof]

So for simplicity I propose to test only JDK >= 16

👍

[Godin]

As the initialization is specific to the InjectedClassRuntime. I wonder whether the code should move there for better encapsulation. This would also allow to only create a class loader for Lookup and not for the overall InjectedClassRuntime class which has more aspects like bytecode generation.

Note that custom ClassLoader delegates to the parent for everything except InjectedClassRuntime and its nested class(es)

			public Class<?> loadClass(String name)
					throws ClassNotFoundException {
				if (!name.startsWith(InjectedClassRuntime.class.getName())) {
					return super.loadClass(name);
				}

so ASM classes for bytecode generation are still loaded by the same ClassLoader as before if that's your concern.

Unfortunately to create ClassLoader only for Lookup we can't simply change only the above code on

  			public Class<?> loadClass(String name)
  					throws ClassNotFoundException {
- 				if (!name.startsWith(InjectedClassRuntime.class.getName())) {
+ 				if (!name.equals(Lookup.class.getName())) {
  					return super.loadClass(name);
  				}

while retaining

.loadClass(InjectedClassRuntime.class.getName())

because according to https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-5.html#jvms-5.4.3.1

To resolve an unresolved symbolic reference from D to a class or interface C denoted by N, the following steps are performed:
The defining loader of D is used to load and thereby create a class or interface denoted by N. This class or interface is C.

reference to Lookup from InjectedClassRuntime resolved by defining loader of InjectedClassRuntime.

This means that to use custom ClassLoader only for Lookup:
reflection and loadClass(Lookup.class.getName()) of this custom ClassLoader should be used instead of direct references in a place where these references to Lookup are - startup method of InjectedClassRuntime.
And since we need a module of this custom ClassLoader for instrumentation.redefineModule, then either Instrumentation should be passed down to startup or ClassLoader.

Currently InjectedClassRuntime does not require Instrumentation - enough if by any means (not necessary via Instrumentation) module with the target class opens the package containing the target class to at least module of InjectedClassRuntime.

So I pushed two commits:
first one moves code from PreMain into InjectedClassRuntime.createFor
and second one to use custom ClassLoader only for Lookup while preserving ability to use InjectedClassRuntime without Instrumentation

Both achieve the same result. What are the benefits of the second one over the first one?

Custom ClassLoader and Lookup can be GCed immediately after startup of runtime, instead of living all the time runtime is alive, which maybe is not such a big benefit.

And a bit fewer CPU cycles at runtime creation because of fewer Strings comparisons in custom ClassLoader. Then some of these saved CPU cycles are wasted on reflective call of Lookup. And in my opinion amount of comparisons even in the first one is small - note that only the resolution of references in InjectedClassRuntime and Lookup goes thru this custom ClassLoader (recall "defining loader" above). More precisely in the first case loadClass is invoked only for the following names

org.jacoco.agent.rt.internal_5a43169.core.runtime.InjectedClassRuntime
org.jacoco.agent.rt.internal_5a43169.core.runtime.AbstractRuntime
java.lang.ClassLoader
org.jacoco.agent.rt.internal_5a43169.core.runtime.InjectedClassRuntime$1
java.lang.Class
java.lang.String
java.lang.StringBuilder
java.lang.Package
org.jacoco.agent.rt.internal_5a43169.core.runtime.InjectedClassRuntime$Lookup
java.lang.Object
java.lang.invoke.MethodHandles
java.lang.reflect.Method
java.lang.invoke.MethodHandles$Lookup
org.jacoco.agent.rt.internal_5a43169.asm.ClassWriter
java.lang.reflect.Field
org.jacoco.agent.rt.internal_5a43169.asm.MethodVisitor
org.jacoco.agent.rt.internal_5a43169.core.runtime.RuntimeData

and only two of them

org.jacoco.agent.rt.internal_5a43169.core.runtime.InjectedClassRuntime
org.jacoco.agent.rt.internal_5a43169.core.runtime.InjectedClassRuntime$Lookup

will be defined by this custom ClassLoader.

Also to me seems that the second one requires the addition of even more in-code comments than the first one, e.g. to explain use of setAccessible.

Thus I'm really not sure whether the second commit is actually needed or not. @marchof WDYT?

[marchof]

Hi @Godin, now as it properly encapsulated in InjectedClassRuntime I think it is ok to create the classloader in createFor() only (so leave out the second commit).

But in createFor() I would add some implementation comments why the class loader is created.

[marchof]

To further encapsulate the implementation details of InjectedClassRuntime I would add a static method isSupported() to this class. The code here in PreMain then reduces to:

private static IRuntime createRuntime(final Instrumentation inst)
		throws Exception {
	if (InjectedClassRuntime.isSupported()) {
		return InjectedClassRuntime.createFor(inst);
	} else {
		return ModifiedSystemClassRuntime.createFor(inst,
				"java/lang/UnknownError");
	}
}

[marchof]

Wow very impressed by all the technical details of this fix! I only have two minor cleanup proposals.

[Godin]

I only have two minor cleanup proposals.

@marchof actually maybe I myself have one more 😆 I have a feeling that maybe better to move createFor method into PreMain class: strictly speaking InjectedClassRuntime is not in the internal package and thus part of our public API, whereas createFor is just a particular way to instantiate InjectedClassRuntime specific to our agent with hard-coded values such as java.lang and $JaCoCo. WDYT?

[marchof]

I only have two minor cleanup proposals.

@marchof actually maybe I myself have one more 😆 I have a feeling that maybe better to move createFor method into PreMain class: strictly speaking InjectedClassRuntime is not in the internal package and thus part of our public API, whereas createFor is just a particular way to instantiate InjectedClassRuntime specific to our agent with hard-coded values such as java.lang and $JaCoCo. WDYT?

I fully agree, except that PreMain isn't really a better place neither. What about a new class in package org.jacoco.agent.rt.internal which handles the aspect of class loader creation, module opening and loading specific classes? This may open opportunities for unit testing too. But I need to find a proper name and API.

[Godin]

But I need to find a proper name and API

Here are some ideas about name:

• InjectedClassRuntimeInstantiator
• we already have some classes with prefix Support - InjectedClassRuntimeSupport
• well maybe strictly speaking these are not really suitable names, but anyway - InjectedClassRuntimeFactory or InjectedClassRuntimeBuilder

Also what do you think about just moving the method into PreMain and merging now to unlock this bug fix, and then working on the improvement of PreMain separately?

[marchof]

I wonder whether this *Support tool should be specific about the InjectedClassRuntime at all. I would encapsulate the aspect of loading/instantiating a arbitrary class in its own class loader. This way it may become unit testable.

[pzygielo]

Seems that the change b865890 causes failure https://ci.eclipse.org/glassfish/job/hk2/job/PR-819/3/console.

I've not investigated fully, it's possible that upgrade proposed in

• Bump jacoco-maven-plugin from 0.8.8 to 0.8.9 eclipse-ee4j/glassfish-hk2#819

needs some additional modifications.

Reporting just for cross-linking, while it's relatively fresh.

[Godin]

@pzygielo Thank you for letting us know! ❤️ I created #1425

[pzygielo]

Evgeny, thanks for checking.