Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Andrew Dinn]

On 30/09/16 14:55, Rafael Winterhalter wrote:
> That does not work in the general case but only if the initiating class
> is also on the class path. A class can only be identified uniquely as a
> pair of name and class loader.
> 
> I experimented with this and ended up iterating over
> Instrumentation.getAllLoadedClasses which resulted in rather poor
> performance and ambiguity if there exist classes with the same name on
> different class loaders.

Yeah but don't you still have your ByteBuddy API class, the one which
initiates the load in the class path? Your agent can still push the
instance to that class by calling a setter it provides and it can hand
the instance out to classes which are allowed to use it.

So, then you would have to stop any old clients from calling your API to
retrieve an Instrumentation instance, for example by setting up a
security manager. Otherwise it makes no difference whether the
Instrumentation instance is accessible via a public static field or is
handed out on demand by your API. You have set up this circumstance by
providing a library which hands out an Instrumentation instance on demand.

What I don't understand is why you think migrating this into the JDK
changes things. You suggested that this would also need a security
manager to control access. If that's needed to safeguard the API on
Instrumentation then why is i tnot good enough to use the same mechanism
to restrict access to your ByteBuddy API class?

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Rafael Winterhalter]

That does not work in the general case but only if the initiating class is
also on the class path. A class can only be identified uniquely as a pair
of name and class loader.

I experimented with this and ended up iterating over
Instrumentation.getAllLoadedClasses which resulted in rather poor
performance and ambiguity if there exist classes with the same name on
different class loaders.

Again, I argue that this problem is therefore worth addressing.

Best regards, Rafael

2016-09-30 15:51 GMT+02:00 Andrew Dinn :

> On 30/09/16 14:27, Rafael Winterhalter wrote:
> > A Java agent ends up on the class path. The Byte Buddy agent (or any
> > similar library) basically adds a single class:
> >
> > package net.bytebuddy.agent;
> > public class Installer {
> >   public static volatile Instrumentation instrumentation;
> >   public static void agentMain(String argument, Instrumentation
> > instrumentation) {
> > Agent.instrumentation = instrumentation
> >   }
> > }
> >
> > Since the class is added using self-attachment, the agentmain method is
> > called by the VM and the field value is set. In order to keep the field
> > accessible, it needs to be public such that any class loader can call:
> >
> > Instrumentation instrumentation = (Instrumentation)
> > Class.forName("net.bytebuddy.agent.Installer", false,
> > ClassLoader.getSystemClassLoader()).getDeclaredField("
> instrumentation").get(null);
> >
> > Any library on the class path can now also call the above code without
> > requiring any priviledges as the Instrumentation instance is exposed
> > without constraints. Adding a proper method for reading an instance of
> > Instrumentation would prevent this.
>
> Well, that's easily fixed. Make the agent push the Instrumentation
> instance to the class which loaded the agent.
>
> For example, provide the name of the class and name of a public setter
> method as arguments to agentMain (and, if you want, a shared key to
> validate the set). Then get the agent to locate the class, lookup the
> setter method and hand over the instance.
>
> regards,
>
>
> Andrew Dinn
> ---
>
>

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Andrew Dinn]

On 30/09/16 14:27, Rafael Winterhalter wrote:
> A Java agent ends up on the class path. The Byte Buddy agent (or any
> similar library) basically adds a single class:
> 
> package net.bytebuddy.agent;
> public class Installer {
>   public static volatile Instrumentation instrumentation;
>   public static void agentMain(String argument, Instrumentation
> instrumentation) {
> Agent.instrumentation = instrumentation
>   }
> }
> 
> Since the class is added using self-attachment, the agentmain method is
> called by the VM and the field value is set. In order to keep the field
> accessible, it needs to be public such that any class loader can call:
> 
> Instrumentation instrumentation = (Instrumentation)
> Class.forName("net.bytebuddy.agent.Installer", false,
> ClassLoader.getSystemClassLoader()).getDeclaredField("instrumentation").get(null);
> 
> Any library on the class path can now also call the above code without
> requiring any priviledges as the Instrumentation instance is exposed
> without constraints. Adding a proper method for reading an instance of
> Instrumentation would prevent this.

