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 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 ***@***.***> on > core-libs-dev ***@***.***>:* > > On 19/04/2021 22:20, Rafael Winterhalter wrote: > > : > 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. > > I should have been clearer, it's the combination of the two that creates > the attractive nuisance. I don't think there are any objections to a > defineClass for agents specified on the command line with -javaagent. > However we have to be cautious about extending that capability to agents > that are loaded into a running VM with the attach mechanism. > > ByteBuddy looks great for code generation and transforming classes but > ByteBuddyAgent makes me nervous. It looks like I can deploy > byte-buddy-agent-<version>.jar on my class path and invoke the public > static ByteBuddyAgent.install() method to get the Instrumentation object > for the current VM. That may be convenient for some but this is the > all-powerful Instrumentation object that shouldn't be leaked to library > or application code. Now combine this with the proposed defineClass and > it means that any code on the class path could inject a class into > java.lang or any run-time package without any agent voodoo or opt-in via > the command line. That would be difficult genie to re-bottle if it were > to get traction. > > You mentioned restricting JNI in the future. I'm not aware of a definite > plan or time-frame. Project Panama is pioneering restricting access to > native operations as a bug or mis-use with the linker API can easily > crash the VM or breakage in other ways. Extending this to JNI would be a > logical next step but I could imagine it taking a long time and many > releases to get there. > > As regards this PR then I would be happy to work with you on a revised > proposed that would limit it to agents specified with -javaagent. That > would not preclude extending the capability, maybe in a more restricted > form, to agents loaded into a running VM in the future. > > -Alan. > > — > 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-823281169>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABCIA4FE2B4DGBZS4QO6SM3TJV7T5ANCNFSM43BSDEGQ> > . > ------------- PR: https://git.openjdk.java.net/jdk/pull/3546
