Skip to content
This repository has been archived by the owner on Apr 19, 2023. It is now read-only.

Reduce overhead of the security manager #149

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Reduce overhead of the security manager #149

wants to merge 2 commits into from

Conversation

retronym
Copy link

Continues the work started in #11 by systematically overriding
all the other methods of SecurityManager for a fast path when
the base security manager is null.

Fixes #134

@retronym
Reduce overhead of the security manager
3366a9e 
Continues the work started in facebookarchive#11 by systematically overriding
all the other methods of SecurityManager for a fast path when
the base security manager is null.

Fixes facebookarchive#134
@retronym
Copy link
Author

In practice, overriding checkRead / checkWrite / checkDelete / checkExec would likely be enough, if you're prefer a targetted version to the blanket approach I've taken here.

jvican
jvican approved these changes Aug 28, 2018
View reviewed changes
Copy link
Contributor

@jvican jvican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@retronym
Copy link
Author

@sbalabanov How does this approach look?

JoshRosen
JoshRosen reviewed Sep 12, 2018
View reviewed changes
nailgun-server/src/main/java/com/martiansoftware/nailgun/NGSecurityManager.java Outdated
/** Avoid constructing a FilePermission object in checkRead if base manager is null. */
// Overrides below avoid the cost of creating Permissions objects if base manager is null.
// FilePermission, in particular, is expensive to create.

public void checkRead(String file) {
if (base != null) {
super.checkRead(file);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be base.checkRead() instead of super.checkRead()? I have the same question for the new overloads added in this PR. It looks like @qma asked the same question on the original patch: #11 (comment)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On reflection, I suspect that this was done for backwards-compatibility reasons, since overriding and delegating to base would be a behavior change whereas this change is only a performance optimization.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super.checkRead will eventually get to base.checkPermission. But it would be potentially more efficient to call base.checkRead directly here. The semantics should be the same for any well behaved SecurityManager.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a commit that delegates directly to the corresponding method.

JoshRosen added a commit to databricks/nailgun that referenced this pull request Sep 12, 2018
@JoshRosen
Backport of facebookarchive#149 on top of 0.9.1
bebdce6 
This patch backports the changes from facebookarchive#149
(commit 3366a9e) on top of Nailgun 0.9.1.
@styurin styurin force-pushed the master branch 2 times, most recently from 495eff8 to 948c51c Compare April 25, 2019 02:42
@unkarjedy
Copy link

unkarjedy commented Nov 15, 2021 •
edited
Loading

Note that SecurityManager is deprecated since Java 17 and will be removed in future releases:
https://openjdk.java.net/jeps/411

To move the Java Platform forward, we will deprecate the legacy Security Manager technology for removal from the JDK. We plan to deprecate and attenuate the capabilities of the Security Manager over a number of releases, simultaneously creating alternative APIs for such tasks as blocking System::exit and other use cases considered important enough to have replacements.

Currently, NailgunServer produces a warning in stderr:

WARNING: A terminally deprecated method in java.lang.System has been called
WARNING: System::setSecurityManager has been called by %s
WARNING: Please consider reporting this to the maintainers of %s
WARNING: System::setSecurityManager will be removed in a future release

ghost pushed a commit to JetBrains/nailgun that referenced this pull request Jan 30, 2023
@vasilmkd
Reduce the overhead of the custom security manager #SCL-20064
710ee7d 
Changes from facebookarchive#149.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JoshRosen JoshRosen JoshRosen left review comments

@jvican jvican jvican approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overhead of SecurityManager
4 participants
@retronym @unkarjedy @JoshRosen @jvican