Fix for false positive MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR when referencing but not calling an overridable method #2900
Conversation
It looks like the problem for spotbugs#1867 was fixed elsewhere
There was a problem hiding this comment.
I agree, it's better to avoid false positives with the potential of having false negatives.
I also prefer your solution here to mine with CT_CONSTRUCTOR_THROW, but it's still a workaround, and several detectors have problems with bootstrap methods like this.
I think it would be worth to work a proper solution sometime (not in this PR) to the lambda handling, not just say that "it's known that the handling of lambdas are not correct."
spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2837Test.java
Outdated
Show resolved
Hide resolved
| function = v -> v + "!"; | ||
|
|
||
| // just to ignore warnings about unused member | ||
| System.out.println(function.apply("hello")); |
There was a problem hiding this comment.
If I understood well, the problem here is that the StringBuilder.append() is reported as overridable, and the error is reported at line 10 without the change.
However, something is a bit off here, since using it inside a stream (Arrays.asList("a", "b").stream().map(v -> v + "!")), doesn't report the error.
With line 13 this one becomes a false negative, since the function is used and called in the constructor - if this line would be moved to another function which is not called inside the constructor, then it would be a true negative.
There was a problem hiding this comment.
My understanding is that this detectors tries to catch a class of vulnerabilities where an attacker extends a class an override a method. For instance you'd override isAdmin(), now if the constructor does this.admin = isAdmin() the object ends up in an invalid state.
In that particular case StringBuilder.append() is not controllable by the attacker, so the bug was a false positive for another reason.
Although this PR seems to fix #1867 (because in the sample the lambda was immediately assigned to a field) the underlying problem with lambdas is still there.
There was a problem hiding this comment.
Thank you for your explanation.
Overridable methods are only reported once, add a 2nd one to make sure the unit test shows the problem when assigning a method reference to a field
… referencing but not calling an overridable method (spotbugs#2900) * test: issue spotbugs#2837 reproducer * fix: do not consider a method reference assigned to a field * test: added false positive check for issue spotbugs#1867 It looks like the problem for spotbugs#1867 was fixed elsewhere * test: added a true positive test for a lambda called in constructor * doc: added changelog entry * test: fixed tests * fix: removed unused variable * test: added a 2nd overridable method for the true positive case Overridable methods are only reported once, add a 2nd one to make sure the unit test shows the problem when assigning a method reference to a field
INVOKEDYNAMIC followed by PUTFIELD means that we assigned the method reference to a field.
I think it's a bit better to avoid false positives here, with the potential for false negatives due to this change