Merge pull request #20190 from Napalys/java/jvm-exit-query-promotion

Java: Enhance `java/jvm-exit` query and add to quality

Author: Napalys

java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected

@@ -82,6 +82,7 @@ ql/java/ql/src/Violations of Best Practice/Records/IgnoredSerializationMembersOf
 ql/java/ql/src/Violations of Best Practice/SpecialCharactersInLiterals/NonExplicitControlAndWhitespaceCharsInLiterals.ql
 ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToRunFinalizersOnExit.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

java/ql/integration-tests/java/query-suite/not_included_in_qls.expected

@@ -186,7 +186,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

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();
     }
-}
+}
+
+// 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
+        }
+    }
+}

java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.qhelp

@@ -13,17 +13,20 @@ program state from being written to disk consistently.</p>
 
 <p>It is sometimes considered acceptable to call <code>System.exit</code>
 from a program's <code>main</code> method in order to indicate the overall exit status
-of the program. Such calls are an exception to this rule.</p>
+of the program. The <code>main</code> 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.</p>
 
 </overview>
 <recommendation>
 
-<p>It is usually preferable to use a different mechanism for reporting
-failure conditions. Consider returning a special value (perhaps
-<code>null</code>) 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.</p>
+<p>Instead of calling <code>System.exit</code> from non-main methods, prefer to propagate
+errors upward to the <code>main</code> method where they can be handled appropriately.
+Consider returning a special value (perhaps <code>null</code>) 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 <code>main</code> method can then catch
+these exceptions and decide whether to exit the program and with what exit code.</p>
 
 </recommendation>
 <example>
@@ -38,12 +41,14 @@ upwards and be handled by a method that knows how to recover.</p>
 <p>Problem 2 is more subtle. In this example, there is just one entry point to
 the program (the <code>main</code> method), which constructs an
 <code>Action</code> and performs it. <code>Action.run</code> calls
-<code>System.exit</code> to indicate successful completion. Consider,
-however, how this code might be integrated in an application server that
-constructs <code>Action</code> instances and calls
+<code>System.exit</code> to indicate successful completion. Instead, the 
+<code>run</code> method should return a status code or throw an exception
+on failure, allowing the <code>main</code> method to decide whether to exit
+and with what exit code. Consider how this code might be integrated in an 
+application server that constructs <code>Action</code> instances and calls
 <code>run</code> on them without going through <code>main</code>.
 The fact that <code>run</code> terminates the JVM instead of returning its
-exit code as an integer makes that use-case impossible.</p>
+exit code makes that use-case impossible.</p>
 
 <sample src="CallsToSystemExit.java" />
 

java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql

@@ -4,26 +4,80 @@
  *              reuse and prevent important cleanup steps from running.
  * @kind problem
  * @problem.severity warning
- * @precision low
+ * @precision medium
  * @id java/jvm-exit
- * @tags reliability
- *       maintainability
+ * @previous-id java/jvm-exit-prevents-cleanup-and-reuse
+ * @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 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.getEnclosingCallable*() instanceof LikelyTestMethod
+      or
+      this.getDeclaringType()
+          .getEnclosingType*()
+          .(LocalClassOrInterface)
+          .getLocalTypeDeclStmt()
+          .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."

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. |

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

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: <example_cmd> <example_args> : " + message);
+        Runtime.getRuntime().exit(exitCode); // COMPLIANT
+    }
+}

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
+            }
+        }
+    }
+}

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: <example_cmd> <example_args> : " + message);
+        System.exit(exitCode); // COMPLIANT
+    }
+}