Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hei Mandy,

after further investigation I have found several other use cases of
sun.misc.Unsafe::defineClass for which the proposal for JDK-8200559 would
not currently offer a solution. I will try to describe a generic case of
the problem I found:

Java agents sometimes need to communicate state from instrumented classes
to a dispatcher that comes with the agent. However, a Java agent cannot
generally expect that any agent classes are visible to an application as a
Java agent is loaded on the class path whereas application classes might
not be loaded by the system class loader or any of its children. Therefore,
instrumented classes cannot typically callback into the agent. This is
typically solved by adding classes to the bootstrap class loader which is
universally visible.

Such a callback typically looks like this:

public abstract class Dispatcher {
  public volatile Dispatcher dispatcher;
  public abstract void receiveMessage(Object msg);
  public static void sendMessage(Object msg) {
Dispatcher d = dispatcher;
if (d != null) d.receiveMessage(msg);
  }
}

The agent can now register a subclass of the dispatcher in the field and
receive messages from any instrumented code throughout an application
without considering class loaders.

Adding this call would be trivial by using the
Instrumentation::appendToBootSearchPath method. However, using this method
has two major downsides:
1. The method accepts a file which must be stored on the local file system.
This can be really tricky in some cases, it is typically easier to just
define the classes as byte arrays and sun.misc.Unsafe::defineClass is a way
to avoid this problem.
2. Appending to the search path is only possible in the unnamed module
since Java 9. It is no longer possible to define the above dispatcher to be
located in java.util for instance as the new standard class loader does not
consider the new jar for known modules anymore. Defining the dispatcher in
a well known package has two major advantages:
  a) It is not necessary to alter the module graph to make sure that all
modules with instrumented classes read the boot loaders unnamed module.
This requires a lot of additional maintenance which impacts performance and
memory use. Also, bugs can trigger security risks if edges are added
excessively. With defining a class in java.util, this can be avoided since
the java.base module is impliiclty read by any module.
  b) Some class loaders filter packages that are considered to be loaded,
especially OSGi and JBoss modules class loaders. Those class loaders would
for example never attempt to find classes in the com.acme package on the
bootstrap loader which is why it has become a popular practice to add
classes to java.* packages for such universal dispatch. There are dozens of
such restricting class loaders and working around them is nearly impossible
as there are so many.

With the current consideration, it would instead be required to "pseudo
redefine" a class such as java.util.List only to inject the dispatcher into
the desired package. (Alternatively one can also open the module of
java.base and use a method handles lookup but this is a bad idea if the
Java agent runs on the unnamed system class loader.) Note that all this is
possible using standard API. It is even easier to do with JVMTI where class
definition is trivial.

For all the reasons I have mentioned in previous mails and also for this
additional use case I hope that adding a method for defining a class can be
added to the Instrumentation interface, it would definitely allow for the
cleanest, fastest and least error-prone migration while not inviting to the
mentioned work-arounds which are also currently favored by most teams
working with Java agents that I know of where many of them just open
jdk.internal.misc to use the new Unsafe class what is really unfortunate as
this is a chance to avoid use of internal API alltogether. This choice
would also reduce additional time for Java 11 being supported by all major
production monitoring tools that could just add simple switches to their
code to move from Unsafe to Instrumentation.

It would also be trivial to implement with an interface extension like:

interface Instrumentation {
  default Class defineClass(byte[] bytes, ClassLoader loader,
ProtectionDomain pd) { throw new
UnsupportedOperationException("defineClass"); }
}

Thanks for considering my suggestion and your feedback!
Best regards, Rafael



2018-04-15 8:23 GMT+02:00 mandy chung :

> Background:
>
> Java agents support both load time and dynamic instrumentation.   At load
> time,
> the agent's ClassFileTransformer is invoked to transform class bytes.
> There is
> no Class objects at this time.  Dynamic instrumentation is when
> redefineClasses
> or retransformClasses is used to redefine an existing loaded class.  The
> ClassFileTransformer is invoked with class bytes where the Class object is
> present.
>
> Java agent doing instrumentation needs a means to define 

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hei Mandy,

yes, this sums it up, I think and I have worked with all use cases where
sun.misc.Unsafe::defineClass is more then sufficient. One major issue I
also have with the current API are the costs in refactoring existing
agents, especially since one would need to deliver a seperate agent for
Java 11+ to rely on the additional API that is not available in previous
versions. Since Java 8 is still popular and most likely will be in many
year to come, I believe this additional maintainance cost would drive the
decision to not use the proposed API.

If one had a direct equivalent - such as Instrumentation::defineClass -
this would just be the easiest path forward and propably result in a quick
migration since current agents can simply call the new method using
reflection where an agent can decide at runtime if Unsafe::defineClass or
Instrumentation::defineClass could be used.

Finally, after I prototyped a bit, I found the proposed solution a bit
difficult to use. For example, if I instrument three classes in the same
package that all communicate via a new class that I inject, I only need to
inject the class in question a single time. However, I cannot always
determine which of the three classes is loaded first as this depends on the
application. Now I need to add some form of atomic boolean to apply the
injection on the first instrumentation but this can also be problematic
since another class might be loaded concurrently by another thread where I
ended up inserting locks etc what yielded even more problems. In the
current code, I just inject the class in question before registering the
transformer to avoid the problem at all.

Therefore I believe that the Instrumentation::defineClass would offer a
best solution given today's situation. The proposed API's capabilities
could easily be emulated by an agent author if this package-local scope was
required for a plugin. Also, since the instrumentation API already offers a
way of defining classes in random packages without using internal API (the
suggested noop redefinition of a class in the target package or by opening
modules with a subsequent use of method handles loopup), this does not
expose new functionality that is not already offered in a less convenient
way. Beyond the workarounds that I already brought up, I know of several
more that are in use today and I believe that by offering an easy way of
class definition as with Instrumentation::defineClass, there is a good
chance that all of those will go away eventually.

Thank you for considering!
Best regards, Rafael

2018-05-30 6:46 GMT+02:00 mandy chung :

