rules_scala icon indicating copy to clipboard operation
rules_scala copied to clipboard

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

Open thirtyseven opened this issue 1 year ago • 4 comments

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 avatar Jan 17 '24 20:01 thirtyseven

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

thirtyseven avatar Jan 18 '24 07:01 thirtyseven

@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 :)

srdo avatar Jan 22 '24 16:01 srdo

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?

simuons avatar Feb 07 '24 09:02 simuons

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

srdo avatar Feb 07 '24 15:02 srdo

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

simuons avatar Apr 23 '24 06:04 simuons