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.

Sign up for GitHub

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

Jump to bottom

Stop using ValidationSecurityManager as SecurityManager #1983

Merged
merged 2 commits into from Apr 8, 2022
Merged

Stop using ValidationSecurityManager as SecurityManager #1983

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efee878
Stop using ValidationSecurityManager as SecurityManager
wborn Mar 17, 2022
1ae85fc
Merge branch 'master' into remove-securitymanager
KengoTODA Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
### Fixed
- Bumped Saxon-HE from 10.6 to 11.2 ([#1955](https://github.com/spotbugs/spotbugs/pull/1955))
- Fixed traversal of nested archives governed by `-nested:true` ([#1930](https://github.com/spotbugs/spotbugs/pull/1930))
- Warnings of deprecated System::setSecurityManager calls on Java 17 ([#1983](https://github.com/spotbugs/spotbugs/pull/1983))

## 4.6.0 - 2022-03-08
### Fixed
Expand Down
106 changes: 0 additions & 106 deletions ...bugs-tests/src/test/java/edu/umd/cs/findbugs/ba/jsr305/ValidationSecurityManagerTest.java
View file
Open in desktop

This file was deleted.

19 changes: 1 addition & 18 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/ba/jsr305/TypeQualifierValue.java
View file
Open in desktop
Expand Up @@ -148,15 +148,6 @@ private TypeQualifierValue(ClassDescriptor typeQualifier, @CheckForNull Object v
try {
Global.getAnalysisCache().getClassAnalysis(ClassData.class, checkerName);

// found it.
Copy link

Choose a reason for hiding this comment

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

Isn't it the better approach, to use env variable to on/off it,? This bug is just for "recent" jdk's only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the maintainers can tell if we can part with the functionality provided by the ValidationSecurityManager. Does it solve an issue you have?

Copy link
Member

Choose a reason for hiding this comment

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

🤔 I'm not so sure about why we need to set a security manager here. The comment on 4a9eaf4 gave no hint.

At least existing test cases got no effect from this deletion. It also has already been disabled when we run SpotBugs as an IDE plugin. So I think it's OK to remove it.

😊 @spotbugs/core-devs please share your thought if you have.

BTW SpotBugs usually uses system property instead of environment variables, to switch behavior. Just like the DEBUG_CLASSLOADING variable in this class. So if we need a feature toggle, better to use a system property.

Copy link

Choose a reason for hiding this comment

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

Would it make sense to check the JVM version before calling, and only call on older platforms where it makes sense (like Java 8 as mentioned above)?

SecurityManager m = System.getSecurityManager();
if (m == null) {
if (DEBUG_CLASSLOADING) {
System.out.println("Setting ValidationSecurityManager");
}
System.setSecurityManager(ValidationSecurityManager.INSTANCE);
}

Class<?> c = ValidatorClassLoader.INSTANCE.loadClass(checkerName.getDottedClassName());
if (TypeQualifierValidator.class.isAssignableFrom(c)) {

Expand Down Expand Up @@ -187,14 +178,6 @@ private TypeQualifierValue(ClassDescriptor typeQualifier, @CheckForNull Object v
AnalysisContext.logError("Unable to construct type qualifier checker " + checkerName + " due to "
+ e.getClass().getSimpleName() + ":" + e.getMessage());
}
} else if (DEBUG_CLASSLOADING) {
SecurityManager m = System.getSecurityManager();
if (m == null) {
if (DEBUG_CLASSLOADING) {
System.out.println("Setting ValidationSecurityManager");
}
System.setSecurityManager(ValidationSecurityManager.INSTANCE);
}
}
}
this.validator = validator1;
Expand Down Expand Up @@ -269,7 +252,7 @@ public When validate(@CheckForNull Object constantValue) {
Profiler profiler = analysisCache.getProfiler();
profiler.start(validator.getClass());
try {
return ValidationSecurityManager.sandboxedValidation(proxy, validator, constantValue);
return validator.forConstantValue(proxy, constantValue);
} catch (Exception e) {
AnalysisContext.logError("Error executing custom validator for " + typeQualifier + " " + constantValue, e);
return When.UNKNOWN;
Expand Down
130 changes: 0 additions & 130 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/ba/jsr305/ValidationSecurityManager.java
View file
Open in desktop

This file was deleted.

78 changes: 0 additions & 78 deletions spotbugsTestCases/src/java/jsr305/validation/CheckValidatorSandboxing.java
View file
Open in desktop

This file was deleted.