Java: Add new quality query to detect finalize calls#19075
Merged
jcogs33 merged 22 commits intogithub:mainfrom Apr 22, 2025
Merged
Java: Add new quality query to detect finalize calls#19075jcogs33 merged 22 commits intogithub:mainfrom
finalize calls#19075jcogs33 merged 22 commits intogithub:mainfrom
Conversation
815802f to
90653ed
Compare
added 9 commits
March 27, 2025 19:35
1674c8d to
ed22a16
Compare
Contributor
Author
|
@knewbury01 for awareness |
owen-mc
reviewed
Apr 1, 2025
java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.md
Outdated
Show resolved
Hide resolved
java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql
Outdated
Show resolved
Hide resolved
Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>
owen-mc
reviewed
Apr 2, 2025
java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.md
Outdated
Show resolved
Hide resolved
owen-mc
previously approved these changes
Apr 3, 2025
Contributor
|
@jcogs33 Thanks for writing up this PR ✨ The docs team will look this over and review it within the next couple of days |
mchammer01
previously approved these changes
Apr 10, 2025
Contributor
mchammer01
left a comment
There was a problem hiding this comment.
@jcogs33 - Approving on behalf of Docs 👍🏻
Just a minor suggestion that you can ignore if you don't agree 😅
java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.md
Outdated
Show resolved
Hide resolved
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
Contributor
Author
|
Thanks @mchammer01! I've applied your suggestion. 🙇 |
tamasvajk
approved these changes
Apr 22, 2025
owen-mc
approved these changes
Apr 22, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a new quality query to detect
finalizecalls. 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:
finalizecalls now. Alerts forrunFinalizersOnExitandgcwill continue to be reported on the pre-existing queriesjava/run-finalizers-on-exitandjava/garbage-collection, respectively. I will make follow-up PRs updating these pre-existing queries if needed.super.finalizecalls that occur within a callable that overridesfinalizesince the Java doc forfinalizestates: "If a subclass overridesfinalizeit must invoke the superclass finalizer explicitly", and alerting on these calls would contradict our pre-existingjava/missing-super-finalizequery. 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.finalize. My understanding is that the JVM will not call overloads of finalize when handling garbage collection, but will only callfinalize()and overrides offinalize(). 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:
finalizecall then attempts to useAutoCloseableas suggested in the QHelp.