Bazel@HEAD on JDK 17: GoogleTestSecurityManager WARNING: System::setSecurityManager will be removed in a future release

[davido]

I'm running Gerrit tests on JDK 17 and seeing this warning:

  $ bazeldev test --config java17 //javatests/com/google/gerrit/acceptance/...
[...]
WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by com.google.testing.junit.runner.util.GoogleTestSecurityManager (file:/home/davido/.cache/bazel/_bazel_davido/27a001f4182820ef315d8d2d4f1edafe/external/remote_java_tools/java_tools/Runner_deploy.jar)
WARNING: Please consider reporting this to the maintainers of com.google.testing.junit.runner.util.GoogleTestSecurityManager
WARNING: System::setSecurityManager will be removed in a future release

To reproduce, clone recursively Gerrit apply this CL: [1], and run with Bazel@HEAD (at least 5.0.0rc3):

  $ bazeldev test --config java17 //javatests/com/google/gerrit/acceptance/rest/revision:RevisionIT

Related: #11146.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/291864

[sventiffe]

@comius

[comius]

cc @cushon

[cushon]

System::setSecurityManager will be removed in a future release

This warning is being emitted by the JDK, and is the expected result of https://openjdk.java.net/jeps/411

I don't think anything here is specific to Bazel?

[fmeum]

I don't think anything here is specific to Bazel?

It's Bazel's TestRunner that calls System::setSecurityManager. Unless there is a way to disable the warning or replace the usage, Bazel tests with JDK 17 will probably always show this warning.

[cushon]

Of course, thanks.

We're looking at alternatives to the test runner. We may end up remove the use of SecurityManager entirely.

As far as I know there isn't a timeline for the removal yet, but we'll keep an eye on this and try to have something in place before it actually goes away.

In the interim the warnings are not very actionable, but I'm not sure they're suppressible.

IIUC there is some ongoing discussion upstream about provide alternatives to some of the supported functionality, e.g. discouraging calls to System.exit, but I'm not sure there are concrete plans there yet.

[cushon]

Reading https://openjdk.java.net/jeps/411,

• the warnings can be disabled (at least for now) with -Djava.security.manager=allow
• the bug for an alternate way to make it harder to call System.exit in tests is https://bugs.openjdk.java.net/browse/JDK-8199704

[fmeum]

• the warnings can be disabled (at least for now) with -Djava.security.manager=allow

Are you sure that's true? The JEP seems to imply that this flag only makes the startup warning go away, not the runtime warning when setSecurityManager is called. The relevant part of the code also doesn't seem to check the property.

[cushon]

The relevant part of the code also doesn't seem to check the property.

Entirely possible I'm missing something here, but I thought it did check the property? See:

https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/java.base/share/classes/java/lang/System.java#L192-L194

https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/java.base/share/classes/java/lang/System.java#L2191-L2199

[fmeum]

It does check it in these places and will not print the startup warning if it's set to allow, but I think that it will still print the four-line usage warning repeated in the issue description.

[davido]

Reading https://openjdk.java.net/jeps/411,

• the warnings can be disabled (at least for now) with -Djava.security.manager=allow

Thanks, @cushon. This works on Java 17: the warning is disappeared with this option.

However, it cannot be set unconditionally, because, with this option Java 11 doesn't work, as the setting is not backwards compatible:

java.lang.Error: Could not create SecurityManager
	at java.lang.System.initPhase3(java.base@11.0.12/System.java:2065)
Caused by: java.lang.ClassNotFoundException: allow
	at jdk.internal.loader.BuiltinClassLoader.loadClass(java.base@11.0.12/BuiltinClassLoader.java:581)
	at jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base@11.0.12/ClassLoaders.java:178)
	at java.lang.ClassLoader.loadClass(java.base@11.0.12/ClassLoader.java:522)
	at java.lang.Class.forName0(java.base@11.0.12/Native Method)
	at java.lang.Class.forName(java.base@11.0.12/Class.java:398)
	at java.lang.System.initPhase3(java.base@11.0.12/System.java:2050)

At least for Gerrit Code Review, this diff seems to work in both cases (Java 11 and Java 17):

diff --git a/BUILD b/BUILD
index f6ec40e2aa..30d6547da5 100644
--- a/BUILD
+++ b/BUILD
@@ -4,9 +4,9 @@ load("//tools/bzl:pkg_war.bzl", "pkg_war")
 package(default_visibility = ["//visibility:public"])
 
 config_setting(
-    name = "java11",
+    name = "java17",
     values = {
-        "java_toolchain": "@bazel_tools//tools/jdk:toolchain_java11",
+        "java_language_version": "17",
     },
 )
 