Well, that's easily fixed. Make the agent push the Instrumentation
instance to the class which loaded the agent.

For example, provide the name of the class and name of a public setter
method as arguments to agentMain (and, if you want, a shared key to
validate the set). Then get the agent to locate the class, lookup the
setter method and hand over the instance.

regards,


Andrew Dinn
---

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Rafael Winterhalter]

A Java agent ends up on the class path. The Byte Buddy agent (or any
similar library) basically adds a single class:

package net.bytebuddy.agent;
public class Installer {
  public static volatile Instrumentation instrumentation;
  public static void agentMain(String argument, Instrumentation
instrumentation) {
Agent.instrumentation = instrumentation
  }
}

Since the class is added using self-attachment, the agentmain method is
called by the VM and the field value is set. In order to keep the field
accessible, it needs to be public such that any class loader can call:

Instrumentation instrumentation = (Instrumentation)
Class.forName("net.bytebuddy.agent.Installer", false,
ClassLoader.getSystemClassLoader()).getDeclaredField("instrumentation").get(null);

Any library on the class path can now also call the above code without
requiring any priviledges as the Instrumentation instance is exposed
without constraints. Adding a proper method for reading an instance of
Instrumentation would prevent this.

2016-09-30 15:14 GMT+02:00 Andrew Dinn :

> On 30/09/16 13:15, Rafael Winterhalter wrote:
> > Such a library exists already:
> > http://mvnrepository.com/search?q=byte-buddy-agent
>
> Ok, so problem solved :-)
>
> > I do however not share your point of view on this: if there is a valid
> > use case for self-attachment - Aleksey, you and I already named several
> > such use cases - why not add convenience, security and performance to
> > the API? Right now, a library can already self-attach when there is no
> > security manager but will always expose the Instrumentation instance
> > accessibly on the system class loader.
>
> I am not really sure what you mean by "will always expose the
> Instrumentation instance accessibly on the system class loader". Are you
> talking about reflective access from classes loaded by the system class
> loader? or do you mean something else?
>
> regards,
>
>
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Andrew Dinn]

On 30/09/16 13:15, Rafael Winterhalter wrote:
> Such a library exists already:
> http://mvnrepository.com/search?q=byte-buddy-agent

Ok, so problem solved :-)

> I do however not share your point of view on this: if there is a valid
> use case for self-attachment - Aleksey, you and I already named several
> such use cases - why not add convenience, security and performance to
> the API? Right now, a library can already self-attach when there is no
> security manager but will always expose the Instrumentation instance
> accessibly on the system class loader.

I am not really sure what you mean by "will always expose the
Instrumentation instance accessibly on the system class loader". Are you
talking about reflective access from classes loaded by the system class
loader? or do you mean something else?

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Rafael Winterhalter]

Such a library exists already:
http://mvnrepository.com/search?q=byte-buddy-agent

I do however not share your point of view on this: if there is a valid use
case for self-attachment - Aleksey, you and I already named several such
use cases - why not add convenience, security and performance to the API?
Right now, a library can already self-attach when there is no security
manager but will always expose the Instrumentation instance accessibly on
the system class loader.  It is my opinion that adding such an API would
only improve the situation compared to today. In this light, calling
ByteBuddyAgent.install() is a less secure way of calling the proposed
Instrumentation.getInstance().

I understand that this needs to be assesed thoroughly but I still hope this
proposal is not dismissed prematurely.

Best regards, Rafael

Best regards, Rafael

2016-09-30 13:51 GMT+02:00 Andrew Dinn :

