diff --git a/java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected b/java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected index 7a1a986b2aa1..8e79288d9402 100644 --- a/java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected +++ b/java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected @@ -81,6 +81,7 @@ ql/java/ql/src/Violations of Best Practice/Naming Conventions/SameNameAsSuper.ql ql/java/ql/src/Violations of Best Practice/Records/IgnoredSerializationMembersOfRecordClass.ql ql/java/ql/src/Violations of Best Practice/SpecialCharactersInLiterals/NonExplicitControlAndWhitespaceCharsInLiterals.ql ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToStringToString.ql +ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql ql/java/ql/src/Violations of Best Practice/Undesirable Calls/DefaultToString.ql ql/java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql ql/java/ql/src/Violations of Best Practice/Undesirable Calls/PrintLnArray.ql diff --git a/java/ql/integration-tests/java/query-suite/not_included_in_qls.expected b/java/ql/integration-tests/java/query-suite/not_included_in_qls.expected index 1f58e51ad800..a0bc27b8051a 100644 --- a/java/ql/integration-tests/java/query-suite/not_included_in_qls.expected +++ b/java/ql/integration-tests/java/query-suite/not_included_in_qls.expected @@ -187,7 +187,6 @@ ql/java/ql/src/Violations of Best Practice/Magic Constants/MagicNumbersUseConsta ql/java/ql/src/Violations of Best Practice/Magic Constants/MagicStringsUseConstant.ql ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverridesNames.ql ql/java/ql/src/Violations of Best Practice/Naming Conventions/LocalShadowsField.ql -ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql ql/java/ql/src/Violations of Best Practice/Undesirable Calls/GarbageCollection.ql ql/java/ql/src/Violations of Best Practice/legacy/AutoBoxing.ql ql/java/ql/src/Violations of Best Practice/legacy/FinallyMayNotComplete.ql diff --git a/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.java b/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.java index da7277aa25c3..50e0fb1cbda0 100644 --- a/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.java +++ b/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.java @@ -4,7 +4,7 @@ boolean write(String[] s) { try { output.write(s.getBytes()); } catch (IOException e) { - System.exit(1); + System.exit(1); // BAD: Should handle or propagate error instead of exiting } return true; } @@ -16,9 +16,30 @@ public void run() { // ... // Perform tasks ... // ... - System.exit(0); + System.exit(0); // BAD: Should return status or throw exception } public static void main(String[] args) { - new Action(args).run(); + new Action().run(); } -} \ No newline at end of file +} + +// 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 + } + } +} diff --git a/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.qhelp b/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.qhelp index e4d4fa7a7f0e..6992a734607c 100644 --- a/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.qhelp +++ b/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.qhelp @@ -13,17 +13,20 @@ program state from being written to disk consistently.

It is sometimes considered acceptable to call System.exit from a program's main method in order to indicate the overall exit status -of the program. Such calls are an exception to this rule.

+of the program. The main method should be the primary place +where exit conditions are handled, as it represents the natural termination point +of the application. Such calls are an exception to this rule.

-

It is usually preferable to use a different mechanism for reporting -failure conditions. Consider returning a special value (perhaps -null) that users of the current method check for and -recover from appropriately. Alternatively, throw a suitable exception, which -unwinds the stack and allows properly written code to clean up after itself, -while leaving other threads undisturbed.

+

Instead of calling System.exit from non-main methods, prefer to propagate +errors upward to the main method where they can be handled appropriately. +Consider returning a special value (perhaps null) that users of the current +method check for and recover from appropriately. Alternatively, throw a suitable exception, +which unwinds the stack and allows properly written code to clean up after itself, +while leaving other threads undisturbed. The main method can then catch +these exceptions and decide whether to exit the program and with what exit code.

@@ -38,12 +41,14 @@ upwards and be handled by a method that knows how to recover.

Problem 2 is more subtle. In this example, there is just one entry point to the program (the main method), which constructs an Action and performs it. Action.run calls -System.exit to indicate successful completion. Consider, -however, how this code might be integrated in an application server that -constructs Action instances and calls +System.exit to indicate successful completion. Instead, the +run method should return a status code or throw an exception +on failure, allowing the main method to decide whether to exit +and with what exit code. Consider how this code might be integrated in an +application server that constructs Action instances and calls run on them without going through main. The fact that run terminates the JVM instead of returning its -exit code as an integer makes that use-case impossible.

+exit code makes that use-case impossible.

diff --git a/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql b/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql index 93d7911694cf..c17141122d1e 100644 --- a/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql +++ b/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql @@ -4,26 +4,71 @@ * reuse and prevent important cleanup steps from running. * @kind problem * @problem.severity warning - * @precision low + * @precision medium * @id java/jvm-exit - * @tags reliability - * maintainability + * @tags quality + * reliability + * correctness * external/cwe/cwe-382 */ import java -from Method m, MethodCall sysexitCall, Method sysexit, Class system -where - sysexitCall = m.getACallSite(sysexit) and - (sysexit.hasName("exit") or sysexit.hasName("halt")) and - sysexit.getDeclaringType() = system and - ( - system.hasQualifiedName("java.lang", "System") or - system.hasQualifiedName("java.lang", "Runtime") - ) and - m.fromSource() and - not m instanceof MainMethod -select sysexitCall, - "Avoid calls to " + sysexit.getDeclaringType().getName() + "." + sysexit.getName() + - "() as this makes code harder to reuse." +/** + * A `Method` which, when called, causes the JVM to exit or halt. + * Explicitly includes these methods from the java standard library: + * - `java.lang.System.exit` + * - `java.lang.Runtime.halt` + * - `java.lang.Runtime.exit` + */ +class ExitOrHaltMethod extends Method { + ExitOrHaltMethod() { + exists(Class system | this.getDeclaringType() = system | + this.hasName("exit") and + system.hasQualifiedName("java.lang", ["System", "Runtime"]) + or + this.hasName("halt") and + system.hasQualifiedName("java.lang", "Runtime") + ) + } +} + +/** A `MethodCall` to an `ExitOrHaltMethod`, which causes the JVM to exit abruptly. */ +class ExitOrHaltMethodCall extends MethodCall { + ExitOrHaltMethodCall() { + exists(ExitOrHaltMethod exitMethod | this.getMethod() = exitMethod | + exists(SourceMethodNotMainOrTest srcMethod | this = srcMethod.getACallSite(exitMethod)) + ) + } +} + +/** + * An intentional `MethodCall` to a system or runtime "exit" method, such as for + * functions which exist for the purpose of exiting the program. Assumes that a an exit method + * call within a method is intentional if the exit code is passed from a parameter of the + * enclosing method. + */ +class IntentionalExitMethodCall extends ExitOrHaltMethodCall { + IntentionalExitMethodCall() { + this.getMethod().hasName("exit") and + this.getAnArgument() = this.getEnclosingCallable().getAParameter().getAnAccess() + } +} + +/** + * A `Method` that is defined in source code and is not a `MainMethod` or a `LikelyTestMethod`. + */ +class SourceMethodNotMainOrTest extends Method { + SourceMethodNotMainOrTest() { + this.fromSource() and + not this instanceof MainMethod and + not this instanceof LikelyTestMethod and + not this.getEnclosingCallable() instanceof LikelyTestMethod + } +} + +from ExitOrHaltMethodCall mc +where not mc instanceof IntentionalExitMethodCall +select mc, + "Avoid calls to " + mc.getMethod().getDeclaringType().getName() + "." + mc.getMethod().getName() + + "() as this prevents runtime cleanup and makes code harder to reuse." diff --git a/java/ql/test/query-tests/CallsToSystemExit/CallsToSystemExit.expected b/java/ql/test/query-tests/CallsToSystemExit/CallsToSystemExit.expected new file mode 100644 index 000000000000..cad6d0097c7b --- /dev/null +++ b/java/ql/test/query-tests/CallsToSystemExit/CallsToSystemExit.expected @@ -0,0 +1,6 @@ +| ExampleRuntimeExit.java:22:17:22:44 | exit(...) | Avoid calls to Runtime.exit() as this prevents runtime cleanup and makes code harder to reuse. | +| ExampleRuntimeExit.java:25:17:25:44 | exit(...) | Avoid calls to Runtime.exit() as this prevents runtime cleanup and makes code harder to reuse. | +| ExampleRuntimeHalt.java:18:17:18:44 | halt(...) | Avoid calls to Runtime.halt() as this prevents runtime cleanup and makes code harder to reuse. | +| ExampleRuntimeHalt.java:21:17:21:44 | halt(...) | Avoid calls to Runtime.halt() as this prevents runtime cleanup and makes code harder to reuse. | +| ExampleSystemExit.java:22:17:22:30 | exit(...) | Avoid calls to System.exit() as this prevents runtime cleanup and makes code harder to reuse. | +| ExampleSystemExit.java:25:17:25:30 | exit(...) | Avoid calls to System.exit() as this prevents runtime cleanup and makes code harder to reuse. | diff --git a/java/ql/test/query-tests/CallsToSystemExit/CallsToSystemExit.qlref b/java/ql/test/query-tests/CallsToSystemExit/CallsToSystemExit.qlref new file mode 100644 index 000000000000..4561fcfcfd04 --- /dev/null +++ b/java/ql/test/query-tests/CallsToSystemExit/CallsToSystemExit.qlref @@ -0,0 +1,2 @@ +query: Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/java/ql/test/query-tests/CallsToSystemExit/ExampleRuntimeExit.java b/java/ql/test/query-tests/CallsToSystemExit/ExampleRuntimeExit.java new file mode 100644 index 000000000000..13fd53b11419 --- /dev/null +++ b/java/ql/test/query-tests/CallsToSystemExit/ExampleRuntimeExit.java @@ -0,0 +1,37 @@ +import java.io.FileOutputStream; +import java.io.IOException; + +public class ExampleRuntimeExit { + + public static void main(String[] args) { + Action action = new Action(); + try { + action.run(); + } catch (Exception e) { + printUsageAndExit(e.getMessage(), 1); + } + Runtime.getRuntime().exit(0); // COMPLIANT + } + + static class Action { + public void run() { + try { + FileOutputStream fos = new FileOutputStream("output.txt"); + fos.write("Hello, World!".getBytes()); + fos.close(); + Runtime.getRuntime().exit(0); // $ Alert + } catch (IOException e) { + e.printStackTrace(); + Runtime.getRuntime().exit(1); // $ Alert + } catch (Exception e) { + // re-throw the exception + throw e; + } + } + } + + protected static void printUsageAndExit(final String message, final int exitCode) { + System.err.println("Usage: : " + message); + Runtime.getRuntime().exit(exitCode); // COMPLIANT + } +} diff --git a/java/ql/test/query-tests/CallsToSystemExit/ExampleRuntimeHalt.java b/java/ql/test/query-tests/CallsToSystemExit/ExampleRuntimeHalt.java new file mode 100644 index 000000000000..b1d4be04f201 --- /dev/null +++ b/java/ql/test/query-tests/CallsToSystemExit/ExampleRuntimeHalt.java @@ -0,0 +1,25 @@ +import java.io.FileOutputStream; +import java.io.IOException; + +public class ExampleRuntimeHalt { + + public static void main(String[] args) { + Action action = new Action(); + action.run(); + Runtime.getRuntime().halt(0); // COMPLIANT + } + + static class Action { + public void run() { + try { + FileOutputStream fos = new FileOutputStream("output.txt"); + fos.write("Hello, World!".getBytes()); + fos.close(); + Runtime.getRuntime().halt(0); // $ Alert + } catch (IOException e) { + e.printStackTrace(); + Runtime.getRuntime().halt(1); // $ Alert + } + } + } +} diff --git a/java/ql/test/query-tests/CallsToSystemExit/ExampleSystemExit.java b/java/ql/test/query-tests/CallsToSystemExit/ExampleSystemExit.java new file mode 100644 index 000000000000..dece6e689ba1 --- /dev/null +++ b/java/ql/test/query-tests/CallsToSystemExit/ExampleSystemExit.java @@ -0,0 +1,37 @@ +import java.io.FileOutputStream; +import java.io.IOException; + +public class ExampleSystemExit { + + public static void main(String[] args) { + Action action = new Action(); + try { + action.run(); + } catch (Exception e) { + printUsageAndExit(e.getMessage(), 1); + } + System.exit(0); // COMPLIANT + } + + static class Action { + public void run() { + try { + FileOutputStream fos = new FileOutputStream("output.txt"); + fos.write("Hello, World!".getBytes()); + fos.close(); + System.exit(0); // $ Alert + } catch (IOException e) { + e.printStackTrace(); + System.exit(1); // $ Alert + } catch (Exception e) { + // re-throw the exception + throw e; + } + } + } + + protected static void printUsageAndExit(final String message, final int exitCode) { + System.err.println("Usage: : " + message); + System.exit(exitCode); // COMPLIANT + } +}