diff --git a/tools/bzl/junit.bzl b/tools/bzl/junit.bzl
index 0e7ea3a7e1..0e1fa5db49 100644
--- a/tools/bzl/junit.bzl
+++ b/tools/bzl/junit.bzl
@@ -75,6 +75,10 @@ POST_JDK8_OPTS = [
     "-Djava.locale.providers=COMPAT,CLDR,SPI",
 ]
 
+POST_JDK17_OPTS = [
+    "-Djava.security.manager=allow",
+]
+
 def junit_tests(name, srcs, **kwargs):
     s_name = name.replace("-", "_") + "TestSuite"
     _gen_suite(
@@ -82,7 +86,12 @@ def junit_tests(name, srcs, **kwargs):
         srcs = srcs,
         outname = s_name,
     )
-    jvm_flags = kwargs.get("jvm_flags", []) + POST_JDK8_OPTS
+    jvm_flags = kwargs.get("jvm_flags", [])
+    jvm_flags = jvm_flags + select({
+         "//:java17": POST_JDK8_OPTS + POST_JDK17_OPTS,
+         "//conditions:default": POST_JDK8_OPTS,
+     })
+    POST_JDK8_OPTS
     java_test(
         name = name,
         test_class = s_name,

The better question is: can we somehow move the fix to upstream Bazel, to avoid proliferation of this diff to downstream projects?

Alone for Gerrit Code Review, we have 100+ plugins, where Bazlets Repository is used, and we also have JGit and Gitiles, that have their own Bazel build tool chains.

See also this CL: [1] for the whole fix in Gerrit.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/291864

[cushon]

I agree it would be good to solve this in Bazel.

Medium term we may want to use the API for preventing calls to System.exit if https://bugs.openjdk.java.net/browse/JDK-8199704 happens, or else we could consider removing this security manager.

It would be nice to have a short term fix, and automatically passing -Djava.security.manager=allow to java_test when targeting 17 seems like a reasonable option. I think it should probably be based on --java_runtime_version, though? It's valid to configure e.g. compiling at the Java 11 language level and then running the tests on JDK 17.

[davido]

I think it should probably be based on --java_runtime_version, though?

Right. However, at least for RBE to work with remote JDK, we need to pass --java_runtime_version=remotejdk_17:

# Builds using remotejdk_17, executes using remotejdk_17
build:java17 --java_language_version=17
build:java17 --java_runtime_version=remotejdk_17
build:java17 --tool_java_language_version=17
build:java17 --tool_java_runtime_version=remotejdk_17

So that config_setting should be defined as:

config_setting(
     name = "java17",
     values = {
        "java_runtime_version": "remotejdk_17",
     },
 )

[Jonpez2]

As of jdk 18, this has become a hard failure. What are the current thoughts on upstreaming the ‘allow’ setting conditional on jdk runtime >= 17? Or is there some other approach being considered?

Thanks!

[fmeum]

An alternative could be to have the test runner attach a Java agent that modifies the behavior of System.exit using ByteBuddy or ASM.

[cushon]

To confirm, isn't it a failure by default in 18 that can still be disabled by passing -Djava.security.manager=allow?

Internally we are currently passing that flag and waiting to see if https://bugs.openjdk.java.net/browse/JDK-8199704 happens before the API is removed.

Another option being considered is to remove the test SecurityManager sooner and rely on static analysis to prevent calls to System.exit.

[Jonpez2]

Right, true, it can be worked around by everyone doing that. I was just wondering whether in the interim, instead of everyone c&p'ing that into various bits of their code, it would be possible to handle it upstream.

[cushon]

I think it makes sense to add a shared workaround to Bazel in the interim. I'm not sure what the cleanest way only pass -Djava.security.manager=allow to JDK versions that recognize it is, though.

[jwhpryor]

I recently ran into this issue with JNI Bind and your solution worked!

That said, it would be great to have a solution that's future proof. Is this expected to be fixed in Bazel itself? I would think this would effect all Bazel users that use java_test?

Sorry for the basic question, I didn't think I'd done anything special in my setup, but I'm not very competent with setting this stuff up so I'm wondering if I'm doing something non-standard.

Thanks!

[cushon]

I think it makes sense to add a shared workaround to Bazel in the interim. I'm not sure what the cleanest way only pass -Djava.security.manager=allow to JDK versions that recognize it is, though.

This is another example where something like java_runtime(..., feature_version = 11) would be helpful.

Related: #6354 (comment)

[fmeum]

@bazel-io flag

[keertk]

@bazel-io fork 6.2.0