Draft: Enable security manager for ScalacWorker to allow using JDK 18+

bazelbuild / rules_scala

Scala rules for Bazel

Apache License 2.0

360 stars 273 forks source link

Draft: Enable security manager for ScalacWorker to allow using JDK 18+ #1539

Closed thirtyseven closed 4 months ago

thirtyseven commented 7 months ago

Description

ScalacWorker.java uses the Java Security Manager to trap calls to System.exit(). In JDK 18+, enabling the security manager is disallowed by default, so a flag must be passed to allow it.

Motivation

When building with --tool_java_runtime=remotejdk_21, I found that this flag was needed when using Scalac persistent workers otherwise they would crash with an UnsupportedOperationException.

thirtyseven commented 7 months ago

Hm, looks like this flag is not backwards compatible with earlier JVMs, maybe some kind of version detection is needed.

srdo commented 7 months ago

@thirtyseven Thanks for looking into this.

We've been discussing this issue over here.

I think the version detection you mention has become available in JavaRuntimeInfo, there's an example here of how to interact with it https://github.com/bazelbuild/bazel/commit/7556e1107b666d10b660470a571631463c7eb4ec#diff-ece537407bf4ad71e14df88b62cc07ad0666e3938a3422d91e8714962c004d48R57. Probably rules_scala could do something similar.

I hope this is helpful :)

simuons commented 7 months ago

Hi, @thirtyseven, have you looked at what @srdo suggested? Maybe that would work?

But in general, since jdk does not provide any alternative for exit code trapping is see two options how we could go here:

remove security manager as it will go away in the future. Also not sure this exit code trapping saves us from anything (need more opinions on this)
configure -Djava.security.manager=allow on java toolchain level. I'll try to come up with an example and probably note in a readme (this means a little bit of inconvenience for users since preconfigured java toolchains doesn't work by default)

Frankly I'm not a big fan of ifs by java version which might break in future java releases.

WDYT would extra java toolchain configuration be a big hurdle?

srdo commented 7 months ago

@simuons I don't understand why adjusting the Java toolchain would be the only option other than removing the exit trap? Can't we detect the Java version from Starlark in the way I suggested, and add -Djava.security.manager=allow to those command invocations that actually lead to an attempt to use the SM (e.g. the scalac worker), and not all Java invocations?

I think that would keep basic toolchains working, and not cause any problems for existing users. It should keep working until the SM is actually removed from the JDK, and at that point there will hopefully be a replacement.

My understanding is that not trapping exits means that any user code (e.g. tests) that invokes System.exit will also cause the Scala worker to exit. Not sure what the consequences are of that?

Edit: A fourth option would be to add this flag unconditionally and drop support for Java 11, but I don't know if that's acceptable?

simuons commented 4 months ago

This was resolved by https://github.com/bazelbuild/rules_scala/pull/1556