> On 30/09/16 10:37, Alan Bateman wrote:
> > On 30/09/2016 09:39, Rafael Winterhalter wrote:
> >
> >> I agree that this should be considered carefully.
> >>
> >> However, without a security manager, it is already possible to get
> >> such an instance for most environments. And starting with Java 9, this
> >> will extend to non JDK-VMs as the former tools.jar is now a module.
> > Assuming you mean the jdk.attach module then it's JDK-specific. It's not
> > linked into the runtime image that is the JRE for example.
> >
> > In any case, this proposal is a significant concern as it is exposing
> > capabilities in the standard API that were intended for tool agents. The
> > implications are huge.
>
> I agree with all Alan's concerns here.
>
> Also,  although many developers have used the attach API to self-hoist
> an agent into a JVM (Byteman also does this when used with JUnit/TestNG)
> and have, perhaps, re-implemented the same wheel to do so I don't think
> fixing that circumstance really merits a JVM change. A couple of small
> library jars could address this problem, shrink-wrapping the necessary
> functionality into a common API.
>
> The first jar could provide a class with a getInstrumentation() method
> which drives the attach API, loading the 2nd agent jar (which it can
> locate from the classpath).
>
> The agent jar can securely hand over the Instrumentation instance to the
> API class provided in the first jar allowing it to be returned to the
> caller.
>
> If these 2 jars were posted for common use (e.g. to Maven Central) then
> the functionality can be made available simply by adding the required
> jars to the classpath and calling the getInstrumentation() method.
>
> regards,
>
>
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Andrew Dinn]

On 30/09/16 10:37, Alan Bateman wrote:
> On 30/09/2016 09:39, Rafael Winterhalter wrote:
> 
>> I agree that this should be considered carefully.
>>
>> However, without a security manager, it is already possible to get
>> such an instance for most environments. And starting with Java 9, this
>> will extend to non JDK-VMs as the former tools.jar is now a module.
> Assuming you mean the jdk.attach module then it's JDK-specific. It's not
> linked into the runtime image that is the JRE for example.
> 
> In any case, this proposal is a significant concern as it is exposing
> capabilities in the standard API that were intended for tool agents. The
> implications are huge.

I agree with all Alan's concerns here.

Also,  although many developers have used the attach API to self-hoist
an agent into a JVM (Byteman also does this when used with JUnit/TestNG)
and have, perhaps, re-implemented the same wheel to do so I don't think
fixing that circumstance really merits a JVM change. A couple of small
library jars could address this problem, shrink-wrapping the necessary
functionality into a common API.

The first jar could provide a class with a getInstrumentation() method
which drives the attach API, loading the 2nd agent jar (which it can
locate from the classpath).

The agent jar can securely hand over the Instrumentation instance to the
API class provided in the first jar allowing it to be returned to the
caller.

If these 2 jars were posted for common use (e.g. to Maven Central) then
the functionality can be made available simply by adding the required
jars to the classpath and calling the getInstrumentation() method.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Alan Bateman]

On 30/09/2016 09:39, Rafael Winterhalter wrote:


I agree that this should be considered carefully.

However, without a security manager, it is already possible to get 
such an instance for most environments. And starting with Java 9, this 
will extend to non JDK-VMs as the former tools.jar is now a module.
Assuming you mean the jdk.attach module then it's JDK-specific. It's not 
linked into the runtime image that is the JRE for example.


In any case, this proposal is a significant concern as it is exposing 
capabilities in the standard API that were intended for tool agents. The 
implications are huge.


-Alan

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Rafael Winterhalter]

I agree that this should be considered carefully.

However, without a security manager, it is already possible to get such an
instance for most environments. And starting with Java 9, this will extend
to non JDK-VMs as the former tools.jar is now a module. I think by adding
such a method, security would improve as current solutions often use their
privileged access to read such an instance but often leave this instance
exposed in a public field reachable via the system class loader. I think by
including such a method, one could avoid the spreading of custom libraries
(like mine) that do self-attachment and properly secure the access via a
security manager.

Thank you for considering this!
Best regards, Rafael

2016-09-30 10:31 GMT+02:00 Alan Bateman :

