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

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

Closed
wants to merge 1 commit into from

Conversation

thirtyseven
Copy link
Contributor

@thirtyseven thirtyseven commented Jan 17, 2024

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 thirtyseven changed the title Enable security manager for ScalacWorker to allow using JDK 18+ Draft: Enable security manager for ScalacWorker to allow using JDK 18+ Jan 17, 2024
@thirtyseven
Copy link
Contributor Author

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

@srdo
Copy link

srdo commented Jan 22, 2024

@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 bazelbuild/bazel@7556e11#diff-ece537407bf4ad71e14df88b62cc07ad0666e3938a3422d91e8714962c004d48R57. Probably rules_scala could do something similar.

I hope this is helpful :)

@simuons
Copy link
Collaborator

simuons commented Feb 7, 2024

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

srdo commented Feb 7, 2024

@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 added a commit to simuons/rules_scala that referenced this pull request Mar 19, 2024
This is an emergency pr to make build pass on ci

Probably tool_java_runtime_version started to resolve to jdk 21

And builds are failing with
`java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release`

Need to solve bazelbuild#1539 asap
@simuons
Copy link
Collaborator

simuons commented Apr 23, 2024

This was resolved by #1556

@simuons simuons closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants