Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

raphw
Copy link
Member

@raphw raphw commented Apr 16, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

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

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3546/head:pull/3546
$ git checkout pull/3546

Update a local copy of the PR:
$ git checkout pull/3546
$ git pull https://git.openjdk.java.net/jdk pull/3546/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3546

View PR using the GUI difftool:
$ git pr show -t 3546

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3546.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 16, 2021

👋 Welcome back winterhalter! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 16, 2021
@openjdk
Copy link

openjdk bot commented Apr 16, 2021

@raphw The following labels will be automatically applied to this pull request:

  • core-libs
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Apr 16, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Webrevs

@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from Remi Forax on core-libs-dev:

----- Mail original -----

De: "Rafael Winterhalter" <winterhalter at openjdk.java.net>
?: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "serviceability-dev" <serviceability-dev at openjdk.java.net>
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

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from Remi Forax on core-libs-dev:

----- Mail original -----

De: "Rafael Winterhalter" <winterhalter at openjdk.java.net>
?: "core-libs-dev" <core-libs-dev at openjdk.java.net>, "serviceability-dev" <serviceability-dev at openjdk.java.net>
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

@AlanBateman
Copy link
Contributor

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.

Separately, the proposal in JEP 410 is to terminally deprecate ProtectionDomain.

@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from Rafael Winterhalter on core-libs-dev:

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 <forax at univ-mlv.fr>:

----- Mail original -----

De: "Rafael Winterhalter" <winterhalter at openjdk.java.net>
?: "core-libs-dev" <core-libs-dev at openjdk.java.net>,
"serviceability-dev" <serviceability-dev at openjdk.java.net>
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
pull/3546/head:pull/3546

@raphw
Copy link
Member Author

raphw commented Apr 16, 2021

The ticket was created as a reaction to a write-up I made in January 2018. 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.

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.

@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "Rafael Winterhalter" <rafael.wth at gmail.com>
?: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Rafael Winterhalter" <winterhalter at openjdk.java.net>, "core-libs-dev"
<core-libs-dev at openjdk.java.net>, "serviceability-dev"
<serviceability-dev at 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 < [
mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ] >:

----- Mail original -----

De: "Rafael Winterhalter" < [ mailto:winterhalter at openjdk.java.net |
winterhalter at openjdk.java.net ] >
?: "core-libs-dev" < [ mailto:core-libs-dev at openjdk.java.net |
core-libs-dev at openjdk.java.net ] >, "serviceability-dev" < [
mailto:serviceability-dev at openjdk.java.net |
serviceability-dev at openjdk.java.net ] >
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

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

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "Rafael Winterhalter" <rafael.wth at gmail.com>
?: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Rafael Winterhalter" <winterhalter at openjdk.java.net>, "core-libs-dev"
<core-libs-dev at openjdk.java.net>, "serviceability-dev"
<serviceability-dev at 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 < [
mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ] >:

----- Mail original -----

De: "Rafael Winterhalter" < [ mailto:winterhalter at openjdk.java.net |
winterhalter at openjdk.java.net ] >
?: "core-libs-dev" < [ mailto:core-libs-dev at openjdk.java.net |
core-libs-dev at openjdk.java.net ] >, "serviceability-dev" < [
mailto:serviceability-dev at openjdk.java.net |
serviceability-dev at openjdk.java.net ] >
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

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

@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from Rafael Winterhalter on core-libs-dev:

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 <forax at univ-mlv.fr>:

------------------------------

*De: *"Rafael Winterhalter" <rafael.wth at gmail.com>
*?: *"Remi Forax" <forax at univ-mlv.fr>
*Cc: *"Rafael Winterhalter" <winterhalter at openjdk.java.net>,
"core-libs-dev" <core-libs-dev at openjdk.java.net>, "serviceability-dev" <
serviceability-dev at 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 <forax at univ-mlv.fr>:

----- Mail original -----

De: "Rafael Winterhalter" <winterhalter at openjdk.java.net>
?: "core-libs-dev" <core-libs-dev at openjdk.java.net>,
"serviceability-dev" <serviceability-dev at openjdk.java.net>
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
pull/3546/head:pull/3546

@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

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
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

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

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
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

@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "Rafael Winterhalter" <rafael.wth at gmail.com>
?: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Rafael Winterhalter" <winterhalter at openjdk.java.net>, "core-libs-dev"
<core-libs-dev at openjdk.java.net>, "serviceability-dev"
<serviceability-dev at openjdk.java.net>
Envoy?: Vendredi 16 Avril 2021 18:41:26
Objet: Re: RFR: 8200559: Java agents doing instrumentation need a means to
define auxilary classes

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.

I was thinking about adding a dummy class in the package you want to load classes.

Anyway, you can also call ClassLoader.defineClass directly. Put the code that calls defineClass in a module and use Instrumentation.redefineModule() to open java.base to this module.

R?mi

Am Fr., 16. Apr. 2021 um 18:35 Uhr schrieb < [ mailto:forax at univ-mlv.fr |
forax at univ-mlv.fr ] >:

