-
Notifications
You must be signed in to change notification settings - Fork 578
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
Extend detector FindOverridableMethodCall
to detect indirect cases
#1716
Extend detector FindOverridableMethodCall
to detect indirect cases
#1716
Conversation
This new detector detects invocation of overridable method in constructors (`MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR`) and clone() method (`MC_OVERRIDABLE_METHOD_CALL_IN_CLONE`), according to SEI CERT rules [MET05-J. Ensure that constructors do not call overridable methods](https://wiki.sei.cmu.edu/confluence/display/java/MET05-J.+Ensure+that+constructors+do+not+call+overridable+methods) and [MET06-J. Do not invoke overridable methods in clone()](https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88487921)
An overridable method may be called indirectly from a constructor or from a `clone()` method. This patch extends the `FindOverridableMathodCall` to also these cases. (Only thos cases are detected where the non-overridable caller calls the overridable method (directrly or indirectly) on itself.
This is a PR over PR #1711. |
Only a few additional findings compared to the previous PR: Diff on SpotBugs itself, Jenkins, OpenGrok, MATSim, NanoHTTPD, Bt, Ttorrent |
spotbugs/etc/messages.xml
Outdated
<p>Detector for patterns where a constructor or a clone() method calls an overridable | ||
method.</p> | ||
<p> | ||
Calling an overridalbe method from a constructor may result in the use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misspelled "overridable".
spotbugs/etc/messages.xml
Outdated
<Details> | ||
<![CDATA[<p> | ||
Calling an overridable method during in a constructor may result in the use of uninitialized data. It may also | ||
leak the this reference of the partially constructed object. Only static, final or private methods shoud be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misspelled "should".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to add a test case to cover public Object clone()
method, current implementation seems having a problem in the sawOpcode(int seen)
method.
And please merge the latest master
branch to resolve conflict in the CHANGELOG.md
. Thanks in advance!
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOverridableMethodCall.java
Outdated
Show resolved
Hide resolved
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOverridableMethodCall.java
Outdated
Show resolved
Hide resolved
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOverridableMethodCall.java
Show resolved
Hide resolved
checkAndRecordCallFromConstructor(ctor.method, method, ctor.sourceLine); | ||
} | ||
} | ||
if (clone != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two conditions could be combined.
} | ||
if (callers != null) { | ||
for (XMethod caller : callers) { | ||
if (!method.isPrivate() && !method.isFinal()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"if not...else" is a double negative, please refactor.
if (cp.getClassIndex() != getThisClass().getClassNameIndex()) { | ||
return; | ||
} | ||
ConstantNameAndType cnt = (ConstantNameAndType) getConstantPool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this variable name, it's too close to inappropriate.
} | ||
XMethod method = getXClass().findMethod(metOpt.get().getName(), metOpt.get().getSignature(), | ||
metOpt.get().isStatic()); | ||
if (ctor != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two conditions could be combined.
An overridable method may be called indirectly from a constructor or from a
clone()
method. This patch extends theFindOverridableMathodCall
to also these cases. (Only thos cases are detected where the non-overridable caller calls the overridable method (directrly or indirectly) on itself.Make sure these boxes are checked before submitting your PR -- thank you!
CHANGELOG.md
if you have changed SpotBugs code