From dcf6b6dcb0a90c3381b779fe41d0f69c7337a3c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stig=20D=C3=B8ssing?= Date: Tue, 18 Mar 2025 09:37:20 +0100 Subject: [PATCH] Add Java 24 compatibility and remove SecurityManager support Java 24 degrades the SecurityManager into a hollow shell that just throws exceptions. Later versions will remove the interface entirely. This commit removes references to the SecurityManager from rules_scala, which will make tests that call System.exit crash the worker. It is often possible to edit code to avoid calling System.exit, and guarding against it requires more effort in Java 24+, likely involving attaching an agent. Until someone actually has that need, it doesn't seem worth doing. Bazel is doing the same thing for now, see bazelbuild/bazel#24354 Add -Dcom.google.testing.junit.runner.shouldInstallTestSecurityManager=false for junit tests. While recent Bazel versions set this automatically, older Bazel versions do not, and since we're removing the flag that allows the SM, we need to set this to prevent crashing for people running older Bazel versions with Java 17+ Also exclude the .ijwb directory from the buildifier lint check. Failures due to those files are irritating when running the test locally. --- scala/private/phases/phase_coverage.bzl | 6 +---- scala/private/phases/phase_scalafmt.bzl | 6 +---- .../private/phases/phase_write_executable.bzl | 19 +++----------- scala/private/rule_impls.bzl | 11 +------- scala_proto/private/scala_proto_aspect.bzl | 7 +++--- .../io/bazel/rulesscala/worker/Worker.java | 25 ------------------- .../bazel/rulesscala/worker/WorkerTest.java | 3 --- tools/BUILD | 6 +++++ twitter_scrooge/twitter_scrooge.bzl | 7 +++--- 9 files changed, 19 insertions(+), 71 deletions(-) diff --git a/scala/private/phases/phase_coverage.bzl b/scala/private/phases/phase_coverage.bzl index 4cd0d5dca..3432731cf 100644 --- a/scala/private/phases/phase_coverage.bzl +++ b/scala/private/phases/phase_coverage.bzl @@ -8,10 +8,6 @@ load( "//scala/private:coverage_replacements_provider.bzl", _coverage_replacements_provider = "coverage_replacements_provider", ) -load( - "//scala/private:rule_impls.bzl", - _allow_security_manager = "allow_security_manager", -) def phase_coverage_library(ctx, p): args = struct( @@ -64,7 +60,7 @@ def _phase_coverage(ctx, p, srcjars): outputs = [output_jar], executable = ctx.attr._code_coverage_instrumentation_worker.files_to_run, execution_requirements = {"supports-workers": "1"}, - arguments = ["--jvm_flag=%s" % f for f in _allow_security_manager(ctx)] + [args], + arguments = [args], ) replacements = {input_jar: output_jar} diff --git a/scala/private/phases/phase_scalafmt.bzl b/scala/private/phases/phase_scalafmt.bzl index 51e73a4d8..290952aef 100644 --- a/scala/private/phases/phase_scalafmt.bzl +++ b/scala/private/phases/phase_scalafmt.bzl @@ -4,10 +4,6 @@ # Outputs to format the scala files when it is explicitly specified # load("//scala/private:paths.bzl", _scala_extension = "scala_extension") -load( - "//scala/private:rule_impls.bzl", - _allow_security_manager = "allow_security_manager", -) def phase_scalafmt(ctx, p): if ctx.attr.format: @@ -26,7 +22,7 @@ def _build_format(ctx): files = [] srcs = [] manifest_content = [] - jvm_flags = ["-Dfile.encoding=UTF-8"] + _allow_security_manager(ctx) + jvm_flags = ["-Dfile.encoding=UTF-8"] for src in ctx.files.srcs: # only format scala source files, not generated files if src.path.endswith(_scala_extension) and src.is_source: diff --git a/scala/private/phases/phase_write_executable.bzl b/scala/private/phases/phase_write_executable.bzl index afebeb3e7..52a144ddc 100644 --- a/scala/private/phases/phase_write_executable.bzl +++ b/scala/private/phases/phase_write_executable.bzl @@ -1,3 +1,5 @@ +load("//scala/private:common.bzl", "rlocationpath_from_file") + # # PHASE: write executable # @@ -5,16 +7,13 @@ # load( "//scala/private:rule_impls.bzl", - "allow_security_manager", "expand_location", "first_non_empty", "is_windows", "java_bin", "java_bin_windows", "runfiles_root", - "specified_java_runtime", ) -load("//scala/private:common.bzl", "rlocationpath_from_file") def phase_write_executable_scalatest(ctx, p): # jvm_flags passed in on the target override scala_test_jvm_flags passed in on the @@ -29,7 +28,7 @@ def phase_write_executable_scalatest(ctx, p): jvm_flags = [ "-DRULES_SCALA_MAIN_WS_NAME=%s" % ctx.workspace_name, "-DRULES_SCALA_ARGS_FILE=%s" % rlocationpath_from_file(ctx, p.runfiles.args_file), - ] + expand_location(ctx, final_jvm_flags) + _allow_security_manager_for_specified_java_runtime(ctx), + ] + expand_location(ctx, final_jvm_flags), use_jacoco = ctx.configuration.coverage_enabled, ) return _phase_write_executable_default(ctx, p, args) @@ -44,7 +43,7 @@ def phase_write_executable_repl(ctx, p): def phase_write_executable_junit_test(ctx, p): args = struct( rjars = p.coverage_runfiles.rjars, - jvm_flags = p.jvm_flags + ctx.attr.jvm_flags + _allow_security_manager_for_specified_java_runtime(ctx), + jvm_flags = p.jvm_flags + ctx.attr.jvm_flags + ["-Dcom.google.testing.junit.runner.shouldInstallTestSecurityManager=false"], main_class = "com.google.testing.junit.runner.BazelTestRunner", use_jacoco = ctx.configuration.coverage_enabled, ) @@ -186,13 +185,3 @@ def _jar_path_based_on_java_bin(ctx): java_bin_var = java_bin(ctx) jar_path = java_bin_var.rpartition("/")[0] + "/jar" return jar_path - -# Allow security manager for generated test executables if they will be run with jdk >= 17 -def _allow_security_manager_for_specified_java_runtime(ctx): - return allow_security_manager( - ctx, - specified_java_runtime( - ctx, - default_runtime = ctx.attr._java_runtime[java_common.JavaRuntimeInfo], - ), - ) diff --git a/scala/private/rule_impls.bzl b/scala/private/rule_impls.bzl index 9d5740c2b..0b6498c6f 100644 --- a/scala/private/rule_impls.bzl +++ b/scala/private/rule_impls.bzl @@ -124,7 +124,7 @@ def compile_scala( # TODO: scalac worker is run with @bazel_tools//tools/jdk:runtime_toolchain_type # which is different from rules_java where compilation runtime is used from # @bazel_tools//tools/jdk:toolchain_type - final_scalac_jvm_flags = first_non_empty(scalac_jvm_flags, toolchain.scalac_jvm_flags) + allow_security_manager(ctx) + final_scalac_jvm_flags = first_non_empty(scalac_jvm_flags, toolchain.scalac_jvm_flags) ctx.actions.run( inputs = ins, @@ -221,12 +221,3 @@ def java_bin_windows(ctx): def is_windows(ctx): return ctx.configuration.host_path_separator == ";" - -# Return a jvm flag allowing security manager for jdk runtime >= 17 -# If no runtime is supplied then runtime is taken from ctx.attr._java_host_runtime -# This must be a runtime used in generated java_binary script (usually workers using SecurityManager) -def allow_security_manager(ctx, runtime = None): - java_runtime = runtime if runtime else ctx.attr._java_host_runtime[java_common.JavaRuntimeInfo] - - # Bazel 5.x doesn't have java_runtime.version defined - return ["-Djava.security.manager=allow"] if hasattr(java_runtime, "version") and java_runtime.version >= 17 else [] diff --git a/scala_proto/private/scala_proto_aspect.bzl b/scala_proto/private/scala_proto_aspect.bzl index e4efd9e55..7abb89b13 100644 --- a/scala_proto/private/scala_proto_aspect.bzl +++ b/scala_proto/private/scala_proto_aspect.bzl @@ -1,19 +1,18 @@ +load("@bazel_skylib//lib:dicts.bzl", "dicts") load("@rules_proto//proto:defs.bzl", "ProtoInfo") load("//scala/private:common.bzl", "write_manifest_file") load("//scala/private:dependency.bzl", "legacy_unclear_dependency_info_for_protobuf_scrooge") +load("//scala/private:phases/api.bzl", "extras_phases", "run_aspect_phases") load( "//scala/private:rule_impls.bzl", "compile_scala", "specified_java_compile_toolchain", - _allow_security_manager = "allow_security_manager", ) load("//scala/private/toolchain_deps:toolchain_deps.bzl", "find_deps_info_on") load( "//scala_proto/private:scala_proto_aspect_provider.bzl", "ScalaProtoAspectInfo", ) -load("//scala/private:phases/api.bzl", "extras_phases", "run_aspect_phases") -load("@bazel_skylib//lib:dicts.bzl", "dicts") def _import_paths(proto, ctx): # Under Bazel 7.x, direct_sources from generated protos may still contain @@ -77,7 +76,7 @@ def _generate_sources(ctx, toolchain, proto): ctx.actions.run( executable = toolchain.worker, - arguments = ["--jvm_flag=%s" % f for f in _allow_security_manager(ctx)] + [toolchain.worker_flags, args], + arguments = [toolchain.worker_flags, args], inputs = depset(transitive = [descriptors, toolchain.generators_jars]), outputs = outputs.values(), tools = [toolchain.protoc], diff --git a/src/java/io/bazel/rulesscala/worker/Worker.java b/src/java/io/bazel/rulesscala/worker/Worker.java index e5ed754bb..dbf7b39cb 100644 --- a/src/java/io/bazel/rulesscala/worker/Worker.java +++ b/src/java/io/bazel/rulesscala/worker/Worker.java @@ -9,10 +9,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Paths; -import java.security.Permission; import java.util.List; -import java.util.regex.Matcher; -import java.util.regex.Pattern; /** * A base for JVM workers. @@ -54,15 +51,6 @@ public static void workerMain(String workerArgs[], Interface workerInterface) th /** The main loop for persistent worker processes */ private static void persistentWorkerMain(Interface workerInterface) { - System.setSecurityManager( - new SecurityManager() { - @Override - public void checkPermission(Permission permission) { - Matcher matcher = exitPattern.matcher(permission.getName()); - if (matcher.find()) throw new ExitTrapped(Integer.parseInt(matcher.group(1))); - } - }); - InputStream stdin = System.in; PrintStream stdout = System.out; PrintStream stderr = System.err; @@ -94,8 +82,6 @@ public void checkPermission(Permission permission) { String[] workerArgs = stringListToArray(request.getArgumentsList()); String[] args = expandArgsIfArgsfile(workerArgs); workerInterface.work(args); - } catch (ExitTrapped e) { - code = e.code; } catch (Exception e) { if (e instanceof Interface.WorkerException) System.err.println(e.getMessage()); else e.printStackTrace(); @@ -177,17 +163,6 @@ public void reset() { } } - static class ExitTrapped extends RuntimeException { - final int code; - - ExitTrapped(int code) { - super(); - this.code = code; - } - } - - private static Pattern exitPattern = Pattern.compile("exitVM\\.(-?\\d+)"); - private static String[] stringListToArray(List argList) { int numArgs = argList.size(); String[] args = new String[numArgs]; diff --git a/src/java/io/bazel/rulesscala/worker/WorkerTest.java b/src/java/io/bazel/rulesscala/worker/WorkerTest.java index 11e75a164..88d8ea848 100644 --- a/src/java/io/bazel/rulesscala/worker/WorkerTest.java +++ b/src/java/io/bazel/rulesscala/worker/WorkerTest.java @@ -214,9 +214,6 @@ public void testBufferWriteReadAndReset() throws Exception { @AfterClass public static void teardown() { - // Persistent workers install a security manager. We need to - // reset it here so that our own process can exit! - System.setSecurityManager(null); } // Copied/modified from Bazel's MoreAsserts diff --git a/tools/BUILD b/tools/BUILD index 6ab71335a..84264eb76 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -18,6 +18,9 @@ WARNINGS_CONFIG = [ buildifier( name = "lint_check", + exclude_patterns = [ + "./.ijwb/*", + ], lint_mode = "warn", lint_warnings = WARNINGS_CONFIG, mode = "check", @@ -25,6 +28,9 @@ buildifier( buildifier( name = "lint_fix", + exclude_patterns = [ + "./.ijwb/*", + ], lint_mode = "fix", lint_warnings = WARNINGS_CONFIG, mode = "fix", diff --git a/twitter_scrooge/twitter_scrooge.bzl b/twitter_scrooge/twitter_scrooge.bzl index 05edd6af3..6d8ceb173 100644 --- a/twitter_scrooge/twitter_scrooge.bzl +++ b/twitter_scrooge/twitter_scrooge.bzl @@ -1,3 +1,4 @@ +load("@bazel_skylib//lib:dicts.bzl", "dicts") load( "//scala/private:common.bzl", "write_manifest_file", @@ -8,13 +9,11 @@ load( ) load( "//scala/private:rule_impls.bzl", - "allow_security_manager", "compile_java", "compile_scala", ) -load("//thrift:thrift_info.bzl", "ThriftInfo") load("//thrift:thrift.bzl", "merge_thrift_infos") -load("@bazel_skylib//lib:dicts.bzl", "dicts") +load("//thrift:thrift_info.bzl", "ThriftInfo") def _colon_paths(data): return ":".join([f.path for f in sorted(data)]) @@ -86,7 +85,7 @@ def _generate_jvm_code(ctx, label, compile_thrifts, include_thrifts, jar_output, # be correctly handled since the executable is a jvm app that will # consume the flags on startup. #arguments = ["--jvm_flag=%s" % flag for flag in ctx.attr.jvm_flags] + - arguments = ["--jvm_flag=%s" % f for f in allow_security_manager(ctx)] + ["@" + argfile.path], + arguments = ["@" + argfile.path], ) def _compiled_jar_file(actions, scrooge_jar):