De: "Rafael Winterhalter" < [ mailto:rafael.wth at gmail.com | rafael.wth at gmail.com
] >
?: "Remi Forax" < [ mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ] >
Cc: "Rafael Winterhalter" < [ mailto:winterhalter at openjdk.java.net |
winterhalter at openjdk.java.net ] >, "core-libs-dev" < [
mailto:core-libs-dev at openjdk.java.net | core-libs-dev at openjdk.java.net ] >,
"serviceability-dev" < [ mailto:serviceability-dev at openjdk.java.net |
serviceability-dev at 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 < [
mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ] >:

----- Mail original -----

De: "Rafael Winterhalter" < [ mailto:winterhalter at openjdk.java.net |
winterhalter at openjdk.java.net ] >
?: "core-libs-dev" < [ mailto:core-libs-dev at openjdk.java.net |
core-libs-dev at openjdk.java.net ] >, "serviceability-dev" < [
mailto:serviceability-dev at openjdk.java.net |
serviceability-dev at openjdk.java.net ] >
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

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

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "Rafael Winterhalter" <rafael.wth at gmail.com>
?: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Rafael Winterhalter" <winterhalter at openjdk.java.net>, "core-libs-dev"
<core-libs-dev at openjdk.java.net>, "serviceability-dev"
<serviceability-dev at openjdk.java.net>
Envoy?: Vendredi 16 Avril 2021 18:41:26
Objet: Re: RFR: 8200559: Java agents doing instrumentation need a means to
define auxilary classes

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.

I was thinking about adding a dummy class in the package you want to load classes.

Anyway, you can also call ClassLoader.defineClass directly. Put the code that calls defineClass in a module and use Instrumentation.redefineModule() to open java.base to this module.

R?mi

Am Fr., 16. Apr. 2021 um 18:35 Uhr schrieb < [ mailto:forax at univ-mlv.fr |
forax at univ-mlv.fr ] >:

De: "Rafael Winterhalter" < [ mailto:rafael.wth at gmail.com | rafael.wth at gmail.com
] >
?: "Remi Forax" < [ mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ] >
Cc: "Rafael Winterhalter" < [ mailto:winterhalter at openjdk.java.net |
winterhalter at openjdk.java.net ] >, "core-libs-dev" < [
mailto:core-libs-dev at openjdk.java.net | core-libs-dev at openjdk.java.net ] >,
"serviceability-dev" < [ mailto:serviceability-dev at openjdk.java.net |
serviceability-dev at 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 < [
mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ] >:

----- Mail original -----

De: "Rafael Winterhalter" < [ mailto:winterhalter at openjdk.java.net |
winterhalter at openjdk.java.net ] >
?: "core-libs-dev" < [ mailto:core-libs-dev at openjdk.java.net |
core-libs-dev at openjdk.java.net ] >, "serviceability-dev" < [
mailto:serviceability-dev at openjdk.java.net |
serviceability-dev at openjdk.java.net ] >
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

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

@mlbridge
Copy link

mlbridge bot commented Apr 16, 2021

Mailing list message from Rafael Winterhalter on core-libs-dev:

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.Bateman at 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

@raphw raphw changed the title 8200559: Java agents doing instrumentation need a means to define auxilary classes 8200559: Java agents doing instrumentation need a means to define auxiliary classes Apr 17, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 18, 2021

Mailing list message from Alan Bateman on core-libs-dev:

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 find the mails in
the archive, it would have been on serviceability-dev in 2016 or 2017.
The compromise at the time was to introduce the
-XX:EnableDynamicAgentLoading option with an initial value of "true" and
re-visit it later. Flipping the default would mean that JVMTI and Java
agents could not be loaded into a target VM without opt-in on the
command line. No impact to agents specified on the command line with
-javaagent, no impact to other usages of the attach mechanism so jcmd
and the other diagnostic tools would continue to work.

To re-cap, the main concern is the Instrumentation object is
all-powerful and is intended for tools and the instrumentation is
intended to be benign to support use-cases such as monitoring and
tracing. It was never intended to a back-door into JDK internals.
Specifying an agent on the command line with -javaagent is the opt-in to
trust that agent. A tool run by the same user that loads its agent into
a target VM is the use-case we targeted when we added the attach
mechanism and the late binding agent support in JDK 6. The issue with
that (and it's a regret now) is that the mechanism doesn't distinguish
usage by a tool from a library/application using the attach mechanism to
load an agent into the current VM (directly or using an intermediate VM).

Time has moved on and maybe a better approach is to not change the XX
option but instead load the agents with reduced capabilities. It would
require opt-in on the command line to give these agents the same
capabilities as agents specified on the command line with -javaagent. It
would require exploration but it might be that unrecognized agents would
not be allowed to redefine java.base or other core modules or instrument
classes in those modules. The APIs are already specified to allow
Unmodified{Module,Class}Exception be thrown as there have been cases in
the past where some classes could not be transformed. Project Panama is
currently exploring how to restrict and grant access to native code and
maybe it should be the same mechanism or at least be consistent.

Hopefully this helps sets some context as to why we have to be cautious
with this PR. If the proposal were a defineClass that was limited to
agents specified on the command line then it might okay. This would
serve use-cases with tool agents that are way more advanced that the
use-cases that we envisaged back in Java 5/6. The previous exploration
into allowing non-public auxiliary classes be defined in the same
run-time package as the class being instrumented was also okay.

-Alan

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Apr 18, 2021

Mailing list message from Alan Bateman on core-libs-dev:

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 find the mails in
the archive, it would have been on serviceability-dev in 2016 or 2017.
The compromise at the time was to introduce the
-XX:EnableDynamicAgentLoading option with an initial value of "true" and
re-visit it later. Flipping the default would mean that JVMTI and Java
agents could not be loaded into a target VM without opt-in on the
command line. No impact to agents specified on the command line with
-javaagent, no impact to other usages of the attach mechanism so jcmd
and the other diagnostic tools would continue to work.

To re-cap, the main concern is the Instrumentation object is
all-powerful and is intended for tools and the instrumentation is
intended to be benign to support use-cases such as monitoring and
tracing. It was never intended to a back-door into JDK internals.
Specifying an agent on the command line with -javaagent is the opt-in to
trust that agent. A tool run by the same user that loads its agent into
a target VM is the use-case we targeted when we added the attach
mechanism and the late binding agent support in JDK 6. The issue with
that (and it's a regret now) is that the mechanism doesn't distinguish
usage by a tool from a library/application using the attach mechanism to
load an agent into the current VM (directly or using an intermediate VM).

Time has moved on and maybe a better approach is to not change the XX
option but instead load the agents with reduced capabilities. It would
require opt-in on the command line to give these agents the same
capabilities as agents specified on the command line with -javaagent. It
would require exploration but it might be that unrecognized agents would
not be allowed to redefine java.base or other core modules or instrument
classes in those modules. The APIs are already specified to allow
Unmodified{Module,Class}Exception be thrown as there have been cases in
the past where some classes could not be transformed. Project Panama is
currently exploring how to restrict and grant access to native code and
maybe it should be the same mechanism or at least be consistent.

Hopefully this helps sets some context as to why we have to be cautious
with this PR. If the proposal were a defineClass that was limited to
agents specified on the command line then it might okay. This would
serve use-cases with tool agents that are way more advanced that the
use-cases that we envisaged back in Java 5/6. The previous exploration
into allowing non-public auxiliary classes be defined in the same
run-time package as the class being instrumented was also okay.

-Alan

@mlbridge
Copy link

mlbridge bot commented Apr 18, 2021

Mailing list message from Rafael Winterhalter on core-libs-dev:

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.Bateman at 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 find the mails in
the archive, it would have been on serviceability-dev in 2016 or 2017.
The compromise at the time was to introduce the
-XX:EnableDynamicAgentLoading option with an initial value of "true" and
re-visit it later. Flipping the default would mean that JVMTI and Java
agents could not be loaded into a target VM without opt-in on the
command line. No impact to agents specified on the command line with
-javaagent, no impact to other usages of the attach mechanism so jcmd
and the other diagnostic tools would continue to work.

To re-cap, the main concern is the Instrumentation object is
all-powerful and is intended for tools and the instrumentation is
intended to be benign to support use-cases such as monitoring and
tracing. It was never intended to a back-door into JDK internals.
Specifying an agent on the command line with -javaagent is the opt-in to
trust that agent. A tool run by the same user that loads its agent into
a target VM is the use-case we targeted when we added the attach
mechanism and the late binding agent support in JDK 6. The issue with
that (and it's a regret now) is that the mechanism doesn't distinguish
usage by a tool from a library/application using the attach mechanism to
load an agent into the current VM (directly or using an intermediate VM).

Time has moved on and maybe a better approach is to not change the XX
option but instead load the agents with reduced capabilities. It would
require opt-in on the command line to give these agents the same
capabilities as agents specified on the command line with -javaagent. It
would require exploration but it might be that unrecognized agents would
not be allowed to redefine java.base or other core modules or instrument
classes in those modules. The APIs are already specified to allow
Unmodified{Module,Class}Exception be thrown as there have been cases in
the past where some classes could not be transformed. Project Panama is
currently exploring how to restrict and grant access to native code and
maybe it should be the same mechanism or at least be consistent.

Hopefully this helps sets some context as to why we have to be cautious
with this PR. If the proposal were a defineClass that was limited to
agents specified on the command line then it might okay. This would
serve use-cases with tool agents that are way more advanced that the
use-cases that we envisaged back in Java 5/6. The previous exploration
into allowing non-public auxiliary classes be defined in the same
run-time package as the class being instrumented was also okay.

-Alan

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Apr 18, 2021

Mailing list message from Rafael Winterhalter on core-libs-dev:

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.Bateman at 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 find the mails in
the archive, it would have been on serviceability-dev in 2016 or 2017.
The compromise at the time was to introduce the
-XX:EnableDynamicAgentLoading option with an initial value of "true" and
re-visit it later. Flipping the default would mean that JVMTI and Java
agents could not be loaded into a target VM without opt-in on the
command line. No impact to agents specified on the command line with
-javaagent, no impact to other usages of the attach mechanism so jcmd
and the other diagnostic tools would continue to work.

To re-cap, the main concern is the Instrumentation object is
all-powerful and is intended for tools and the instrumentation is
intended to be benign to support use-cases such as monitoring and
tracing. It was never intended to a back-door into JDK internals.
Specifying an agent on the command line with -javaagent is the opt-in to
trust that agent. A tool run by the same user that loads its agent into
a target VM is the use-case we targeted when we added the attach
mechanism and the late binding agent support in JDK 6. The issue with
that (and it's a regret now) is that the mechanism doesn't distinguish
usage by a tool from a library/application using the attach mechanism to
load an agent into the current VM (directly or using an intermediate VM).

Time has moved on and maybe a better approach is to not change the XX
option but instead load the agents with reduced capabilities. It would
require opt-in on the command line to give these agents the same
capabilities as agents specified on the command line with -javaagent. It
would require exploration but it might be that unrecognized agents would
not be allowed to redefine java.base or other core modules or instrument
classes in those modules. The APIs are already specified to allow
Unmodified{Module,Class}Exception be thrown as there have been cases in
the past where some classes could not be transformed. Project Panama is
currently exploring how to restrict and grant access to native code and
maybe it should be the same mechanism or at least be consistent.

Hopefully this helps sets some context as to why we have to be cautious
with this PR. If the proposal were a defineClass that was limited to
agents specified on the command line then it might okay. This would
serve use-cases with tool agents that are way more advanced that the
use-cases that we envisaged back in Java 5/6. The previous exploration
into allowing non-public auxiliary classes be defined in the same
run-time package as the class being instrumented was also okay.

-Alan

@plevart
Copy link
Contributor

plevart commented Apr 19, 2021

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.

@raphw
Copy link
Member Author

raphw commented Apr 19, 2021 via email

@plevart
Copy link
Contributor

plevart commented Apr 19, 2021

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 <#3546 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCIA4EYHK5OHTDAN3FKYULTJPWS5ANCNFSM43BSDEGQ .

Ah sorry, I didn't know that. So what about the following: agent is able to define (or load) a class from bootstrap loader. So it would be able to instantiate an intermediary class, loaded by bootstrap loader which would serve as an intermediary to call into this limited Instrumentation API point...
Or better: add a Lookup parameter to the Instrumentation method. The implementation would check that the Lookup is an unrestricted Lookup from a class loaded by bootstrap loader. Agent would just have to take the pain of obtaining such Lookup instance...
But then again, rouge agent could pass Instrumentation as well as Lookup instance to application code, so we are back to square one. Caller-sensitive method is harder to fool though. But not impossible (given you have a Lookup instance)....

@AlanBateman
Copy link
Contributor

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.

The agent JAR file is added to application class path and is loaded using the system class loader. So almost always the defining loader will be the application class loader.

In general it's a hard problem to try to balance the integrity and security of the platform with the needs of agents that do arbitrary injection and instrumentation. Specifying an agent on the command line with -javaagent is the opt-in to trust that agent and a defineClass that allows arbitrary injection is plausible for that deployment. As Rafael's mentioned in one of the messages, there is enough power in the existing Instrumentation API to do that in a round about way already.

We don't have anything equivalent for agents that are loaded by tools into a target VM. I added the attach mechanism and the dynamic agent loading back in JDK 6 and regret not putting more restrictions around this. As it stands, it is open to mis-use in that 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 Instrumentation object. A less powerful Instrumentation object would not be able to redefine java.base or other core modules and would not be able to retransform classes in those modules. The option on the table during JDK 9 was to disable dynamic loading of agents by default but that was kicked down the road. I don't particularly like that option and I think we can do better.

@adinn
Copy link
Contributor

adinn commented Apr 22, 2021

Sorry to be late to this party. I've been wanting to read this thread for a while but have bene too busy up to now. I have just a few comments

I too was party to the discussions about agent capabilities and recall well the decision to gradually impose restrictions, the first one being to control dynamic agent loading. I was happy to accept that general decision and the specific one to limit the opportunity for an agent self-hoisting into the current JVM. However, a key part of the plan to move forward on restrictions was to provide an override switch. I'd very much like to see that retained as an option. I know that in some uses self-hoisting is much preferable to having to install the agent on the command line and I'd expect he same to be true for any other capabilities for which restrictions were adopted.

Although it is true -- as Ron said - -that configuring -javaagent on the command line is always /possible/ there are actually many scenarios for agent use where deployment of an agent after startup is pragmatically much more desirable. An obvious use is trouble shooting, where you only want an agent in place when something goes wrong. That turns out to be critical to solving some seriously difficult support cases. The interesting use cases also fall under testing where self-hoisting of a test agent by a test framework can result in an enormous simplification of test configuration. Usage of Byteman for testing went through the roof with this capability in place. Never underestimate the degree to which even the most minimal configuration complexity puts off Enterprise java developers when it is multiplied by the test suite size of a large project.

So, likewise with other restrictions on behaviour, I'm very happy to see them put in place for dynamically hoisted agents so long as there is still a command line override along the lines of the agent attach property that allows a dynamic agent to do all that a command line agent can.

One other thing I'd like to correct is a point made in the discussion about agent code residing in the system loader. It is true that the main agent class gets loaded by the system loader. However, it is perfectly possible to ensure that all the rest of the agent code is loaded by the bootstrap loader. A Main class can add the agent jar to the bootstrap path and then load and use reflection to invoke an effective main routine on a bootstrap loaded SubMain class.

Byteman uses this trick on request in order to allow it to instrument bootstrap classes. Because all the Byteman classes except for the original Main shell class are loaded by the bootstrap loader Byteman can safely inject references to the Byteman rule engine and Byteman exception classes into bootstrap code.

@raphw
Copy link
Member Author

raphw commented Apr 22, 2021 via email

@mlbridge
Copy link

mlbridge bot commented Apr 23, 2021

Mailing list message from Alan Snyder on core-libs-dev:

On Apr 21, 2021, at 11:40 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:

Sure, if you are using native code then you have the full power of JVM TI and JNI available. Project Panama is exploring how to restrict access to native code, I think too early to say how this might extend to JNI.

I looked at some of the Panama documents and saw no hint that it might be extended to JNI. It seems to be positioned as an (partial) alternative to JNI.

What I do see is that native code has no direct access to the JDK via Panama, only the ability to invoke provided upcalls, which can only access objects and methods that the caller has access to. That is indeed much more restricted than JNI.

Even though it is ?too early?, can you explain why you think Panama?s restrictions might apply to JNI?

As a library developer, I have often made use of JNI to work around limitations in current and older JDKs. I would hate to lose that ability.

Alan

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Apr 23, 2021

Mailing list message from Alan Snyder on core-libs-dev:

On Apr 21, 2021, at 11:40 AM, Alan Bateman <Alan.Bateman at oracle.com> wrote:

Sure, if you are using native code then you have the full power of JVM TI and JNI available. Project Panama is exploring how to restrict access to native code, I think too early to say how this might extend to JNI.

I looked at some of the Panama documents and saw no hint that it might be extended to JNI. It seems to be positioned as an (partial) alternative to JNI.

What I do see is that native code has no direct access to the JDK via Panama, only the ability to invoke provided upcalls, which can only access objects and methods that the caller has access to. That is indeed much more restricted than JNI.

Even though it is ?too early?, can you explain why you think Panama?s restrictions might apply to JNI?

As a library developer, I have often made use of JNI to work around limitations in current and older JDKs. I would hate to lose that ability.

Alan

@bridgekeeper
Copy link

bridgekeeper bot commented May 21, 2021

@raphw This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 18, 2021

@raphw This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@kriegaex
Copy link

kriegaex commented Jan 25, 2024

@raphw and everyone else, I apologise for commenting on an auto-closed issue, but I have no other way of contacting anyone, because neither do I have an account to comment on JDK-8200559 nor am I a member in any other of the JDK development or "JVM rock star" circles for good reason: I simply have a very limited understanding of the topic at hand.

I am, however, the current maintainer and project lead of Eclipse AspectJ, and since JDK 16 the weaving agent requires --add-opens java.base/java.lang=ALL-UNNAMED along with -javaagent:/path/to/aspectjweaver.jar for the reason my predecessor Andy Clement has explained in AspectJ bug 546305, which is also linked to JDK-8200559. @mlchung was also involved in the discussion there.

Here, there has been a lot of discussion about Byte Buddy and Mockito. For AspectJ, which is widely used in the industry, not just in Spring but also in countless other productive contexts, there still is no solution to the problem that we need to instrument a target class to call an auxiliary class (generated by the agent) that does not have a Lookup object on the target class to begin with. The target class is still being transformed, i.e. not loaded yet, and hence we cannot look it up.

Is this case being considered? Can any of you advise me how to solve that problem? I am open for advice and hands-on chat, desktop sharing or audio/video call sessions.

@adinn
Copy link
Contributor

adinn commented Jan 25, 2024

@kriegaex I resolved this problem in Byteman by opening up the module of the target class to a module (dynamically) created by Byteman. The Instrumentation instance provided to the agent allows you to perform such an opens operation at runtime rather than on the command line.

Byteman relies on an API provided by this module to create the lookups and hand them back for use in woven code. n.b. although this relies on the module exporting a public API, that API can be secured by requiring callers to pass a non-null Instrumentation instance i.e. to have agent capabilities.

@raphw
Copy link
Member Author

raphw commented Jan 25, 2024

Requiring such an API opens the module to anybody, though, punching a hole into the module boundary.

BB currently opens the jdk.internal.misc.Unsafe class to a module on a seperate class loader that is not reachable outside an agent, using Instrumentation. This also caters the need to inject utility classes from an agent before any class file transformer is triggered, to maintain a well-defined life-cycle.

Of course I'd prefer if there was a way to resolve a lookup from Instrumentation for a given class loader and for example a package-info class, however rendering the issue that packages might not exist (yet).

@adinn
Copy link
Contributor

adinn commented Jan 25, 2024

Requiring such an API opens the module to anybody, though, punching a hole into the module boundary.

How so? Any module created to print Lookups can easily rely on a shared secret to secure the API. Byteman employs a non-null Instrumentation object (a value which any agent ought to keep secret). However, it could just as easily have employed an arbitrary bit length hash key. The key can be used to initialize a module-private static long[] field of an API implementation class generated into the module i.e. the hole can actually be a keyhole in the shape of a key known only to the API client and implementation.

@kriegaex
Copy link

Thanks @adinn, @raphw for your feedback. I am not pretending to fully understand what you just explained or to have the slightest clue how to do what you suggested, but reading it for the second time since yesterday seems to make it clearer already. I guess, I cannot hope for a how-to with sample code. But I can immerse myself into the topic some more next time I have a vacant time box and look at Byte Buddy and Byteman source code.

@kriegaex
Copy link

BB currently opens the jdk.internal.misc.Unsafe class to a module on a seperate class loader that is not reachable outside an agent, using Instrumentation.

@raphw, may I ask how? Is there any sample code that is not connected to the BB code base with its nested classes, interfaces etc.? I know, that caters nicely to the fluent DSL BB provides, but to me it is just a maze of code that is hard to comprehend.

@AlanBateman
Copy link
Contributor

AlanBateman commented Jan 26, 2024

BB currently opens the jdk.internal.misc.Unsafe class to a module on a seperate class loader that is not reachable outside an agent, using Instrumentation.

@raphw, may I ask how? Is there any sample code that is not connected to the BB code base with its nested classes, interfaces etc.? I know, that caters nicely to the fluent DSL BB provides, but to me it is just a maze of code that is hard to comprehend.

Agents should not be using the JDK's internal Unsafe. This needs to said in the strongest possible terms.

For the discussion, it would be useful to provide a brief summary on what AspectJ is trying to do with this weaving. The JBS issue was originally about load time instrumentation defining auxiliary classes, as you might get at compile time when compiling a source file containing more than one class.. I can't tell from your comments here or in Eclipse 546305 if this is relevant to what you are trying to do or not. It may even be better to start a new discussion on serviceability-dev.

@kriegaex
Copy link

kriegaex commented Jan 26, 2024

@AlanBateman, the AspectJ weaving agent creates an auxiliary class to implement an "around" advice for a method, i.e. a method execution is intercepted and the user has options to do something before optionally calling the target method and then do something afterwards too. In doing so, she can modify method arguments before calling the target method, then also modify the result. Instead of calling the target method, she may also return a result of the expected type instead. Before, after or instead of calling the target method, she can also throw an exception.

The target class is transformed in such a way to call the auxiliary class, which necessitates the the aux-class to be in the same classloader as the target class. But because the aux-class is defined while the target class is still being transformed during class-loading, we cannot have any look-up instance pointinmg to it yet.

IMO, this is absolutely on topic, which is why the Bugzilla bug is linked to the JDK issue and was discussed with Mandy in this context.

@raphw
Copy link
Member Author

raphw commented Jan 26, 2024

I migrate the day https://bugs.openjdk.org/browse/JDK-8200559 finds a solution (and fallback for older JDKs).

I can start a new discussion, but briefly summarized from the last time this was on the mailing list:

  • Agents sometimes need to add classes, for example when intercepting a reactive API where a modified callback subclass is injected, with an extra field, for instance.
  • The suggestion was that an argument would be provided to ClassFileTransformer which would allow adding classes to the current package. A prototype was created.
  • The (my) counter argument was that communication between two packages would be difficult then, as the agent does not control class loading order. If the reactive receiver would need to cast a callback object to read the field that was added, the agent's class might not yet have been instrumented and the class would not exist, yielding an error. The receiver instrumebtation can neither inject the class.
  • Often there is also a need to inject infrastructure into class loaders/modules that the instrumentation relies on, so I argued that the facility shouldn't be added to ClassFileTransformer to begin with, but to Instrumentation. Otherwise one could also "pseudo-transform" classes to inject infrastructure, but that felt unnecessary.
  • The discussion staled thereafter.

@kriegaex
Copy link

kriegaex commented Jan 26, 2024

@AlanBateman: As for using means to achieve user benefit that the JDK team deems unfit, nobody would do that, if there was an adequate way to provide such user value. Neither AspectJ nor Byte Buddy have a reputation of being some kind of shady hacker tools.

Edit, because nobody posted in between yet: The JDK issue has been open for a long time, and the chance was missed to provide a canonical way to do what tool providers and users need for LTS 17, now again for LTS 21. I.e., even if in the future such JDK functionality will finally be provided, legacy support will carry on suboptimal, hacky solutions for many years. Please do not blame tool providers. Our customers simply want to be able to continue using tools without setting lots of extra JVM options.

@AlanBateman
Copy link
Contributor

@AlanBateman, the AspectJ weaving agent creates an auxiliary class to implement an "around" advice for a method, i.e. a method execution is intercepted and the user has options to do something before optionally calling the target method and then do something afterwards too. In doing so, she can modify method arguments before calling the target method, then also modify the result. Instead of calling the target method, she may also return a result of the expected type instead. Before, after or instead of calling the target method, she can also throw an exception.

The target class is transformed in such a way to call the auxiliary class, which necessitates the the aux-class to be in the same classloader as the target class. But because the aux-class is defined while the target class is still being transformed during class-loading, we cannot have any look-up instance pointinmg to it yet.

Right, this is what JDK-8200559 was originally about. Mandy and I discussed it several times and load-time instrumentation that defines auxiliary classes in the same run-time package is a reasonable addition.

The more general request for an "unrestricted defineClass" conflicts with several ongoing efforts so we've had to kick that to touch.

@kriegaex
Copy link

kriegaex commented Jan 26, 2024

load-time instrumentation that defines auxiliary classes in the same run-time package is a reasonable addition

Thanks for finding some common ground. I appreciate it.

The more general request for an "unrestricted defineClass" conflicts with several ongoing efforts so we've had to kick that to touch.

I better do not start to share my general opinion about JMS (edit: sorry, couldn't resist in the end) - I mean the ongoing effort that started with Jigsaw - in detail, because that would end up in a flame war.

Let me just say, that "well-meant" is not the necessarily same as "constructive", "helpful" or "necessary". Back when Jigsaw was designed, it seemed to be a good idea. But it has been introduced a long time ago, and all my enterprise customers since then are trying to ignore its existence as much as they can, mostly seeing it as an impediment or at least a major annoyance. I think, one of the reasons why Python gained so much traction for enterprise-level big data and machine learning stuff is that you have a dynamic language environment in which you can pretty much do whatever you want and just get things done.

Those same enterprise customers want to use the tools of their choosing with as many and as easy to use features as possible. They do not trade those features for security, but implement security in a different way, such as devising a micro service architecture and isolating containers from each other in OpenShift or what have you, defining service mesh rules on top of that, if necessary.

Deprecating security managers was a laudable idea, but adding those unnecessary restrictions in the JVM in exchange was kind of one step forward, two steps back. They just get in the way of getting useful things done. This is what a language and a VM are for: getting things done in a productive way with as few barriers as possible. Instead, with every new Java release, users and tool providers alike are forced to jump through more hoops. There is so much great, innovative stuff in the JVM and Java SE API. E.g., virtual threads are the greatest thing since bread came sliced. We want more of that and fewer barriers. Streams, NIO, the whole lambda and functional bunch of stuff, structured concurrency, better GC, records and neat ways to deconstruct them etc. - wow, great stuff. But new "security features" (a.k.a. restrictions) like modules and sealed classes - not so great from a productivity perspective. Since JDK 9, I have seen generations of developers trying to be more productive not because but despite those new JVM "features".

I know that my perspective is quite subjective and might be biased. But as an agile coach dealing mostly with developers in the JVM ecosystem, I have yet to find a team in which more than a tiny minority thinks that JMS is useful. That is not because they are lazy to learn or do not want to leave their confort zones. It is, because the JVM makes it progressively harder to get things done instead of treating users like adults and letting them take responsibility for security as they see fit. Enforcing things like a helicopter parent is not the smartest move.

I apologise for polluting the thread with what ended up to be too much text that is related to the topic, but not exactly focused on the problem at hand.

@adinn
Copy link
Contributor

adinn commented Jan 26, 2024

@kriegaex Luckily, you and your customers are not obliged to use the JPMS, nor find it useful for whatever libraries or apps you write or deploy. However, the fact that you or many other programmers do not use it does not mean it has not been a success. Anyone deeply involved with JDK and/or JVM development in recent years knows that it has been and continues to be critical to maintaining and extending the Java platform.

Regarding my previous comment about Byteman using its own dedicated, dynamic module to provide secure access to MethodLookup instance you might want to look at the relevant code. It relies on a sanctioned API of Instrumentation that was introduced as part of the negotiation of JPMS integration precisely to allow agents to interact with and reconfigurethe module system at runtime. The resulting Byteman code provides a simple API that allows methods to be executed indirectly, either via reflection in jdk8- or via method handles in jdk9+. You can see the details of how I achieved this in the Byteman layer and jigsaw subdirectories.

@JornVernee
Copy link
Member

The target class is transformed in such a way to call the auxiliary class, which necessitates the the aux-class to be in the same classloader as the target class. But because the aux-class is defined while the target class is still being transformed during class-loading, we cannot have any look-up instance pointinmg to it yet.

It seems like you could lazily define the aux class when the target class first needs it, instead of eagerly while the target class is being defined. e.g. generate the class bytes for the aux class up front, and embed them in the target class (For instance as a Base64 encoded string, which fits well into the constant pool). Then, have the transformed code in the target class define the the aux class when it is first needed, at which point you do have access to a lookup.

@kriegaex
Copy link

Luckily, you and your customers are not obliged to use the JPMS

They are obliged to deal with it, and so am I as a tool maintainer. Just look the the approaches mentioned here. They all are in the category which in German we would call "von hinten durch die Brust ins Auge". That literally translates as "(a shot) from behind through the chest into the eye". Think Kennedy and "magic bullet". It means that something is unnecessarily complicated. But it is not of any developer's own choosing, but because the Java API and runtime environment requires them to jump through hoops. That is exactly what I meant before. Bytecode transformation should not be rocket science, but it progressively is developing in that direction. I have seen what Byte Buddy does to get access to an Unsafe instance, thought about what @JornVernee just suggested and have yet to look into the ByteMan source code. This all sounds pretty much like black magic and a maintenance nightmare to me.

A language ought to provide means to be productive and maybe offer some (opt-in) guard railing, but not be a corset so tight that I cannot breathe anymore. I need something that supports me without strangling me.

@raphw
Copy link
Member Author

raphw commented Jan 28, 2024

I tried to give the discussion a fresh start but moved it to the mailing list: https://mail.openjdk.org/pipermail/core-libs-dev/2024-January/118622.html

I have given this some thought the recent years, so I hope that there is a solution in reach that satisfies everybody's concerns and needs.

@raphw
Copy link
Member Author

raphw commented Jan 28, 2024

pass a non-null Instrumentation instance i.e. to have agent capabilities.

What stops people from supplying a fake instance? Wouldn't you need to "test run" the instance first?

@adinn
Copy link
Contributor

adinn commented Jan 29, 2024

What stops people from supplying a fake instance? Wouldn't you need to "test run" the instance first?

Not necessarily. When the generated API implementation relies on the capabilities of class Instrumentation -- such as opening modules -- to implement the invoked operation the obvious answer is that a fake instance just won't work.

However, if you want the implementation to validate an incoming call you can easily arrange for that. For example, provide a method on the agent class that says yes to its own instance and no for any other instances e.g.

class AgentClass {
  private static Instrumentation myInst = null;
  
  void premain(Instrumentation inst) {
    myInst = inst;
    . . .
  }
  static boolean validate(Instrumentation inst) {
    return myInst != null && inst == myInst;
  }

  . . .
}

Method validate can be used to ensure API calls only proceed when invoked by the agent or code that the agent trusts.

@AlanBateman
Copy link
Contributor

What stops people from supplying a fake instance? Wouldn't you need to "test run" the instance first?

In passing, Instrumentation was a candidate to be sealed at one point as the only implementations should be in the java.instrument module. I haven't seen any delegating or other implementations but they might exist so we would need to be careful with compatibility.

@raphw
Copy link
Member Author

raphw commented Jan 29, 2024

There's plenty of them in Byte Buddy and I have seen a bunch in other agents.

@adinn
Copy link
Contributor

adinn commented Jan 29, 2024

Bytecode transformation should not be rocket science, but it progressively is developing in that direction.

Hmm? Bytecode transformation of the JDK runtime implementation is a lot more complicated than your comments seem to acknowledge and, here's the important thing, it always has been. You need to remember that instrumenting JDK runtime code involves rebuilding the engine that you are driving while you are in mid-flight. If you think there are few-to-none hidden gotchas involved in doing that then I suggest you are significantly underestimating the opportunity for things to go wrong -- not just when it comes to instrumenting some specific release of OpenJDK but also when it comes to keeping instrumentation working across legacy and future releases, with all the variety of moving parts that the (necessary) development of the platform requires.

The same observation explains why project Jigsaw was needed. The danger of clients using internal JDK runtime APIs -- especially the core runtime APIs -- is much more subtle than many of the programmers who have routinely relied on it recognize. The biggest threat is that public runtime APIs are often implemented via calls to multiple internal APIs -- which may themselves involve multiple entries and re-entries to the JVM. It has always been (and always will be) the case that an isolated call from a client to an internal API can leave the JDK runtime and/or the JVM in an incoherent state because correct use of that internal API requires a correct sequence of invocations with matched inputs and outputs. It is easy even for OpenJDK developers to fails to get this right, especially when calls involve entry to the JVM. The possibility for a programmer who is not very familiar with the JDK runtime code and the JVM code to get it wrong are significant. Worse, the problems may not manifest immediately or in all cases so the danger can be unapparent until disaster strikes.

@kriegaex
Copy link

kriegaex commented Jan 30, 2024

Bytecode transformation should not be rocket science, but it progressively is developing in that direction.

Hmm? Bytecode transformation of the JDK runtime implementation is a lot more complicated than your comments seem to acknowledge

That is, because I was not talking about JDK runtime transformation but about what the AspectJ weaving agent does: transformation of application classes during class-loading.

I am aware of the fact, that it is also possible to retransform already loaded classes, as a special case also bootstrap ones from the JRE. Of course, this is more complicated than the simple case. But my point was, that even the simple case is not simple, if I need to define classes in an arbitrary class loader - not because technically it is difficult, but simply because the JRE API to do so is more and more sealed off with each new Java release. This is also what I mean, when I said, that developers are not treated as adults but "protected" by well-meaning, but ill-doing helicopter parents.

here's the important thing, it always has been.

No, byte code transformation is not complicated per se. Getting the transformed classes where they need to be is complicated, but artificially so.

You need to remember that instrumenting JDK runtime code involves rebuilding the engine that you are driving while you are in mid-flight.

No, I do not need to remember, because I am aware of that fact. It is just off-topic here with regard to what I asked about. But that other use case, which I have experimented with in another context (test mocking and stubbing) in the past, is an intriguing one, too. I am not underestimating anything there, but for AspectJ it is simply out of scope. Should I ever decide to add the capability to weave aspects into JRE classes, of course that will up the complexity by a notch or two.

BTW, if developers want to experiment with instrumenting JRE classes during runtime and in doing so crash the engine against the wall, they are still adults. They might learn something along the way, e.g. how not to do certain things. But they also have a chance to discover how to do it right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org rfr Pull request is ready for review serviceability serviceability-dev@openjdk.org
6 participants