Skip to content

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

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

jcogs33
Copy link
Contributor

@jcogs33 jcogs33 commented Mar 20, 2025

Description

Adds a new quality query to detect finalize calls. This query is migrated from the advance security team's quality queries.

Consideration

Changes from original query. Let me know if you disagree with any of these changes:

  • The query only alerts on finalize calls now. Alerts for runFinalizersOnExit and gc will continue to be reported on the pre-existing queries java/run-finalizers-on-exit and java/garbage-collection, respectively. I will make follow-up PRs updating these pre-existing queries if needed.
  • Added an exclusion for super.finalize calls that occur within a callable that overrides finalize since the Java doc for finalize states: "If a subclass overrides finalize it must invoke the superclass finalizer explicitly", and alerting on these calls would contradict our pre-existing java/missing-super-finalize query. This exclusion reduces the number of alerts on the MRVA top-100 from 66 to 24 and on the MRVA top-1000 from 954 to 115.
  • Added an exclusion for calls to overloads of finalize. My understanding is that the JVM will not call overloads of finalize when handling garbage collection, but will only call finalize() and overrides of finalize(). So I don't think overloads are relevant for alerts. Let me know if you disagree. This exclusion further reduces the number of alerts on the MRVA top-100 from 24 to 5 and on the MRVA top-1000 from 115 to 40.

Other Notes:

  • Autofixes look okay. Successfully removes the finalize call then attempts to use AutoCloseable as suggested in the QHelp.
  • DCA looks good

@jcogs33 jcogs33 force-pushed the jcogs33/java/do-not-use-finalizers branch from 815802f to 90653ed Compare March 20, 2025 17:40
@jcogs33 jcogs33 force-pushed the jcogs33/java/do-not-use-finalizers branch from 1674c8d to ed22a16 Compare March 27, 2025 23:36
@jcogs33 jcogs33 marked this pull request as ready for review April 1, 2025 01:14
@jcogs33 jcogs33 requested a review from a team as a code owner April 1, 2025 01:14
@jcogs33 jcogs33 requested review from tamasvajk and owen-mc April 1, 2025 01:14
@jcogs33
Copy link
Contributor Author

jcogs33 commented Apr 1, 2025

@knewbury01 for awareness

Jami Cogswell and others added 3 commits April 1, 2025 15:48
owen-mc
owen-mc previously approved these changes Apr 3, 2025
@jcogs33 jcogs33 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Apr 3, 2025
@sunbrye
Copy link

sunbrye commented Apr 4, 2025

@jcogs33 Thanks for writing up this PR ✨ The docs team will look this over and review it within the next couple of days

@jcogs33 jcogs33 added the no-change-note-required This PR does not need a change note label Apr 4, 2025
@mchammer01 mchammer01 self-requested a review April 10, 2025 06:45
Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcogs33 - Approving on behalf of Docs 👍🏻
Just a minor suggestion that you can ignore if you don't agree 😅

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simplify here

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java no-change-note-required This PR does not need a change note ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants