Skip to content
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

Java: Add new quality query to detect finalize calls #19075

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
## Overview

Triggering garbage collection by directly calling `finalize()` may either have no effect or may 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).
Original file line number Diff line number Diff line change
@@ -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()'."
3 changes: 2 additions & 1 deletion java/ql/src/codeql-suites/java-code-quality.qls
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@
- java/unused-container
- java/input-resource-leak
- java/output-resource-leak
- java/type-variable-hides-type
- java/type-variable-hides-type
- java/do-not-call-finalize
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
| Test.java:4:9:4:23 | finalize(...) | Call to 'finalize()'. |
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
28 changes: 28 additions & 0 deletions java/ql/test/query-tests/DoNotCallFinalize/Test.java
Original file line number Diff line number Diff line change
@@ -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");
}

}
Loading