> On 29/09/2016 20:37, Rafael Winterhalter wrote:
>
> It would be perfectly fine, in my opinion, to throw an unsupported
>> operation exception, if the feature was unsupported. The method would
>> primarily be used by testing code and tools.
>>
>> Right, but it essentially means that anything that use
> Instrumentation.getInstance(...) can dynamically extend the class path,
> add to the boot class loader search, redefine any class or module, ...
>  Assume the common case where there is no security manager and not using
> JDK-specific APIs. So I think this requires a lot of consideration before
> going any further - the original intention with this API is that it was for
> tool agents and this is why a method like the proposal method has been kept
> out of the API.
>
> -Alan
>

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Alan Bateman]

On 29/09/2016 20:37, Rafael Winterhalter wrote:

It would be perfectly fine, in my opinion, to throw an unsupported 
operation exception, if the feature was unsupported. The method would 
primarily be used by testing code and tools.


Right, but it essentially means that anything that use 
Instrumentation.getInstance(...) can dynamically extend the class path, 
add to the boot class loader search, redefine any class or module, ...   
Assume the common case where there is no security manager and not using 
JDK-specific APIs. So I think this requires a lot of consideration 
before going any further - the original intention with this API is that 
it was for tool agents and this is why a method like the proposal method 
has been kept out of the API.


-Alan

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Rafael Winterhalter]

It would be perfectly fine, in my opinion, to throw an unsupported
operation exception, if the feature was unsupported. The method would
primarily be used by testing code and tools. In Mockito, we simply do not
offer inline mocks (but subclass mocks) if the runtime attachment fails.
EhCache or JOL also fall back to not offering the related feature. Adding
the method would simply add convenience and security to something that is
already done by many libraries and tools out there.

Also, all major VMs do support it as of today as far as I know. (The
exception being J9 which currently has an unfortunate bug that triggers an
internal assertion error upon retransformation.)

Am 29.09.2016 9:22 nachm. schrieb "Remi Forax" :

>
>
> On September 29, 2016 9:06:12 PM GMT+02:00, Alan Bateman <
> alan.bate...@oracle.com> wrote:
> >On 29/09/2016 18:50, Rafael Winterhalter wrote:
> >
> >> :
> >>
> >> Therefore I want to propose adding a static method to the
> >Instrumentation
> >> interface:
> >>
> >> interface Instrumentation {
> >>static Instrumentation getInstance(boolean redefine, boolean
> >retransform,
> >> boolean nativePrefix) {
> >>  // security manager check
> >>  return getInstance0(redefine, retransform, nativePrefix);
> >>}
> >> }
> >>
>
> I've a code that also does that for implementing a Repl like JShell.
>
> >The original intention, back in JSR 163, was that java.lang.instrument
> >be for instrumentation agents, not applications. This is why the API
> >deliberately does not include a method to get the Instrumentation
> >object
> >along the lines you have propose. Sure, you can leak it from an agent
> >started on the command line or attached at runtime but that is not the
> >original intention. So I think introducing this method is a very
> >significant change that would require a lot of consideration (previous
> >requests to provide a way for applications to get the Instrumentation
> >object without an agent in the picture were resisted).
> >
> >Separately, introducing this method creates a complication for runtimes
> >
> >that do not have JVM TI support (the JPLIS agent is based on JVM TI).
> >As
> >things stand then it's possible to create a runtime that does not have
> >a
> >means to start agents from the command-line (the spec allows this) and
> >so there is no need to have the implementation support for this API. So
> >
> >if a method like this is every introduced then it would either have to
> >be optional or it would require changes to the JVM TI spec to make it
> >non-optional (or is based on an alternative JPLIS implementation that
> >doesn't use JVM TI).
>
> Having it optional seams fine.
> Perhaps it has to be activated by a command line argument too.
>
> >
> >-Alan
>
> Rémi
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
>

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Remi Forax]

On September 29, 2016 9:06:12 PM GMT+02:00, Alan Bateman 
 wrote:
