-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Java: Enhance java/jvm-exit
query and add to quality
#20190
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
base: main
Are you sure you want to change the base?
Conversation
QHelp previews: java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.qhelpForcible JVM terminationCalling one of the methods It is sometimes considered acceptable to call RecommendationInstead of calling ExampleIn the following example, problem 1 shows that Problem 2 is more subtle. In this example, there is just one entry point to the program (the // Problem 1: Miss out cleanup code
class FileOutput {
boolean write(String[] s) {
try {
output.write(s.getBytes());
} catch (IOException e) {
System.exit(1); // BAD: Should handle or propagate error instead of exiting
}
return true;
}
}
// Problem 2: Make code reuse difficult
class Action {
public void run() {
// ...
// Perform tasks ...
// ...
System.exit(0); // BAD: Should return status or throw exception
}
public static void main(String[] args) {
new Action().run();
}
}
// Good example: Proper error handling
class BetterAction {
public int run() throws Exception {
// ...
// Perform tasks ...
// ...
return 0; // Return status instead of calling System.exit
}
public static void main(String[] args) {
try {
BetterAction action = new BetterAction();
int exitCode = action.run();
System.exit(exitCode); // GOOD: Exit from main method
} catch (Exception e) {
System.err.println("Error: " + e.getMessage());
System.exit(1); // GOOD: Exit from main method on error
}
}
} References
|
java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql
Fixed
Show fixed
Hide fixed
2cf0828
to
5a643be
Compare
5a643be
to
f6aad96
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the java/jvm-exit
query by improving its accuracy and reducing false positives, then promotes it from low to medium precision and includes it in the quality query suite.
- Enhanced the query logic to better distinguish between legitimate and problematic JVM exit calls
- Added comprehensive test coverage with examples for System.exit(), Runtime.exit(), and Runtime.halt()
- Updated documentation with improved guidance on proper error handling patterns
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
CallsToSystemExit.ql | Refactored query with improved filtering logic to reduce false positives and better precision |
CallsToSystemExit.qhelp | Enhanced documentation with clearer recommendations and better examples |
CallsToSystemExit.java | Extended example code with both problematic and recommended patterns |
Test files (ExampleSystemExit.java, etc.) | Added comprehensive test cases covering different exit method scenarios |
Query suite files | Added the enhanced query to the quality suite and removed from excluded list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvement!
@@ -4,26 +4,71 @@ | |||
* reuse and prevent important cleanup steps from running. | |||
* @kind problem | |||
* @problem.severity warning | |||
* @precision low | |||
* @precision medium |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, that setting this to medium only adds the query to java-code-quality-extended
(and not java-code-quality
). Maybe it is worth reflecting it in the PR description.
The DCA run for the query looks fine in the sense that, if the query was included in the code-quality
suite it wouldn't cause any overall performance regressions and it appears to produce some results. However, it is worth noting that
- The query was only included in the
nightly.qls
in the DCA run as the precision was set tohigh
before the force push (which set it tomedium
). - The query is only included in the second variant of DCA (due to the above), due to the change in precision.
As the query is only included in query execution for the second DCA variant we are not testing the performance and/or result implications for the changes made to the query in this PR. I think it will make sense to make a DCA run that specifically targets this query (to get the result and performance diff) to get the query included in the executions on both DCA variants.
java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql
Outdated
Show resolved
Hide resolved
this.fromSource() and | ||
not this instanceof MainMethod and | ||
not this instanceof LikelyTestMethod and | ||
not this.getEnclosingCallable() instanceof LikelyTestMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It this to capture local classes in test methods? If so, can local classes be nested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that looks good.
…sToSystemExit.ql Co-authored-by: Michael Nebel <michaelnebel@github.com>
681e568
to
71e827e
Compare
java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql
Fixed
Show fixed
Hide fixed
71e827e
to
eb6e9b8
Compare
java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql
Outdated
Show resolved
Hide resolved
…sToSystemExit.ql Co-authored-by: Michael Nebel <michaelnebel@github.com>
This PR integrates improvements from the
java/jvm-exit-prevents-cleanup-and-reuse
query incodeql-quality-queries
into the main CodeQL repository. The enhanced query now provides better detection of problematic JVM termination calls with reduced false positives and has been promoted quality query suite.It was decided to keep this query at low/medium precision, as there are quite a few false positives. This is largely due to the way command-line interfaces (CLI) and build tools handle application termination: they often use exit calls in ways that differ from standard software practices. As a result, the query may incorrectly flag these legitimate uses as potential issues, which impacts its overall accuracy.
Autofix appears feasible, though acceptance may vary depending on the specific case, as some instances could require more extensive refactoring within the repository.
Note: Since the query's precision is low/medium, it will only be included in the code quality extended suite, and not in the main code quality suite.