> Hi Rafael,
>
> Thanks for the additional use cases of Unsafe::defineClass and looking
> through many different agents to identify their migration path.
>
> Summarize the requirements you collected (please correct/add):
>
> 1. need a means to define auxiliary classes in the same runtime package
>of an instrumented class, e.g. to bridge non-public members
>
> 2. need a means to define agent classes in a module that provides
>similar functionality as appendToBootstrapClassLoader and
>appendToSystemClassLoaderSearch​
>
> 3. define a class in a different runtime package as the instrumented
>class.  It's still unclear how common this is (inject a subclass
>that depends on a package-private constructor)
>
> The proposed API requires the agent to redefine the class in a
> package that the agent intends to inject its class into (e.g.
> redefine java.util.List in order to define java.util.AgentDispatcher).
> The proposed API requires more work than the existing
> Unsafe::defineClass that can define a class in any package but
> still plausible, right?
>
> We should explore other alternatives, if any.
>
> Mandy
> [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/20
> 18-April/023535.html
>
> On 5/29/18 2:17 PM, Rafael Winterhalter wrote:
>
>> Hei Mandy,
>>
>> after further investigation I have found several other use cases of
>> sun.misc.Unsafe::defineClass for which the proposal for JDK-8200559 would
>> not currently offer a solution. I will try to describe a generic case of
>> the problem I found:
>>
>> Java agents sometimes need to communicate state from instrumented classes
>> to a dispatcher that comes with the agent. However, a Java agent cannot
>> generally expect that any agent classes are visible to an application as a
>> Java agent is loaded on the class path whereas application classes might
>> not be loaded by the system class loader or any of its children. Therefore,
>> instrumented classes cannot typically callback into the agent. This is
>> typically solved by adding classes to the bootstrap class loader which is
>> universally visible.
>>
>> Such a callback typically looks like this:
>>
>> public

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hi,

I was wondering if a solution for this problem is still planned for JDK 11
giving the beginning ramp down.

With removing sun.misc.Unsafe::defineClass, Java agents only have an option
to use jdk.internal.misc.Unsafe::defineClass for the use-cases that I
described.

I think it would be a missed opportunity not to offer an alternative as of
JDK 11 as a second migration would make it even less likely that agents
would avoid unsafe API.

Thanks for the information,
best regards, Rafael

mandy chung  schrieb am So., 15. Apr. 2018, 08:23:

> Background:
>
> Java agents support both load time and dynamic instrumentation.   At load
> time,
> the agent's ClassFileTransformer is invoked to transform class bytes.
> There is
> no Class objects at this time.  Dynamic instrumentation is when
> redefineClasses
> or retransformClasses is used to redefine an existing loaded class.  The
> ClassFileTransformer is invoked with class bytes where the Class object is
> present.
>
> Java agent doing instrumentation needs a means to define auxiliary classes
> that are visible and accessible to the instrumented class.  Existing agents
> have been using sun.misc.Unsafe::defineClass to define aux classes directly
> or accessing protected ClassLoader::defineClass method with setAccessible
> to
> suppress the language access check (see [1] where this issue was brought
> up).
>
> Instrumentation::appendToBootstrapClassLoaderSearch and
> appendToSystemClassLoaderSearch
> APIs are existing means to supply additional classes.  It's too limited
> for example it can't inject a class in the same runtime package as the
> class
> being transformed.
>
> Proposal:
>
> This proposes to add a new ClassFileTransformer.transform method taking
> additional ClassDefiner parameter.  A transformer can define additional
> classes during the transformation process, i.e.
> when ClassFileTransformer::transform is invoked.  Some details:
>
> 1. ClassDefiner::defineClass defines a class in the same runtime package
>as the class being transformed.
> 2. The class is defined in the same thread as the transformers are being
>invoked.   ClassDefiner::defineClass returns Class object directly
>before the transformed class is defined.
> 3. No transformation is applied to classes defined by
> ClassDefiner::defineClass.
>
> The first prototype we did is to collect the auxiliary classes and define
> them  until all transformers are invoked and have these aux classes to go
> through the transformation pipeline.  Several complicated issues would
> need to be resolved for example timing whether the auxiliary classes
> should
> be defined before the transformed class (otherwise a potential race where
> some other thread references the transformed class and cause the code to
> execute that in turn reference the auxiliary classes.  The current
> implementation has a native reentrancy check that ensure one class is being
> transformed to avoid potential circularity issues.  This may need JVM TI
> support to be reliable.
>
> This proposal would allow java agents to migrate from internal API and
> ClassDefiner to be enhanced in the future.
>
> Webrev:
>http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/
>
> Mandy
> [1]
> http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January/000405.html
>

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

9:17 GMT+02:00 mandy chung :

> My proposal of ClassDefiner API allows the java agent to define auxiliary
> classes in the same runtime package of the class being instrumented.  You
> raised other use cases that are not addressed by this proposal.  As Alan
> replied, the ability to define any arbitrary class would be an attractive
> nuisance and we think Instrumentation.defineClass isn't the right API to
> add.
>
> I think the proposed ClassDefiner API is useful for the specific use case
> (define auxiliary classes in the runtime package of the class being
> instrumented).  I hold it off and so didn't make 11.  For the other use
> cases, perhaps we should create JBS issues for further investigation.
>
> Mandy
>
> On 7/2/18 1:41 AM, Rafael Winterhalter wrote:
>
>> Hi,
>>
>> I was wondering if a solution for this problem is still planned for JDK
>> 11 giving the beginning ramp down.
>>
>> With removing sun.misc.Unsafe::defineClass, Java agents only have an
>> option to use jdk.internal.misc.Unsafe::defineClass for the use-cases
>> that I described.
>>
>> I think it would be a missed opportunity not to offer an alternative as
>> of JDK 11 as a second migration would make it even less likely that agents
>> would avoid unsafe API.
>>
>> Thanks for the information,
>> best regards, Rafael
>>
>> mandy chung mailto:mandy.ch...@oracle.com>>
>> schrieb am So., 15. Apr. 2018, 08:23:
>>
>> Background:
>>
>> Java agents support both load time and dynamic instrumentation. At
>> load time,
>> the agent's ClassFileTransformer is invoked to transform class
>> bytes.  There is
>> no Class objects at this time.  Dynamic instrumentation is when
>> redefineClasses
>> or retransformClasses is used to redefine an existing loaded class.
>> The
>> ClassFileTransformer is invoked with class bytes where the Class
>> object is present.
>>
>> Java agent doing instrumentation needs a means to define auxiliary
>> classes
>> that are visible and accessible to the instrumented class. Existing
>> agents
>> have been using sun.misc.Unsafe::defineClass to define aux classes
>> directly
>> or accessing protected ClassLoader::defineClass method with
>> setAccessible to
>> suppress the language access check (see [1] where this issue was
>> brought up).
>>
>> Instrumentation::appendToBootstrapClassLoaderSearch and
>> appendToSystemClassLoaderSearch
>> APIs are existing means to supply additional classes.  It's too
>> limited
>> for example it can't inject a class in the same runtime package as
>> the class
>> being transformed.
>>
>> Proposal:
>>
>> This proposes to add a new ClassFileTransformer.transform method
>> taking additional ClassDefiner parameter.  A transformer can define
>> additional
>> classes during the transformation process, i.e.
>> when ClassFileTransformer::transform is invoked. Some details:
>>
>> 1. ClassDefiner::defineClass defines a class in the same runtime
>> package
>> as the class being transformed.
>> 2. The class is defined in the same thread as the transformers are
>> being
>> invoked.   ClassDefiner::defineClass returns Class object directly
>> before the transformed class is defined.
>> 3. No transformation is applied to classes defined by
>> ClassDefiner::defineClass.
>>
>> The first prototype we did is to collect the auxiliary classes and
>> define
>> them  until all transformers are invoked and have these aux classes
>> to go
>> through the transformation pipeline.  Several complicated issues would
>> need to be resolved for example timing whether the auxiliary classes
>> should
>> be defined before the transformed class (otherwise a potential race
>> where
>> some other thread references the transformed class and cause the code
>> to
>> execute that in turn reference the auxiliary classes.  The current
>> implementation has a native reentrancy check that ensure one class
>> is being
>> transformed to avoid potential circularity issues.  This may need
>> JVM TI
>> support to be reliable.
>>
>> This proposal would allow java agents to migrate from internal API
>> and ClassDefiner to be enhanced in the future.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/
>>
>> Mandy
>> [1]
>> http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January/
>> 000405.html
>>
>>

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hei Mandy,

Generally I agree with you that these two should be seperate but I think
that there is a very blurry border between the two. Java agents are
sometimes used to test something in a production environment and there is a
long history of agent development that relies on the easy and general way
of defining classes using sun.misc.Unsafe. I have looked through the agents
I worked on and for the most, classes are defined in the same package. In a
few cases, and this is true for most agents, there is however a need to
define classes in other packages, too.

In this context, things quickly get complex to manage. If I use a method
handle lookup, I need to find a loaded class and potentially open its
module. From within an agent, it is however way to easy to break class
loading orders and to trigger premature loading what makes this
inconvienent. The same goes for retransforming a class to get hold of a
suitable class definer instance for a given package.

For all these reasons, I believe that given this approach, most people will
simply fall back to jdk.internal.misc.Unsafe which can be opened too and
which is much easier. Therefore, given the tradition of using this class
and its more powerful and more performent approach to defining classes
given the multitude of use cases, I believe that this API would not be used
by most but people would continue to rely on jdk.internal.misc.Unsafe. At
least, I think that I would go for this solution for most of my agents,
also considering an easy and safe migration.

For this reason, I still recommend adding a method
Instrumentation::defineClass instead. Especially since a Java agent does
have this privilege, there is no security constraint to uphold here and it
offers the simplest solution that most agent developers hope for.
Therefore, I do not think that the principle of least privilege should
apply here. In contrast, if this API is not added I am afraid that many
tools opening jdk.internal.misc might lead to exploits if this package is
opened to an unnamed module, for example by APM or other production
monitoring tools that typically need to define classes in packages in which
no retransformation is applied. (The example I mentioned comes from a real
use case in a very widely used APM tool.)

Thank you again for considering my point of view on this!
Best regards, Rafael



2018-04-18 9:46 GMT+02:00 mandy chung <mandy.ch...@oracle.com>:

> Hi Rafael,
>
> I think it's best to separate the testing requirement from java agents
> doing instrumentation that may run in production environment.
>
> I have created a JBS issue to track the testing mode idea that would
> require more discussion and investigation:
>   https://bugs.openjdk.java.net/browse/JDK-8201562
>
> I understand it's not as efficient to inject a class in a package
> different than the package of the transformed class.  With the principle of
> least privilege, I prefer not to provide an API to inject any class in any
> package and you can achieve by calling retransformClasses.
>
> What do you think?
>
> Mandy
>
> On 4/18/18 5:23 AM, Rafael Winterhalter wrote:
>
> Hei Mandy,
> Lookup::defineClass would always be an alternative but it would require me
> to open the class first. If the instrumented type can read the module with
> the callback but its module was not opened, this would not help me much,
> unfortunately. Also, I could not resolve this lookup as the class in
> question is not necessarily loaded at this point.
> Best regards, Rafael
>
> 2018-04-17 9:28 GMT+02:00 mandy chung <mandy.ch...@oracle.com>:
>
>> Hi Rafael,
>>
>> I see that mocking/proxying/testing framework should be looked at
>> separately since its requirements and approaches can be different than tool
>> agents.
>>
>> On 4/17/18 5:06 AM, Rafael Winterhalter wrote:
>>
>> Hei Mandy,
>>
>> I have looked into several Java agents that I have worked on and for many
>> of them, this API does unfortunately not supply sufficient access. I would
>> therefore still prefer a method Instrumentation::defineClass.
>>
>> The problem is that some agents need to define classes in other packages
>> then in that of the instrumented class. For example, I might need to
>> enhance a library that defines a set of callback classes in package A. All
>> these classes share a common super class with a package-private
>> constructor. I want to instrument some class in package B to use a callback
>> that the library does not supply and need to add a new callback class to A.
>> This is not possible using the current API.
>>
>>
>> Are these callback classes made available statically?  or just
>> dynamically defining additional class as needed?  Is Lookup::defineClass an
>> alternative if you get a hold of 

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hi David,

>From my perspective as a heavy user of this API in Unsafe today:

>From my experience, the recursive dependency is not a common problem as
auxiliary types normally only reference the instrumented type. Beyond, this
problem is an issue for such explicit dynamic class loading, also when
using a method handle lookup.

I also think it is essential to being able to inject a class into a
non-opened package. Otherwise one could use a method handle. When
instrumenting classes one already has full privilege for all packages as an
agent can open modules. If one opens packages to define a class, this would
however open the package to the agent's module which is typically the
unnamed module. As this cannot be undone, such a requirement might
therefore be counter-productive to its intend.

Best regards, Rafael


David Holmes  schrieb am Mo., 16. Apr. 2018, 08:13:

> Hi Mandy,
>
> How do you handle dependencies across a set of auxiliary types, i.e if
> you are defining a new class A, and it depends on B (which you also
> intend to define), how will the classloader resolve references from A to
> B when it can't load B itself?
>
> Is the key capability here the ability to inject a class into a package
> that would otherwise not be open to it? What are the security
> implications here?
>
> Thanks,
> David
>
> On 15/04/2018 4:23 PM, mandy chung wrote:
> > Background:
> >
> > Java agents support both load time and dynamic instrumentation. At load
> > time,
> > the agent's ClassFileTransformer is invoked to transform class bytes.
> > There is
> > no Class objects at this time.  Dynamic instrumentation is when
> > redefineClasses
> > or retransformClasses is used to redefine an existing loaded class.  The
> > ClassFileTransformer is invoked with class bytes where the Class object
> > is present.
> >
> > Java agent doing instrumentation needs a means to define auxiliary
> classes
> > that are visible and accessible to the instrumented class. Existing
> agents
> > have been using sun.misc.Unsafe::defineClass to define aux classes
> directly
> > or accessing protected ClassLoader::defineClass method with
> setAccessible to
> > suppress the language access check (see [1] where this issue was brought
> > up).
> >
> > Instrumentation::appendToBootstrapClassLoaderSearch and
> > appendToSystemClassLoaderSearch
> > APIs are existing means to supply additional classes.  It's too limited
> > for example it can't inject a class in the same runtime package as the
> class
> > being transformed.
> >
> > Proposal:
> >
> > This proposes to add a new ClassFileTransformer.transform method taking
> > additional ClassDefiner parameter.  A transformer can define additional
> > classes during the transformation process, i.e.
> > when ClassFileTransformer::transform is invoked. Some details:
> >
> > 1. ClassDefiner::defineClass defines a class in the same runtime package
> > as the class being transformed.
> > 2. The class is defined in the same thread as the transformers are being
> > invoked.   ClassDefiner::defineClass returns Class object directly
> > before the transformed class is defined.
> > 3. No transformation is applied to classes defined by
> > ClassDefiner::defineClass.
> >
> > The first prototype we did is to collect the auxiliary classes and define
> > them  until all transformers are invoked and have these aux classes to go
> > through the transformation pipeline.  Several complicated issues would
> > need to be resolved for example timing whether the auxiliary classes
> should
> > be defined before the transformed class (otherwise a potential race where
> > some other thread references the transformed class and cause the code to
> > execute that in turn reference the auxiliary classes.  The current
> > implementation has a native reentrancy check that ensure one class is
> being
> > transformed to avoid potential circularity issues.  This may need JVM TI
> > support to be reliable.
> >
> > This proposal would allow java agents to migrate from internal API and
> > ClassDefiner to be enhanced in the future.
> >
> > Webrev:
> > http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/
> >
> > Mandy
> > [1]
> http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January/000405.html
>

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hei Mandy,

I have looked into several Java agents that I have worked on and for many
of them, this API does unfortunately not supply sufficient access. I would
therefore still prefer a method Instrumentation::defineClass.

The problem is that some agents need to define classes in other packages
then in that of the instrumented class. For example, I might need to
enhance a library that defines a set of callback classes in package A. All
these classes share a common super class with a package-private
constructor. I want to instrument some class in package B to use a callback
that the library does not supply and need to add a new callback class to A.
This is not possible using the current API.

I could however achieve do so by calling Instrumentation::retransform on
one of the classes in A after registering a class file transformer. Once
the retransformation is triggered, I can now define a class in A. Of course
this is inefficient and I would rather open the jdk.internal.misc module
and use the "old" API instead.

For this reason, I argue that this rather restrained API is not convenient
while it does not add anything to security. Also, for the use case of
Mockito, this would neither be sufficient as Mockito sometimes redefines
classes and sometimes adds a subclass without retransforming. We would
rather have direct access to class definition once we are already running
with the privileges of a Java agent.

I would therefore suggest to add a method:

interface Instrumentation {
  Class defineClass(byte[] bytes, ProtectionDomain pd);
}

which can be implemented simply by delegating to jdk.internal.misc.Unsafe.

On a side note. Does JavaLangAccess::defineClass work with the bootstrap
class loader? I have not tried it but I always thought it was just an
access layer for the class loader API that cannot access the null value.

Thanks for considering this use case!
Best regards, Rafael

2018-04-15 8:23 GMT+02:00 mandy chung :

> Background:
>
> Java agents support both load time and dynamic instrumentation.   At load
> time,
> the agent's ClassFileTransformer is invoked to transform class bytes.
> There is
> no Class objects at this time.  Dynamic instrumentation is when
> redefineClasses
> or retransformClasses is used to redefine an existing loaded class.  The
> ClassFileTransformer is invoked with class bytes where the Class object is
> present.
>
> Java agent doing instrumentation needs a means to define auxiliary classes
> that are visible and accessible to the instrumented class.  Existing agents
> have been using sun.misc.Unsafe::defineClass to define aux classes directly
> or accessing protected ClassLoader::defineClass method with setAccessible
> to
> suppress the language access check (see [1] where this issue was brought
> up).
>
> Instrumentation::appendToBootstrapClassLoaderSearch and
> appendToSystemClassLoaderSearch
> APIs are existing means to supply additional classes.  It's too limited
> for example it can't inject a class in the same runtime package as the
> class
> being transformed.
>
> Proposal:
>
> This proposes to add a new ClassFileTransformer.transform method taking
> additional ClassDefiner parameter.  A transformer can define additional
> classes during the transformation process, i.e.
> when ClassFileTransformer::transform is invoked.  Some details:
>
> 1. ClassDefiner::defineClass defines a class in the same runtime package
>as the class being transformed.
> 2. The class is defined in the same thread as the transformers are being
>invoked.   ClassDefiner::defineClass returns Class object directly
>before the transformed class is defined.
> 3. No transformation is applied to classes defined by
> ClassDefiner::defineClass.
>
> The first prototype we did is to collect the auxiliary classes and define
> them  until all transformers are invoked and have these aux classes to go
> through the transformation pipeline.  Several complicated issues would
> need to be resolved for example timing whether the auxiliary classes
> should
> be defined before the transformed class (otherwise a potential race where
> some other thread references the transformed class and cause the code to
> execute that in turn reference the auxiliary classes.  The current
> implementation has a native reentrancy check that ensure one class is being
> transformed to avoid potential circularity issues.  This may need JVM TI
> support to be reliable.
>
> This proposal would allow java agents to migrate from internal API and
> ClassDefiner to be enhanced in the future.
>
> Webrev:
>http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/
>
> Mandy
> [1] http://mail.openjdk.java.net/pipermail/jdk-dev/2018-
> January/000405.html
>

Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary classes

[Rafael Winterhalter]

Hei Mandy,
Lookup::defineClass would always be an alternative but it would require me
to open the class first. If the instrumented type can read the module with
the callback but its module was not opened, this would not help me much,
unfortunately. Also, I could not resolve this lookup as the class in
question is not necessarily loaded at this point.
Best regards, Rafael

2018-04-17 9:28 GMT+02:00 mandy chung <mandy.ch...@oracle.com>:

> Hi Rafael,
>
> I see that mocking/proxying/testing framework should be looked at
> separately since its requirements and approaches can be different than tool
> agents.
>
> On 4/17/18 5:06 AM, Rafael Winterhalter wrote:
>
> Hei Mandy,
>
> I have looked into several Java agents that I have worked on and for many
> of them, this API does unfortunately not supply sufficient access. I would
> therefore still prefer a method Instrumentation::defineClass.
>
> The problem is that some agents need to define classes in other packages
> then in that of the instrumented class. For example, I might need to
> enhance a library that defines a set of callback classes in package A. All
> these classes share a common super class with a package-private
> constructor. I want to instrument some class in package B to use a callback
> that the library does not supply and need to add a new callback class to A.
> This is not possible using the current API.
>
>
> Are these callback classes made available statically?  or just dynamically
> defining additional class as needed?  Is Lookup::defineClass an alternative
> if you get a hold of common super class in A?
>
> I could however achieve do so by calling Instrumentation::retransform on
> one of the classes in A after registering a class file transformer. Once
> the retransformation is triggered, I can now define a class in A. Of course
> this is inefficient and I would rather open the jdk.internal.misc module
> and use the "old" API instead.
>
> For this reason, I argue that this rather restrained API is not convenient
> while it does not add anything to security. Also, for the use case of
> Mockito, this would neither be sufficient as Mockito sometimes redefines
> classes and sometimes adds a subclass without retransforming. We would
> rather have direct access to class definition once we are already running
> with the privileges of a Java agent.
>
> I would therefore suggest to add a method:
>
> interface Instrumentation {
>   Class defineClass(byte[] bytes, ProtectionDomain pd);
> }
>
> which can be implemented simply by delegating to jdk.internal.misc.Unsafe.
>
> On a side note. Does JavaLangAccess::defineClass work with the bootstrap
> class loader? I have not tried it but I always thought it was just an
> access layer for the class loader API that cannot access the null value.
>
>
> The JVM entry point does allow null loader.
>
> Mandy
>
>
> Thanks for considering this use case!
> Best regards, Rafael
>
> 2018-04-15 8:23 GMT+02:00 mandy chung <mandy.ch...@oracle.com>:
>
>> Background:
>>
>> Java agents support both load time and dynamic instrumentation.   At load
>> time,
>> the agent's ClassFileTransformer is invoked to transform class bytes.
>> There is
>> no Class objects at this time.  Dynamic instrumentation is when
>> redefineClasses
>> or retransformClasses is used to redefine an existing loaded class.  The
>> ClassFileTransformer is invoked with class bytes where the Class object
>> is present.
>>
>> Java agent doing instrumentation needs a means to define auxiliary
>> classes
>> that are visible and accessible to the instrumented class.  Existing
>> agents
>> have been using sun.misc.Unsafe::defineClass to define aux classes
>> directly
>> or accessing protected ClassLoader::defineClass method with setAccessible
>> to
>> suppress the language access check (see [1] where this issue was brought
>> up).
>>
>> Instrumentation::appendToBootstrapClassLoaderSearch and
>> appendToSystemClassLoaderSearch
>> APIs are existing means to supply additional classes.  It's too limited
>> for example it can't inject a class in the same runtime package as the
>> class
>> being transformed.
>>
>> Proposal:
>>
>> This proposes to add a new ClassFileTransformer.transform method taking
>> additional ClassDefiner parameter.  A transformer can define additional
>> classes during the transformation process, i.e.
>> when ClassFileTransformer::transform is invoked.  Some details:
>>
>> 1. ClassDefiner::defineClass defines a class in the same runtime package
>>as the class being transformed.
>> 2. The class is

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

[Rafael Winterhalter]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 wrote:

>> To allow agents the definition of auxiliary classes, an API is needed to 
>> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or 
>> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed 
>> from `sun.misc.Unsafe`.
>
> Rafael Winterhalter has refreshed the contents of this pull request, and 
> previous commits have been removed. The incremental views will show 
> differences compared to the previous content of the PR. The pull request 
> contains one new commit since the last revision:
> 
>   8200559: Java agents doing instrumentation need a means to define auxiliary 
> classes

This won't work as agents are loaded by the system class loader, not the
boot loader.

Peter Levart ***@***.***> schrieb am Mo., 19. Apr. 2021,
11:02:

> From: Alan Bateman
>
> JDK-8200559 is about defining auxiliary classes in the same runtime
> package at load-time whereas I think the proposal in this PR is adding an
> unrestricted defineClass to the Instrumentation class. I think this will
> require a lot of discussion as there are significant issues and concerns
> here. An unrestricted defineClass might be okay for tool/java agents when
> started from the command line with -javaagent but only if the
> Instrumentation object is never ever leaked to library or application code.
> It could potentially be part of a large effort to reduce the capabilities
> of agents loaded via the attach mechanism. More generally I think we need
> clearer separation of the requirements of tool agents from the requirement
> of framework/libraries that want to inject proxy and other classes at
> runtime.
>
> I think it would be easy to limit the use of this Instrumentation method
> to the agent code as agent classes are loaded by the bootstrap classloader.
> Simply make the method implementation caller-sensitive and check the caller
> is from bootstrap class loader. This way Instrumentation instance escaped
> to application code would not give that code any ability to define
> arbitrary classes.
>
> Good enough?
>
> Peter
>
> Separately, the proposal in JEP 410 is to terminally deprecate
> ProtectionDomain.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/openjdk/jdk/pull/3546#issuecomment-822302347>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABCIA4EYHK5OHTDAN3FKYULTJPWS5ANCNFSM43BSDEGQ>
> .
>

-

PR: https://git.openjdk.java.net/jdk/pull/3546

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

[Rafael Winterhalter]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 wrote:

>> To allow agents the definition of auxiliary classes, an API is needed to 
>> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or 
>> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed 
>> from `sun.misc.Unsafe`.
>
> Rafael Winterhalter has refreshed the contents of this pull request, and 
> previous commits have been removed. The incremental views will show 
> differences compared to the previous content of the PR. The pull request 
> contains one new commit since the last revision:
> 
>   8200559: Java agents doing instrumentation need a means to define auxiliary 
> classes

At the moment, it is required for root to switch to the user that owns the
JVM process as the domain socket is only accessible to that user to avoid
that users without access to the JVM can inject themselves into a JVM. I am
not sure if operations teams would be thrilled to have a monitoring agent
required to run as root, even in these times of Kubernetes.

I mainly have two comments:

1. The problem is the possibility of self-attach. I think this is the
problem to solve, a library getting agent privileges without being an
agent. I think this should be prevented while dynamic attach should
continue to be possible in today's format. It has proven to be so useful,
it would be a shame if the current tooling convenience would disappear from
the JVM. As it's my understanding, JNI is supposed to be restricted in the
future, in line with Panama. Without this restriction, JNI already allows
for random class definition, for example, which similarly to an agent
offers surpassing the majority of JVM restrictions. The second restriction
would be a control to restrict how a JVM process starts new processes. I
think both are reasonable restrictions for a library to face which require
explicit enabling. Especially with the security manager on it's way out,
certain capabilities should be rethought to begin with. If both are no
longer freely available, self-attachment is no longer possible anyways and
dynamic agents could retain their capabilities.

2. The question of introducing an Instrumentation::defineClass method is
fully independent of that first question. If a dynamic agent was to be
restricted, the method could reject classloader/package combinations for
dynamically loaded agents the same way that
Instrumentation::retransformClasses would need to. At the same time,
introducing the method would allow agents to move to an official API with a
Java 17 baseline which will be the next long-standing base line. I fully
understand it needs a thorough discussion but it is a less complicated
problem then (1) and could therefore be decided prior to having found a
satisfactory solution for it.

Am Mo., 19. Apr. 2021 um 16:07 Uhr schrieb Peter Levart <
***@***.***>:

> an application or library can use the attach mechanism (directly or via
> the attach API in a child VM) to load an agent and leak the Instrumentation
> object. This is the genie that somehow needs to be put back in its bottle.
> One approach that I mentioned here to create is a less powerful
> Instrumentation object for untrusted agents. Trusted agents would continue
> to the full-power
>
> I hear Rafael that dynamic attach is important to support monitoring and
> instrumenting large numbers of JVMs with no preparations (i.e. without
> issueing special command-line options to enable it). As I understand,
> current attach mechanism is designed to allow a process running under the
> same UID as the JVM or under root to attach to the JVM.
>
> What if this dynamic attach mechanism was modified so that only a process
> running under root could dynamically attach to the JVM? Old behavior would
> be enabled by special command line option, so by default, dynamic attach
> would be limited to tools running under root. Rafael mentions discovery,
> monitoring and instrumenting large number of JVMs running on hosts, so if
> one such tool has to attach to different JVMs running under different UIDs,
> it has to run as root now anyway.
>
> With such default "secure" dynamic attach and when the JVM is not running
> as root (which is a recommended security practice anyway), a library in
> such JVM could not attach back to the same JVM even through spawning
> sub-processes.
>
> How to achieve such "secure" dynamic attach? One way that comes to mind is
> a modified handshake. Currently, I think at least on Linux, the tool that
> wishes to attach to the JVM searches for a special UNIX socket (
> $PWD/.java_pid, /tmp/.java_pid) and if not found, creates a
> special attach file ($PWD/.attach_pid, /tmp/.attach_pid) to
> signal the JVM to create a listening UNIX socket under mentioned special
> path, then it connects to th

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxilary classes

[Rafael Winterhalter]

As for the need to inject classes into 'new' packages, in Mockito we solve
this easily by 'claiming' a package by defining a static dummy class within
it which we then use as a hook for injection using a lookup. This works
well for us, and I do of course not know the explicit requirements of
others, but personally, I do not see a big need for this functionality
otherwise.

I remember the discussions about the dynamic agent attachment rather well,
it cuts into my line of work. The main (!) reason why dynamic attachment is
so popular is that it allows monitoring a JVM without additional
configuration. In this light, it does not make much of a difference if a
user has to set a special parameter to allow for dynamic attachment or if
the user has to set up the agent parameter itself, both add about the same
costs to operations. This is the cost you want to avoid by dynamic
attachment in the first place. Imagine an enterprise with thousands of JVM
deployments which are managed by different teams in different styles, with
different technical understanding relying on long chains of communication.
It can take years to roll out a tracing tool that requires explicit
configuration in an org like this. Thanks to dynamic attachment, all you
need is to drop an application per host which discovers all running JVMs
and attaches to them. A tracing tool that is missing only a few VMs by
misconfiguration will render a very incomplete picture of the tracing
state, making it an expensive toy. Easily avoiding such simple misses is an
important selling point for the JVM compared to other platforms where this
is not possible. I know this problem does not sound like much, it's
overcomeable, but orgs will not change, and reducing the capabilities of
dynamic agents would take much more out of the JVM's attractiveness than
the added security and consistency would bring to it.

As for the proposed API, I understand that it needs thought, my hope is
still that Java 17 would allow to write agents without Unsafe, since 17
will be a baseline for many years to come. I'd like to stress out that the
proposed API only encapsulates something that is already implicitly
possible today if one puts in the work.

I think that the problem you want to solve is rather that JVMs should not
attach to themselves to get hold of an instance of Instrumentation for
their own JVM. In this context, one would need to restrict the use of JNI
and prevent that a JVM starts new (Java) processes. I think it would be
better to tackle the issue from this angle rather than taking away a
feature that significantly adds to the JVMs attractiveness.

Am So., 18. Apr. 2021 um 18:24 Uhr schrieb Alan Bateman <
alan.bate...@oracle.com>:

> On 16/04/2021 21:09, Rafael Winterhalter wrote:
> > I have never seen a need for a non-agent to define a class in a
> > non-existing package. Injection is typically required if you want to work
> > with package-private types or methods which is really only relevant for
> the
> > Spring framework, but even there I do not think it's such a big area that
> > it cannot be addressed. Non-agent code generation is typically the use of
> > proxies where the code generation framework is sharing a class loader
> with
> > the user code and there is normally a way to weave it. Agents on the
> other
> > hand have to deal with unknown class loader hierarchies and it can be
> > necessary to inject code into a common class loader parent without
> > instrumenting classes in this loader or even knowing any classes of this
> > loader.
> Yes, the injection of proxy classes and the like is mainly the same
> run-time package as an existing class and these are the use-cases that
> Lookup.defineClass and Lookup.defineHiddenClass are intended for. There
> has been a few requests for defining proxy classes into "new/empty
> packages" but has a few challenges, like extending the set of packages
> in a named module. In general I wouldn't expect Java agents, which was
> intending for tools, to be using Lookup objects.
>
>
> > I have never heard about a 'discussion [that] will eventually lead into
> > putting at least some restrictions on agents loaded into a running VM'
> and
> > as a heavy user and someone who has helped to write a long row of
> > commercial agents and followed them into their use in production and who
> > has seen how helpful they are in reducing deployment complexity, I can
> only
> > hope that you will change your mind on this.
> > :
> >
> > That said, even if it was restricted in the future, this would mean that
> > some of the Instrumentation API methods will throw exceptions in the
> > future. There would not be much difference if an introduced defineClass
> > method would do the same.
> There was fuss on this topic during JDK 9. I'll try to

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

[Rafael Winterhalter]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 wrote:

>> To allow agents the definition of auxiliary classes, an API is needed to 
>> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or 
>> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed 
>> from `sun.misc.Unsafe`.
>
> Rafael Winterhalter has refreshed the contents of this pull request, and 
> previous commits have been removed. The incremental views will show 
> differences compared to the previous content of the PR. The pull request 
> contains one new commit since the last revision:
> 
>   8200559: Java agents doing instrumentation need a means to define auxiliary 
> classes

I fully understand your concerns about ByteBuddyAgent.install(). It is
simply a convenience for something that can be meaningful in some contexts
where I prefer offering a simple API. I use it mainly for two purposes:

a) For testing Java agents and integrations against Instrumentation within
the current VM when tests are triggered by tools that do not support
javaagents, also because builds do not bundle jars until after tests are
executed.

b) For purposefully "hacky" test libraries like Mockito that need agent
capabilities without this being meant to be used in production
environments. I have earlier proposed to offer a "jdk.test" module that
offers the  Instrumentation instance via a simple API similar to Byte
Buddy's. The JVM would not load this module unless requested on the command
line. Build tools like Maven's surefire or Gradle's testrunner could then
standardize on loading this module as a convention to give access to this
test module by default such that libraries like Mockito could continue to
function out of the box without the libraries functioning on a standard VM
without extra configuration. As far as I know, mainly test libraries need
this API. This would also emphasise that Mockito and others are meant for
testing and fewer people would abuse it for production applications. People
would also have an explicit means of running a JVM for a production
application or for executing a test.

As for adding the API, my thought is that if the Instrumentation API were
to throw exceptions on some methods/arguments for dynamic agents in the
future, for example for retransformClasses(Object.class), this breaking
change would then simply extend to the proposed "defineClass" method. In
this sense, the Instrumentation API already assumes full power, I find it
not problematic to add the missing bit to this API even if it was
restricted in the future in the same spirit as other methods of the API
would be.

I mentioned JNI as it is a well-known approach to defining a class today,
using a minimal native binding to an interface that directly calls down to
JNI's:

jclass DefineClass(JNIEnv *env, const char *name, jobject loader, const
jbyte *buf, jsize bufLen);

This interface can then simply be used to define any class just as I
propse, even when not writing an agent or attaching. This method makes
class definitions also already trivial for JVMTI agents compared to Java
agents. Unless restricting JNI, the defineClass method is already a low
hanging fruit, but at the cost of having to maintain a tiny bit of native
code. I'd rather see this avoided and a standard API being offered to
agents up to the time that Panama is in place and a JNI restriction is
possibly also included. As a bonus: Once JNI is restricted, Byte Buddy's
"install" would no longer work unless self-attachment (or JNI) is
explicitly allowed. The emulation already requires to run native code while
the Virtual Machine API explicitly checks for the process id of the current
VM against the one that is targeted. With both disabled, self-attachment
would no longer be practically be possible without needing to prune the
capabilities of dynamic agents which is what I understand would be the
desired effect.

>From this viewpoint, I think that adding Instrumentation::defineClass
method does no harm compared to the status quo. And on the upside, it gives
agents an API to migrate to, avoiding the last need of using unsafe. To
make the JVM a safe platform, binding native code would anyways need
restriction and this would then also solve the problem of dynamic agents
attaching from the same VM being used in libraries. This would in my eyes
be the cleanest solution to the self-attachment problem without disturbing
the existing landscape of dynamic agents. To run Mockito, one would then
instead configure Maven surefire or Gradle to run the JVM with
-Djdk.attach.allowAttachSelf=true. Ideally, some "jdk.test" module would be
added at some point, to avoid the overhead of self-attachment, but I think
this better fits into separate debate.

Am Di., 20. Apr. 2021 um 15:38 Uhr schrieb mlbridge[bot] <
***@***.***>:

> *Mailing list message from Alan Bateman ***@***.**

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxilary classes

[Rafael Winterhalter]

Not by my understanding. A suitable lookup requires a loaded class for the
package. A Java agent might however not provide a handle for a class that
is not yet loaded. Or how would you suggest to approach this?

Am Fr., 16. Apr. 2021 um 16:21 Uhr schrieb Remi Forax :

> - Mail original -
> > De: "Rafael Winterhalter" 
> > À: "core-libs-dev" ,
> "serviceability-dev" 
> > Envoyé: Vendredi 16 Avril 2021 15:52:07
> > Objet: RFR: 8200559: Java agents doing instrumentation need a means to
> define auxilary classes
>
> > To allow agents the definition of auxiliary classes, an API is needed to
> allow
> > this. Currently, this is often achieved by using `sun.misc.Unsafe` or
> > `jdk.internal.misc.Unsafe` ever since the `defineClass` method was
> removed from
> > `sun.misc.Unsafe`.
>
> You can already use Lookup.defineClass() + privateLookupIn() +
> Instrumentation.redefineModule() for that ?
>
> Rémi
>
> >
> > -
> >
> > Commit messages:
> > - 8200559: Java agents doing instrumentation need a means to define
> auxiliary
> > classes
> >
> > Changes: https://git.openjdk.java.net/jdk/pull/3546/files
> > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3546=00
> >  Issue: https://bugs.openjdk.java.net/browse/JDK-8200559
> >  Stats: 185 lines in 4 files changed: 185 ins; 0 del; 0 mod
> >  Patch: https://git.openjdk.java.net/jdk/pull/3546.diff
> >  Fetch: git fetch https://git.openjdk.java.net/jdk
> pull/3546/head:pull/3546
> >
> > PR: https://git.openjdk.java.net/jdk/pull/3546
>

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxilary classes

[Rafael Winterhalter]

On Fri, 16 Apr 2021 13:44:16 GMT, Rafael Winterhalter 
 wrote:

> To allow agents the definition of auxiliary classes, an API is needed to 
> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or 
> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed 
> from `sun.misc.Unsafe`.

The ticket was created as a reaction to a [write-up I made in January 
2018](http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January/000405.html). 
I certainly did not intend to limit the scope to same-package class definitions 
for instrumented classes, and even for those I do not think a 
package-restricted API would be sufficient as I outlined in the comments to 
[JDK-8200559](https://bugs.openjdk.java.net/browse/JDK-8200559).

I will try to make my case on the mailing list. I hoped this could get resolved 
within the release of Java 17 as this would make it possible to write agents 
without use of Unsafe API to support Java 17 and later. Since agents often are 
supplementary to a broad range of Java applications, the LTS release will 
likely be an important support boundary for years and years to come.

You surely mean JEP 411 when mentioning `ProtectionDomain`? The JEP mentions

> We will not deprecate some classes in the java.security package that are 
> related to the Security Manager, for varying reasons: [...] ProtectionDomain 
> — Several significant APIs depend on ProtectionDomain, e.g., 
> ClassLoader::defineClass and Class::getProtectionDomain. ProtectionDomain 
> also has value independent of the Security Manager since it contains the 
> CodeSource of a class.

Also, since this is still a proposal, I would believe that APIs that are 
implemented before JEP 411 is realized should support `ProtectionDomain` for 
users still supporting the security manager in the transition time.

-

PR: https://git.openjdk.java.net/jdk/pull/3546

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxilary classes

[Rafael Winterhalter]

This would be a problem, however. Since there is currently no official way
of defining a class, and since Java agents do not control the class loading
order, if a class was loaded for the first time, you could for example not
add a field with a type of an auxiliary class that you had planned to
inject. A class being loaded is normally the first opportunity for a Java
agent and if no witness class is available at this point, using a method
handle is no option. Since it is difficult to know if such a witness class
is available in the general case, it would also add quite a performance and
managerial toll on agent authors. I would hope that an API equally
convenient to today's unsafe options could be added.

Am Fr., 16. Apr. 2021 um 18:35 Uhr schrieb :

>
>
> --
>
> *De: *"Rafael Winterhalter" 
> *À: *"Remi Forax" 
> *Cc: *"Rafael Winterhalter" ,
> "core-libs-dev" , "serviceability-dev" <
> serviceability-dev@openjdk.java.net>
> *Envoyé: *Vendredi 16 Avril 2021 18:27:46
> *Objet: *Re: RFR: 8200559: Java agents doing instrumentation need a means
> to define auxilary classes
>
> Not by my understanding. A suitable lookup requires a loaded class for the
> package. A Java agent might however not provide a handle for a class that
> is not yet loaded. Or how would you suggest to approach this ?
>
>
> yes, you need a witness class in the package you want to define a new
> class.
> Apart if you load classes in the unamed module, you can not load a class
> in a non existing package anyway (apart if you generate your own
> module-info),
> so you need at least a dummy class to define a package, so you can use it
> to get a Lookup.
>
> Rémi
>
>
>
> Am Fr., 16. Apr. 2021 um 16:21 Uhr schrieb Remi Forax :
>
>> - Mail original -
>> > De: "Rafael Winterhalter" 
>> > À: "core-libs-dev" ,
>> "serviceability-dev" 
>> > Envoyé: Vendredi 16 Avril 2021 15:52:07
>> > Objet: RFR: 8200559: Java agents doing instrumentation need a means to
>> define auxilary classes
>>
>> > To allow agents the definition of auxiliary classes, an API is needed
>> to allow
>> > this. Currently, this is often achieved by using `sun.misc.Unsafe` or
>> > `jdk.internal.misc.Unsafe` ever since the `defineClass` method was
>> removed from
>> > `sun.misc.Unsafe`.
>>
>> You can already use Lookup.defineClass() + privateLookupIn() +
>> Instrumentation.redefineModule() for that ?
>>
>> Rémi
>>
>> >
>> > -
>> >
>> > Commit messages:
>> > - 8200559: Java agents doing instrumentation need a means to define
>> auxiliary
>> > classes
>> >
>> > Changes: https://git.openjdk.java.net/jdk/pull/3546/files
>> > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3546=00
>> >  Issue: https://bugs.openjdk.java.net/browse/JDK-8200559
>> >  Stats: 185 lines in 4 files changed: 185 ins; 0 del; 0 mod
>> >  Patch: https://git.openjdk.java.net/jdk/pull/3546.diff
>> >  Fetch: git fetch https://git.openjdk.java.net/jdk
>> pull/3546/head:pull/3546
>> >
>> > PR: https://git.openjdk.java.net/jdk/pull/3546
>>
>
>

RFR: 8200559: Java agents doing instrumentation need a means to define auxilary classes

[Rafael Winterhalter]

To allow agents the definition of auxiliary classes, an API is needed to allow 
this. Currently, this is often achieved by using `sun.misc.Unsafe` or 
`jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed from 
`sun.misc.Unsafe`.

-

Commit messages:
 - 8200559: Java agents doing instrumentation need a means to define auxiliary 
classes

Changes: https://git.openjdk.java.net/jdk/pull/3546/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3546=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8200559
  Stats: 185 lines in 4 files changed: 185 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3546.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3546/head:pull/3546

PR: https://git.openjdk.java.net/jdk/pull/3546

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxilary classes

[Rafael Winterhalter]

On Fri, 16 Apr 2021 13:44:16 GMT, Rafael Winterhalter 
 wrote:

> To allow agents the definition of auxiliary classes, an API is needed to 
> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or 
> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed 
> from `sun.misc.Unsafe`.

I have never seen a need for a non-agent to define a class in a
non-existing package. Injection is typically required if you want to work
with package-private types or methods which is really only relevant for the
Spring framework, but even there I do not think it's such a big area that
it cannot be addressed. Non-agent code generation is typically the use of
proxies where the code generation framework is sharing a class loader with
the user code and there is normally a way to weave it. Agents on the other
hand have to deal with unknown class loader hierarchies and it can be
necessary to inject code into a common class loader parent without
instrumenting classes in this loader or even knowing any classes of this
loader. It's really agents where this is required and therefore
Instrumentation is a good target for such an addition.

I have never heard about a 'discussion [that] will eventually lead into
putting at least some restrictions on agents loaded into a running VM' and
as a heavy user and someone who has helped to write a long row of
commercial agents and followed them into their use in production and who
has seen how helpful they are in reducing deployment complexity, I can only
hope that you will change your mind on this. Dynamically attached agents
must already run as the same user as the target process. If you are
concerned about agents 'illegally intruding' your Java process, I'd say you
have bigger security issues at hand and it is already possible to disable
dynamic attachment if you want to avoid it. Dynamic attachment has become
pretty much the default approach for a lot of Java tooling in production
environments. It has proven to be very convenient if a for example a
tracing tool is scanning Java processes to attach to them, rather than
requiring the deployment operators to be explicitly set up command line
arguments which are easily forgotten if they only need to be added in some
environments. This reduction in complexity for operations has literally
saved millions of dollars to Java users and Oracle customers. This makes
Java a popular choice, especially compared to languages such as Go where
this is naturally not possible. It's easy and there is no record of this
feature doing any harm. I would not see any good reason to restrict this by
default.

That said, even if it was restricted in the future, this would mean that
some of the Instrumentation API methods will throw exceptions in the
future. There would not be much difference if an introduced defineClass
method would do the same.

Am Fr., 16. Apr. 2021 um 19:33 Uhr schrieb mlbridge[bot] <
***@***.***>:

> *Mailing list message from Alan Bateman ***@***.***> on
> core-libs-dev ***@***.***>:*
>
> On 16/04/2021 17:40, Rafael Winterhalter wrote:
>
> :
> I will try to make my case on the mailing list. I hoped this could get
> resolved within the release of Java 17 as this would make it possible to
> write agents without use of Unsafe API to support Java 17 and later. Since
> agents often are supplementary to a broad range of Java applications, the
> LTS release will likely be an important support boundary for years and
> years to come.
>
> "are supplementary to a board range of Java applications" is part of the
> concern with the proposal. If possible, it would be good if the write-up
> could separate the requirements for injection/instrumentation by
> frameworks at runtime from the requirements of tool agents. If the
> requirements cover testing time and mocking then it would useful to
> separate those too.
>
> Just to add to R?mi's comment: For frameworks/libraries, the
> Lookup.defineClass and defineHiddenClass APIs are to define classes in
> the same run-time package as the Lookup class. There isn't any API for
> libraries/frameworks to define class in a "new run-time package".
> There's a chunky project there. Part of it is the Lookup API itself,
> part of is that there isn't an exposed way to extend the set of packages
> in a named module. Mandy has done some exploration on this topic and may
> be able to say a bit more about this.
>
> On Java agents, then I think the discussion will eventually lead into
> putting at least some restrictions on agents loaded into a running VM.
> Agents started on the command line with -javaagent are all-powerful but
> maybe agents loaded into a target VM get a restricted Instrumentation
> object that cannot redefine modules or retransform classes in named
> modules. The narrower requirement for agents doing load time
> 

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxilary classes [v2]

[Rafael Winterhalter]

> To allow agents the definition of auxiliary classes, an API is needed to 
> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or 
> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed 
> from `sun.misc.Unsafe`.

Rafael Winterhalter has refreshed the contents of this pull request, and 
previous commits have been removed. The incremental views will show differences 
compared to the previous content of the PR. The pull request contains one new 
commit since the last revision:

  8200559: Java agents doing instrumentation need a means to define auxiliary 
classes

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3546/files
  - new: https://git.openjdk.java.net/jdk/pull/3546/files/362efc82..2ef21598

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3546=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3546=00-01

  Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3546.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3546/head:pull/3546

PR: https://git.openjdk.java.net/jdk/pull/3546

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxilary classes

[Rafael Winterhalter]

I have never seen a need for a non-agent to define a class in a
non-existing package. Injection is typically required if you want to work
with package-private types or methods which is really only relevant for the
Spring framework, but even there I do not think it's such a big area that
it cannot be addressed. Non-agent code generation is typically the use of
proxies where the code generation framework is sharing a class loader with
the user code and there is normally a way to weave it. Agents on the other
hand have to deal with unknown class loader hierarchies and it can be
necessary to inject code into a common class loader parent without
instrumenting classes in this loader or even knowing any classes of this
loader. It's really agents where this is required and therefore
Instrumentation is a good target for such an addition.

I have never heard about a 'discussion [that] will eventually lead into
putting at least some restrictions on agents loaded into a running VM' and
as a heavy user and someone who has helped to write a long row of
commercial agents and followed them into their use in production and who
has seen how helpful they are in reducing deployment complexity, I can only
hope that you will change your mind on this. Dynamically attached agents
must already run as the same user as the target process. If you are
concerned about agents 'illegally intruding' your Java process, I'd say you
have bigger security issues at hand and it is already possible to disable
dynamic attachment if you want to avoid it. Dynamic attachment has become
pretty much the default approach for a lot of Java tooling in production
environments. It has proven to be very convenient if a for example a
tracing tool is scanning Java processes to attach to them, rather than
requiring the deployment operators to be explicitly set up command line
arguments which are easily forgotten if they only need to be added in some
environments. This reduction in complexity for operations has literally
saved millions of dollars to Java users and Oracle customers. This makes
Java a popular choice, especially compared to languages such as Go where
this is naturally not possible. It's easy and there is no record of this
feature doing any harm. I would not see any good reason to restrict this by
default.

That said, even if it was restricted in the future, this would mean that
some of the Instrumentation API methods will throw exceptions in the
future. There would not be much difference if an introduced defineClass
method would do the same.

Am Fr., 16. Apr. 2021 um 19:31 Uhr schrieb Alan Bateman <
alan.bate...@oracle.com>:

> On 16/04/2021 17:40, Rafael Winterhalter wrote:
> > :
> > I will try to make my case on the mailing list. I hoped this could get
> resolved within the release of Java 17 as this would make it possible to
> write agents without use of Unsafe API to support Java 17 and later. Since
> agents often are supplementary to a broad range of Java applications, the
> LTS release will likely be an important support boundary for years and
> years to come.
> >
> "are supplementary to a board range of Java applications" is part of the
> concern with the proposal. If possible, it would be good if the write-up
> could separate the requirements for injection/instrumentation by
> frameworks at runtime from the requirements of tool agents. If the
> requirements cover testing time and mocking then it would useful to
> separate those too.
>
> Just to add to Rémi's comment: For frameworks/libraries, the
> Lookup.defineClass and defineHiddenClass APIs are to define classes in
> the same run-time package as the Lookup class. There isn't any API for
> libraries/frameworks to define class in a "new run-time package".
> There's a chunky project there. Part of it is the Lookup API itself,
> part of is that there isn't an exposed way to extend the set of packages
> in a named module. Mandy has done some exploration on this topic and may
> be able to say a bit more about this.
>
> On Java agents, then I think the discussion will eventually lead into
> putting at least some restrictions on agents loaded into a running VM.
> Agents started on the command line with -javaagent are all-powerful but
> maybe agents loaded into a target VM get a restricted Instrumentation
> object that cannot redefine modules or retransform classes in named
> modules. The narrower requirement for agents doing load time
> instrumentation to define auxiliary classes in the same package as the
> class being loaded fits with the intent of the original API and I don't
> think is controversial.
>
> -Alan
>

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

[Rafael Winterhalter]

On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter 
 wrote:

>> To allow agents the definition of auxiliary classes, an API is needed to 
>> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or 
>> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed 
>> from `sun.misc.Unsafe`.
>
> Rafael Winterhalter has refreshed the contents of this pull request, and 
> previous commits have been removed. The incremental views will show 
> differences compared to the previous content of the PR.

Setting '-javaagent' is mainly an operations problem. Many build tools do
not allow to declare a test dependency this way as the life cycles are not
laid out for it, the internal repository location might be machine
dependent, for example, and it's difficult to point to a specific folder
and file reliably. In this case, I'd say it would be easier to specify a
parameter in the sense of '-Djdk.attach.allowAttachSelf=true' as it is a
static value. This would however only work for build tools that initiate a
new VM for running tests which is not overly attractive for simple builds
due to the overhead. Of course, Maven, Gradle and similar tools could set
this parameter by default for their own JVM, that part is likely
overcomeable but it will certainly create confusion about how to run
libraries that are omnipresent today and make the JVM ecosystem less
approachable.

What bothers me more is the tooling perspective for non-self-attached
agents. For example, Aaeron offers a Java agent that adds plenty of debug
logging to relevant lines of code. This affects method size and so forth,
with Aaeron as a high-performance tool for banking and finance which is
written very consciously with regards to the JIT, adding it directly was
not feasible. Normally this logging is therefore thrown into a VM in
retrospect, once an application starts failing and is already taken off the
load balancer. For such a post-mortem, it would be rather annoying to
realize that a JVM cannot be attached to with full capabilities if you
forgot to set some flag. And often you did of course not consider the VM
instance to fail, sometimes it takes months to get a JVM into this buggy
state. This would be fairly inconvenient to face.

Therefore, I really hope that the dynamic attach from 'outside' the VM will
survive without imposing limits and that rather the self-attachment problem
will be targeted as such. When I mention a 'jdk.test' module in the Mockito
context, I am also rather hoping to improve performance compared to
convenience. The problem with '-Djdk.attach.allowAttachSelf=true' is that
you still need to start a new VM etc. Since Java 9, running single tests
with Mockito has for example become much slower compared to Java 8. Despite
the startup performance improvements in the JVM. If one could avoid the
location-bound '-javaagent:...', but get the Instrumentation instance
directly, I think this would render a real performance improvement in
actual execution scenarios.

Am Mi., 21. Apr. 2021 um 20:42 Uhr schrieb mlbridge[bot] <
***@***.***>:

> *Mailing list message from Alan Bateman ***@***.***> on
> core-libs-dev ***@***.***>:*
>
> On 20/04/2021 21:26, Rafael Winterhalter wrote:
>
> I have earlier proposed to offer a "jdk.test" module that
> offers the Instrumentation instance via a simple API similar to Byte
> Buddy's. The JVM would not load this module unless requested on the command
> line. Build tools like Maven's surefire or Gradle's testrunner could then
> standardize on loading this module as a convention to give access to this
> test module by default such that libraries like Mockito could continue to
> function out of the box without the libraries functioning on a standard VM
> without extra configuration. As far as I know, mainly test libraries need
> this API. This would also emphasise that Mockito and others are meant for
> testing and fewer people would abuse it for production applications. People
> would also have an explicit means of running a JVM for a production
> application or for executing a test.
>
> Helping testing is good but a jdk.test module that hands out the
> Instrumentation object could be problematic. Is there a reason why the
> runner for Mockito and other mocking frameworks can't specify -javaagent
> when launching? I would expect at least some mocking scenarios to
> require load time transformations (to drop the final modifier from some
> API elements for example) so important to have the transformer set
> before classes are loaded.
>
> As for adding the API, my thought is that if the Instrumentation API were
> to throw exceptions on some methods/arguments for dynamic agents in the
> future, for example for retransformClasses(Object.class), this breaking
> change would then simply extend to the proposed "defi

Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]

[Rafael Winterhalter]

placed in the
> lock through the command
> line. That this can be circumvented today is irrelevant, as these
> workarounds will be *gradually*
> removed. I hope the operations people will be thrilled with the platform’s
> increased security and
> reduced maintenance pain when upgrading JDK versions, but either way, they
> should start
> preparing for the new reality sooner rather than later. Our goal, then,
> should be to make people’s life
> easier, but only under these constraints, that, at this point, should be
> taken as an axiom for the purpose
> of discussion.
>
> — Ron
>
> > On 20 Apr 2021, at 21:26, Rafael Winterhalter <
> winterhal...@openjdk.java.net> wrote:
> >
> > On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter <
> winterhal...@openjdk.org> wrote:
> >
> >>> To allow agents the definition of auxiliary classes, an API is needed
> to allow this. Currently, this is often achieved by using `sun.misc.Unsafe`
> or `jdk.internal.misc.Unsafe` ever since the `defineClass` method was
> removed from `sun.misc.Unsafe`.
> >>
> >> Rafael Winterhalter has refreshed the contents of this pull request,
> and previous commits have been removed. The incremental views will show
> differences compared to the previous content of the PR. The pull request
> contains one new commit since the last revision:
> >>
> >>  8200559: Java agents doing instrumentation need a means to define
> auxiliary classes
> >
> > I fully understand your concerns about ByteBuddyAgent.install(). It is
> > simply a convenience for something that can be meaningful in some
> contexts
> > where I prefer offering a simple API. I use it mainly for two purposes:
> >
> > a) For testing Java agents and integrations against Instrumentation
> within
> > the current VM when tests are triggered by tools that do not support
> > javaagents, also because builds do not bundle jars until after tests are
> > executed.
> >
> > b) For purposefully "hacky" test libraries like Mockito that need agent
> > capabilities without this being meant to be used in production
> > environments. I have earlier proposed to offer a "jdk.test" module that
> > offers the  Instrumentation instance via a simple API similar to Byte
> > Buddy's. The JVM would not load this module unless requested on the
> command
> > line. Build tools like Maven's surefire or Gradle's testrunner could then
> > standardize on loading this module as a convention to give access to this
> > test module by default such that libraries like Mockito could continue to
> > function out of the box without the libraries functioning on a standard
> VM
> > without extra configuration. As far as I know, mainly test libraries need
> > this API. This would also emphasise that Mockito and others are meant for
> > testing and fewer people would abuse it for production applications.
> People
> > would also have an explicit means of running a JVM for a production
> > application or for executing a test.
> >
> > As for adding the API, my thought is that if the Instrumentation API were
> > to throw exceptions on some methods/arguments for dynamic agents in the
> > future, for example for retransformClasses(Object.class), this breaking
> > change would then simply extend to the proposed "defineClass" method. In
> > this sense, the Instrumentation API already assumes full power, I find it
> > not problematic to add the missing bit to this API even if it was
> > restricted in the future in the same spirit as other methods of the API
> > would be.
> >
> > I mentioned JNI as it is a well-known approach to defining a class today,
> > using a minimal native binding to an interface that directly calls down
> to
> > JNI's:
> >
> > jclass DefineClass(JNIEnv *env, const char *name, jobject loader, const
> > jbyte *buf, jsize bufLen);
> >
> > This interface can then simply be used to define any class just as I
> > propse, even when not writing an agent or attaching. This method makes
> > class definitions also already trivial for JVMTI agents compared to Java
> > agents. Unless restricting JNI, the defineClass method is already a low
> > hanging fruit, but at the cost of having to maintain a tiny bit of native
> > code. I'd rather see this avoided and a standard API being offered to
> > agents up to the time that Panama is in place and a JNI restriction is
> > possibly also included. As a bonus: Once JNI is restricted, Byte Buddy's
> > "install" would no longer work unless self-attachment (or JNI) is
> > explicitly allowe