>On 29/09/2016 18:50, Rafael Winterhalter wrote:
>
>> :
>>
>> Therefore I want to propose adding a static method to the
>Instrumentation
>> interface:
>>
>> interface Instrumentation {
>>static Instrumentation getInstance(boolean redefine, boolean
>retransform,
>> boolean nativePrefix) {
>>  // security manager check
>>  return getInstance0(redefine, retransform, nativePrefix);
>>}
>> }
>>

I've a code that also does that for implementing a Repl like JShell.

>The original intention, back in JSR 163, was that java.lang.instrument 
>be for instrumentation agents, not applications. This is why the API 
>deliberately does not include a method to get the Instrumentation
>object 
>along the lines you have propose. Sure, you can leak it from an agent 
>started on the command line or attached at runtime but that is not the 
>original intention. So I think introducing this method is a very 
>significant change that would require a lot of consideration (previous 
>requests to provide a way for applications to get the Instrumentation 
>object without an agent in the picture were resisted).
>
>Separately, introducing this method creates a complication for runtimes
>
>that do not have JVM TI support (the JPLIS agent is based on JVM TI).
>As 
>things stand then it's possible to create a runtime that does not have
>a 
>means to start agents from the command-line (the spec allows this) and 
>so there is no need to have the implementation support for this API. So
>
>if a method like this is every introduced then it would either have to 
>be optional or it would require changes to the JVM TI spec to make it 
>non-optional (or is based on an alternative JPLIS implementation that 
>doesn't use JVM TI).

Having it optional seams fine.
Perhaps it has to be activated by a command line argument too.

>
>-Alan

Rémi 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Alan Bateman]

On 29/09/2016 18:50, Rafael Winterhalter wrote:


:

Therefore I want to propose adding a static method to the Instrumentation
interface:

interface Instrumentation {
   static Instrumentation getInstance(boolean redefine, boolean retransform,
boolean nativePrefix) {
 // security manager check
 return getInstance0(redefine, retransform, nativePrefix);
   }
}

The original intention, back in JSR 163, was that java.lang.instrument 
be for instrumentation agents, not applications. This is why the API 
deliberately does not include a method to get the Instrumentation object 
along the lines you have propose. Sure, you can leak it from an agent 
started on the command line or attached at runtime but that is not the 
original intention. So I think introducing this method is a very 
significant change that would require a lot of consideration (previous 
requests to provide a way for applications to get the Instrumentation 
object without an agent in the picture were resisted).


Separately, introducing this method creates a complication for runtimes 
that do not have JVM TI support (the JPLIS agent is based on JVM TI). As 
things stand then it's possible to create a runtime that does not have a 
means to start agents from the command-line (the spec allows this) and 
so there is no need to have the implementation support for this API. So 
if a method like this is every introduced then it would either have to 
be optional or it would require changes to the JVM TI spec to make it 
non-optional (or is based on an alternative JPLIS implementation that 
doesn't use JVM TI).


-Alan

Re: Proposal to introduce method "Instrumentation.getInstance()" to instrument the current VM

[Aleksey Shipilev]

On 09/29/2016 07:50 PM, Rafael Winterhalter wrote:
> I want to propose adding a method to the instrumentation API to receive an
> instance of the current VM's instrumentation API. Currently, many
> applications "self-attach" to gain such access. Unfortunately, this only
> works on JDK-VMs but I believe that this approach will grow in popularity
> with Java 9 where the Attach API is also part of any regular VM.

> interface Instrumentation {
>   static Instrumentation getInstance(boolean redefine, boolean retransform,
> boolean nativePrefix) {
> // security manager check
> return getInstance0(redefine, retransform, nativePrefix);
>   }
> }

I would like to second this proposal.

Our very own JOL [1] uses self-attach [2] to get Instrumentation
instance for carefully polling Object instance sizes. Self-attach
enables JOL usage as the library dependency without requiring command
line tweaks.

Thanks,
-Aleksey

[1] http://openjdk.java.net/projects/code-tools/jol/
[2]
http://hg.openjdk.java.net/code-tools/jol/file/b5653b56d154/jol-core/src/main/java/org/openjdk/jol/vm/InstrumentationSupport.java