Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Agent can inject class into Java 9+ bootstrap class loader #829

Merged
merged 21 commits into from
Jan 21, 2019

Conversation

Godin
Copy link
Member

@Godin Godin commented Jan 11, 2019

Starting from JDK 9 there is java.lang.invoke.MethodHandles.Lookup.defineClass
that can define class even in bootstrap classloader

MethodHandles
  .privateLookupIn(Object.class, MethodHandles.lookup())
  .defineClass(classBytes);

when java.base module opens java.lang package to module of agent,
what can be done by agent itself using java.lang.instrument.Instrumentation.redefineModule

instrumentation.redefineModule(
  Object.class.getModule(),
  Collections.emptySet(), // extraReads
  Collections.emptyMap(), // extraExports
  Collections.singletonMap(
    "java.lang",
    Collections.singleton(Agent.class.getModule())
  ), // extraOpens
  Collections.emptySet(), // extraUses
  Collections.emptyMap() // extraProvides
);

This way agent can be used with JDKs compiled into bytecode version whose instrumentation is not yet supported, e.g. JDK 13 EA (now each new JDK version is compiled into a new bytecode version). And I believe that majority of people do not compile their code into new bytecode version when testing new JDK EA builds, so this will be already useful for them.

For this use-case such approach is IMO

  • definitely safer than automation of replacement of class
  • and even than usage of ASM with experimental support of new bytecode version, which is as fragile as our previous tricks with downgrade-upgrade.

Furthermore this approach allows to get rid of conflicts with other agents on JDK 9+.

I wrapped reflective calls of these methods into few helper classes to keep compatibility with JDK 5.

@Godin Godin self-assigned this Jan 11, 2019
@Godin Godin added this to Implementation in Current work items via automation Jan 11, 2019
@Godin Godin force-pushed the class_injection branch 2 times, most recently from 8a04cf2 to 08a9ed6 Compare January 11, 2019 19:15
@Godin Godin requested a review from marchof January 11, 2019 19:57
@Godin Godin moved this from Implementation to Review in Current work items Jan 11, 2019
@marchof
Copy link
Member

marchof commented Jan 14, 2019

@Godin I would prefer to implement a separate IRuntime for this approach and also move it to org.jacoco.core. Looks cleaner to me. This would require some new API like isApplicable().

What I don't like about re-using ModifiedSystemClassRuntime is that there is no actual modification, it just works as the newly injected class looks like a modified class, i.e. has a $jacocoAccess field.

@Godin Godin changed the title For JDK 9+ agent can inject class into bootstrap class loader Agent can inject class into Java 9+ bootstrap class loader Jan 16, 2019
@Godin Godin added this to the 0.8.3 milestone Jan 16, 2019
@Godin
Copy link
Member Author

Godin commented Jan 16, 2019

@marchof implemented ClassInjectionRuntime

@marchof
Copy link
Member

marchof commented Jan 17, 2019

@Godin As a first step I directly fixed Eclipse warnings, hope this is ok for you. Will have a closer look now.

Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions...

*/
public static IRuntime create(final Instrumentation instrumentation) {
try {
redefineJavaBaseModule(instrumentation);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the best error handling strategy here?

A) Just check whether we're on Java 9 and then fail if this runtime can't be initialized?
B) Ignore any error while initialization and simply fall back to ModifiedSystemRuntime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed implementation from second to first.

Lookup.lookup() //
.bind( //
instrumentation, //
"redefineModule", //
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marchof doh, not only reflection, but even this doesn't work in case of --illegal-access=deny 😢

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact

instrumentation.getClass().getMethod(...).invoke(...)

and

Lookup.lookup().bind(instrumentation, ...).invokeWithArguments(...)

are illegal, whereas

java.lang.Instrumentation.class.getMethod(...).invoke(...)

and

Lookup.lookup().findVirtual(java.lang.Instrumentation.class, ...).invokeWithArguments(...)

are legal, because instrumentation.getClass() is sun.instrument.InstrumentationImpl and

module java.instrument {
    exports java.lang.instrument;

    // allow java launcher to load agents in executable JAR files
    exports sun.instrument to java.base;
}

@Godin Godin moved this from Review to Implementation in Current work items Jan 18, 2019
@Godin Godin moved this from Implementation to Review in Current work items Jan 19, 2019
Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Godin I really like this reworked, very concise version, thanks! I came across two minor formal issues, please decide whether you want to address them.

@marchof marchof merged commit d0a0577 into master Jan 21, 2019
Current work items automation moved this from Review to Done Jan 21, 2019
@marchof marchof deleted the class_injection branch January 21, 2019 17:39
@jacoco jacoco locked as resolved and limited conversation to collaborators May 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants