diff --git a/java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected b/java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected index 2cff4a3eaa62..bdd51d7eee69 100644 --- a/java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected +++ b/java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected @@ -10,3 +10,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 diff --git a/java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.md b/java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.md new file mode 100644 index 000000000000..385cbfb5cfe2 --- /dev/null +++ b/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 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 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). diff --git a/java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql b/java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql new file mode 100644 index 000000000000..1abe96f91857 --- /dev/null +++ b/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()'." diff --git a/java/ql/src/codeql-suites/java-code-quality.qls b/java/ql/src/codeql-suites/java-code-quality.qls index 2eafe785532e..af6fc67cdf1b 100644 --- a/java/ql/src/codeql-suites/java-code-quality.qls +++ b/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 @@ -12,4 +13,4 @@ - java/suspicious-date-format - java/type-variable-hides-type - java/unchecked-cast-in-equals - - java/unused-container + - java/unused-container \ No newline at end of file diff --git a/java/ql/test/query-tests/DoNotCallFinalize/DoNotCallFinalize.expected b/java/ql/test/query-tests/DoNotCallFinalize/DoNotCallFinalize.expected new file mode 100644 index 000000000000..ac3c4fa59c01 --- /dev/null +++ b/java/ql/test/query-tests/DoNotCallFinalize/DoNotCallFinalize.expected @@ -0,0 +1 @@ +| Test.java:4:9:4:23 | finalize(...) | Call to 'finalize()'. | diff --git a/java/ql/test/query-tests/DoNotCallFinalize/DoNotCallFinalize.qlref b/java/ql/test/query-tests/DoNotCallFinalize/DoNotCallFinalize.qlref new file mode 100644 index 000000000000..b301797d5295 --- /dev/null +++ b/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 diff --git a/java/ql/test/query-tests/DoNotCallFinalize/Test.java b/java/ql/test/query-tests/DoNotCallFinalize/Test.java new file mode 100644 index 000000000000..b70d0e47581a --- /dev/null +++ b/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"); + } + +}