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

Wed Apr 18 20:28:27 UTC 2018

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.chung at 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.chung at 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.chung at 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 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
>>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180418/62598860/attachment-0001.html>