Merge pull request #19075 from jcogs33/jcogs33/java/do-not-use-finalizers

Java: Add new quality query to detect `finalize` calls

Author: jcogs33

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

@@ -11,3 +11,4 @@ ql/java/ql/src/Likely Bugs/Likely Typos/SuspiciousDateFormat.ql
 ql/java/ql/src/Likely Bugs/Resource Leaks/CloseReader.ql
 ql/java/ql/src/Likely Bugs/Resource Leaks/CloseWriter.ql
 ql/java/ql/src/Performance/StringReplaceAllWithNonRegex.ql
+ql/java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql

java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.md

@@ -0,0 +1,59 @@
+## Overview
+
+Triggering garbage collection by directly calling `finalize()` may either have no effect or trigger unnecessary garbage collection, leading to erratic behavior, performance issues, or deadlock.
+
+## Recommendation
+
+Avoid calling `finalize()` in application code. Allow the JVM to determine a garbage collection schedule instead. If you need to explicitly release resources, provide a specific method to do so, such as by implementing the `AutoCloseable` interface and overriding its `close` method. You can then use a `try-with-resources` block to ensure that the resource is closed.
+
+## Example
+
+```java
+class LocalCache {
+    private Collection<File> cacheFiles = ...;
+    // ...
+}
+
+void main() {
+    LocalCache cache = new LocalCache();
+    // ...
+    cache.finalize(); // NON_COMPLIANT
+}
+
+```
+
+```java
+import java.lang.AutoCloseable;
+import java.lang.Override;
+
+class LocalCache implements AutoCloseable {
+    private Collection<File> cacheFiles = ...;
+    // ...
+
+    @Override
+    public void close() throws Exception {
+        // release resources here if required
+    }
+}
+
+void main() {
+    // COMPLIANT: uses try-with-resources to ensure that
+    // a resource implementing AutoCloseable is closed.
+    try (LocalCache cache = new LocalCache()) {
+        // ...
+    }
+}
+
+```
+
+# Implementation Notes
+
+This rule ignores `super.finalize()` calls that occur within `finalize()` overrides since calling the superclass finalizer is required when overriding `finalize()`. Also, although overriding `finalize()` is not recommended, this rule only alerts on direct calls to `finalize()` and does not alert on method declarations overriding `finalize()`.
+
+## References
+
+- SEI CERT Oracle Coding Standard for Java: [MET12-J. Do not use finalizers](https://wiki.sei.cmu.edu/confluence/display/java/MET12-J.+Do+not+use+finalizers).
+- Java API Specification: [Object.finalize()](https://docs.oracle.com/javase/10/docs/api/java/lang/Object.html#finalize()).
+- Java API Specification: [Interface AutoCloseable](https://docs.oracle.com/javase/10/docs/api/java/lang/AutoCloseable.html).
+- Java SE Documentation: [The try-with-resources Statement](https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html).
+- Common Weakness Enumeration: [CWE-586](https://cwe.mitre.org/data/definitions/586).

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

@@ -0,0 +1,30 @@
+/**
+ * @id java/do-not-call-finalize
+ * @previous-id java/do-not-use-finalizers
+ * @name Do not call `finalize()`
+ * @description Calling `finalize()` in application code may cause
+ *              inconsistent program state or unpredictable behavior.
+ * @kind problem
+ * @precision high
+ * @problem.severity error
+ * @tags quality
+ *       reliability
+ *       correctness
+ *       performance
+ *       external/cwe/cwe-586
+ */
+
+import java
+
+from MethodCall mc
+where
+  mc.getMethod() instanceof FinalizeMethod and
+  // The Java documentation for `finalize()` states: "If a subclass overrides
+  // `finalize` it must invoke the superclass finalizer explicitly". Therefore,
+  // we do not alert on `super.finalize()` calls that occur within a callable
+  // that overrides `finalize`.
+  not exists(Callable caller, FinalizeMethod fm | caller = mc.getCaller() |
+    caller.(Method).overrides(fm) and
+    mc.getQualifier() instanceof SuperAccess
+  )
+select mc, "Call to 'finalize()'."

java/ql/src/codeql-suites/java-code-quality.qls

@@ -2,6 +2,7 @@
 - include:
     id:
       - java/contradictory-type-checks
+      - java/do-not-call-finalize
       - java/equals-on-unrelated-types
       - java/inconsistent-equals-and-hashcode
       - java/input-resource-leak

java/ql/test/query-tests/DoNotCallFinalize/DoNotCallFinalize.expected

@@ -0,0 +1 @@
+| Test.java:4:9:4:23 | finalize(...) | Call to 'finalize()'. |

java/ql/test/query-tests/DoNotCallFinalize/DoNotCallFinalize.qlref

@@ -0,0 +1,2 @@
+query: Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

java/ql/test/query-tests/DoNotCallFinalize/Test.java

@@ -0,0 +1,28 @@
+public class Test {
+    void f() throws Throwable {
+        // NON_COMPLIANT
+        this.finalize(); // $ Alert
+    }
+
+    void f1() throws Throwable {
+        f(); // COMPLIANT
+    }
+
+    @Override
+    protected void finalize() throws Throwable {
+        // COMPLIANT: If a subclass overrides `finalize()`
+        // it must invoke the superclass finalizer explicitly.
+        super.finalize();
+    }
+
+    // Overload of `finalize`
+    protected void finalize(String s) throws Throwable {
+        // ...
+    }
+
+    void f2() throws Throwable {
+        // COMPLIANT: call to overload of `finalize`
+        this.finalize("overload");
+    }
+
+}