Created a new detector for rule MET04-J #1994

sajtizsolt · 2022-03-22T21:35:27Z

I've created a new detector for SEI CERT rule MET04-J.

This detector should prevent the happening of increasing accessibility of methods from the super class. This can occur in three ways:

increasing the accessibility of a package private method to protected
increasing the accessibility of a package private method to public
increasing the accessibility of a protected method to public

A noteable exception is the Cloneable interface: when a class implements the clone method, it should increase it's accessiblity from protected to public.

My changes:

Updated CHANGELOG.md
Created a new detector called FindIncreasedAccessibilityOfMethods in spotbugs module
Created a test for the new detector called FindIncreasedAccessibilityOfMethodsCheckTest in spotbugs-tests module
Created test cases under spotbugs/spotbugsTestCases/src/java/increasedAccessibilityOfMethods
Added the required descriptions to findbugs.xml and messages.xml

My tests:

I've ran my detector on the codebase of Jenkins to make sure it works properly.
I've ran the ./gradlew spotlessApply build smoketest command locally

sajtizsolt · 2022-04-01T05:24:25Z

Thanks for the review @ThrawnCA ! Could you please give me a workflow approval as well? 🙏

KengoTODA

Added several comments, please check them.

Also, could you consider adding one more test, to verify that we can detect cases that publish methods provided not by direct superclass but by other ancestors, such as a parent of the superclass, or the default method defined in an interface?

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

sajtizsolt · 2022-04-07T13:19:14Z

Hey @KengoTODA ! Thanks for you review, I've made the requested changes and also added some test cases (I've made small adjustments on the detector as well).

ThrawnCA · 2022-04-07T22:11:34Z

Are malicious subclasses really a serious concern? There are reasons that the Security Manager is going away...

KengoTODA

LGTM, thanks for your contribution!

sajtizsolt · 2022-04-12T11:53:19Z

I had to resolve conflicts to be able to merge. Please re-approve my PR and the workflow, thanks!

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

sajtizsolt · 2022-04-20T15:43:58Z

@ThrawnCA @KengoTODA Please check and approve my PR and workflow once again (and hopefully for the last time 😄). Thank you in advance!

sajtizsolt · 2022-09-11T20:01:18Z

@ThrawnCA @KengoTODA Hi! I don't want to urge you, I'm just interested - is there any chance that my PR will be included in a future release?

ThrawnCA

This seems to be reverting a bunch of typo fixes.

spotbugs/etc/messages.xml

sajtizsolt · 2022-11-16T18:50:55Z

@ThrawnCA
Hey! I resolved the conflicts with the master branch and reviewed my changes. If I'm right, I resolved all previous merge issues and unnecessary changes, so I would be grateful if you would check my PR once more and approve it if you think it's right. Should I resolve the previous conversations or it is the task of the starter? Also, could you please give me a workflow approval as well?

sajtizsolt · 2023-02-10T18:37:26Z

@KengoTODA @ThrawnCA I've rebased to the latest master, squashed my previous commits into one, amended my commit message to one which follows the conventional commit rules. Please check this PR one last time, give me a workflow approval and merge if you think that my changes are right too. Thanks in advance and thank you for your work!

spotbugs/etc/messages.xml

CHANGELOG.md

spotbugs/etc/messages.xml

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

...s/src/test/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethodsCheckTest.java

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

...s/src/test/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethodsCheckTest.java

spotbugs/etc/messages.xml

JuditKnoll · 2023-07-21T10:03:11Z

I tested your detector on some bigger projects:

I think I found an FP type in MatSim-Libs (e.g. this one): IMO in case of constructors this bug should not be reported.

sajtizsolt · 2023-07-21T12:25:46Z

Good point, thanks for trying it! As the SEI CERT rule definition doesn't mention anything specific about constructors, I think you are right, I removed constructors from the analysis - also added some tests to validate if there are no false positive cases of them. If it is okay now, could you please give me a workflow approval?

resolved.

JuditKnoll

Rerun the detector on bigger projects, the links are the same as before, overwrote the earlier results.

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

JuditKnoll · 2023-07-24T11:22:20Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

+    }
+
+    @Override
+    public void visitClassContext(ClassContext classContext) {


In the current implementation there is a hit type, which may cause problems for the users, so we may need filter it out: we have three classes: SuperClass, Class, and SubClass, SuperClass is parent of Class, which is parent of SubClass. We have a method, which is declared in SuperClass as protected, overriden in both Class and SubClass as public. There will be a hit in Class, which is great, but there will be also a hit in SubClass, about which I'm not so sure. If the user have no write access to SuperClass and Class, then can't really do anything about the problem.

I'm not sure, if this can occur in real life. Do SpotBugs analyze classes to which the user has no write access? For example in dependencies? I see your point, can you maybe show me an example of it where I can reproduce the hit?

If the user provided the necessary compiled files for the analysis, even if doesn't have write access to them, cause e.g. they are inside a jar file. Checked every hit of the tests on the bigger projects, but didn't find any test like the one, I mentioned. (Otherwise I would have referred it.) You can create a test by simply creating a class, which descends from one of the hits and overrides the appropriate function, or reusing your already existing test classes, using SuperClassOfSuperClass as SuperClass in my example, SuperClass as Class, and SubClassFromSamePackage as SubClass. If you simply add an override of superPackagePrivateMethodToPublic() to SuperClass, then you have the test:

@Override public String superPackagePrivateMethodToPublic() { return "SuperClass.superPackagePrivateMethodToPublic"; }

IMO in this case, the error should be only reported in SuperClass, and not in SubClassFromSamePackage.

@sajtizsolt Can you please create a testcase like this and modify the code to not to bury our users in hits they can't do anything about?

I will, I'm just a littly busy nowadays. I will tag you if it happens!

@JuditKnoll Now I thought about this problem again. To achieve the desired behaviour, we should be able to filter out potential bugs based on the bugs already found while we analyzed the super classes of the current class. I don't know, if there is some logic implemented in SpotBugs which enables us to query this list of bugs based on a class, but if there is not, then we should analyze every single super class again. I don't know, I feel that this would increase the complexity of the detector by a huge scale. Do you feel, that the added value of this solution complies with the amount of extra computational demand of it?

Anyway, I've added the requested changes in another commit - take a look at them. Not terribly slow if there are few super classes.

Yes, I think having a more precise detector is worth the increased complexity. Usually there is less than 10 super classes, so it's not as much of an overhead.

However, IMO there is a solution which only needs to check every super class only once, if we take advantage of the order of visiting, and maintain a collection of the correct methods of the examined class. If we phrase the problem like this, that may help: an IAOM bug should be reported, when there is an increase of accessibility of a method from the closest parent declaring/overriding the given method.
When the bug is reported, there is an accessibility increase, then we can remove the current buggy method from the methods of the examined class (subClass) collection. This way, we don't need to check at every parent whether the same bug got reported or not, also, the number of methods to check against decreases.
Removing a method won't cause a problem, since a method can only have one IAOM bug in one class, and the closer parent will be visited first, since JavaClass.getSuperClasses() contains the list of super classes in ascending order.

Answering your question about filtering out bugs based on other bugs, there is a solution using BugAccumulator, but it's a bit different to what you need. As far as I know, that one only works inside one class, not across classes.

sajtizsolt · 2023-07-30T09:37:27Z

@JuditKnoll Can I run CodeChecker with the demo account? I really can't find any way to do it.

JuditKnoll · 2023-07-31T09:44:59Z

@JuditKnoll Can I run CodeChecker with the demo account? I really can't find any way to do it.

The demo account doesn't have rights to store the results. If you would like to run it yourself, you need to set up a separate codechecker server and configure the runs, which is not so straightforward.
I rerun the tests, the links are the same as earlier.

sajtizsolt · 2023-07-31T10:58:48Z

@JuditKnoll Can I run CodeChecker with the demo account? I really can't find any way to do it.

The demo account doesn't have rights to store the results. If you would like to run it yourself, you need to set up a separate codechecker server and configure the runs, which is not so straightforward. I rerun the tests, the links are the same as earlier.

Thank you! I examined the results and they seem to be valid positive cases to me. What do you think? Can this detector be considered as complete or are there any more edge cases which comes into mind?

JuditKnoll · 2023-07-31T15:09:59Z

@JuditKnoll Can I run CodeChecker with the demo account? I really can't find any way to do it.

The demo account doesn't have rights to store the results. If you would like to run it yourself, you need to set up a separate codechecker server and configure the runs, which is not so straightforward. I rerun the tests, the links are the same as earlier.

Thank you! I examined the results and they seem to be valid positive cases to me. What do you think? Can this detector be considered as complete or are there any more edge cases which comes into mind?

Only the one I mentioned at the detector's visitClassContext() function.

JuditKnoll · 2023-09-04T11:57:02Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

+    }
+
+    @Override
+    public void visitClassContext(ClassContext classContext) {


Yes, I think having a more precise detector is worth the increased complexity. Usually there is less than 10 super classes, so it's not as much of an overhead.

JuditKnoll · 2023-09-04T12:09:41Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

+    }
+
+    @Override
+    public void visitClassContext(ClassContext classContext) {


However, IMO there is a solution which only needs to check every super class only once, if we take advantage of the order of visiting, and maintain a collection of the correct methods of the examined class. If we phrase the problem like this, that may help: an IAOM bug should be reported, when there is an increase of accessibility of a method from the closest parent declaring/overriding the given method.
When the bug is reported, there is an accessibility increase, then we can remove the current buggy method from the methods of the examined class (subClass) collection. This way, we don't need to check at every parent whether the same bug got reported or not, also, the number of methods to check against decreases.
Removing a method won't cause a problem, since a method can only have one IAOM bug in one class, and the closer parent will be visited first, since JavaClass.getSuperClasses() contains the list of super classes in ascending order.

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

JuditKnoll · 2023-09-04T12:32:00Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

+        }
+
+        boolean isSameMethodAndAccessibility(IAOMBug other) {
+            return Objects.equals(method.getSignature(), other.method.getSignature())


Are you sure, that checking the signatures is enough to check two methods are the same?

The Method class itself has an equals, which checks both the name of the method and the signature. Is there any reason, why you don't use that equals?

...-tests/src/test/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethodsTest.java

JuditKnoll · 2023-09-04T13:51:46Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

+
+        List<IAOMBug> superClassBugs = new ArrayList<>();
+        for (JavaClass superClass : superClasses) {
+            superClassBugs.addAll(findIAOMBugs(superClass));


It does work, but it could be a bit more efficient, see my comment at the visitClassContext() method.

JuditKnoll · 2023-09-04T13:56:12Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

+    }
+
+    @Override
+    public void visitClassContext(ClassContext classContext) {


Answering your question about filtering out bugs based on other bugs, there is a solution using BugAccumulator, but it's a bit different to what you need. As far as I know, that one only works inside one class, not across classes.

JuditKnoll · 2023-09-06T12:33:31Z

Seems like the build is failing, can you please run ./gradlew :spotbugs:spotlessApply command to solve the spotless problems?

JuditKnoll

Can you please rebase to the current master and resolve the merge conflicts?
Since your last rebase the project got migrated from Junit to Junit Jupiter (see #2605), which will cause build fail after the rebase, but I commented at the relevant lines, which needs to be modified to work properly.

...-tests/src/test/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethodsTest.java

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

MET04-J

JuditKnoll · 2024-04-17T14:36:59Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

+    private List<Method> getMethodsExceptConstructors(JavaClass javaClass) {
+        ArrayList<Method> methods = new ArrayList<>();
+        for (Method method : javaClass.getMethods()) {
+            if (!Const.CONSTRUCTOR_NAME.equals(method.getName())) {


I think you can also filter out the static initializer.

JuditKnoll · 2024-04-17T14:41:38Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

+        }
+
+        boolean isSameMethodAndAccessibility(IAOMBug other) {
+            return Objects.equals(method.getSignature(), other.method.getSignature())


The Method class itself has an equals, which checks both the name of the method and the signature. Is there any reason, why you don't use that equals?

ThrawnCA previously approved these changes Mar 31, 2022

View reviewed changes

KengoTODA requested changes Apr 7, 2022

View reviewed changes

sajtizsolt dismissed ThrawnCA’s stale review via 3de59b1 April 7, 2022 13:10

KengoTODA previously approved these changes Apr 8, 2022

View reviewed changes

sajtizsolt dismissed KengoTODA’s stale review via 4d1c607 April 12, 2022 11:03

ThrawnCA reviewed Apr 12, 2022

View reviewed changes

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java Outdated Show resolved Hide resolved

ThrawnCA approved these changes Apr 21, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

ThrawnCA requested changes Nov 3, 2022

View reviewed changes

spotbugs/etc/messages.xml Outdated Show resolved Hide resolved

spotbugs/etc/messages.xml Outdated Show resolved Hide resolved

spotbugs/etc/messages.xml Outdated Show resolved Hide resolved

spotbugs/etc/messages.xml Outdated Show resolved Hide resolved

ThrawnCA reviewed Nov 7, 2022

View reviewed changes

spotbugs/etc/messages.xml Outdated Show resolved Hide resolved

ThrawnCA approved these changes Nov 7, 2022

View reviewed changes

ThrawnCA approved these changes Feb 7, 2023

View reviewed changes

sajtizsolt force-pushed the MET04-J branch from b27ce56 to cc85b97 Compare February 10, 2023 18:33

sajtizsolt requested a review from KengoTODA February 10, 2023 18:37

ThrawnCA reviewed Feb 12, 2023

View reviewed changes

spotbugs/etc/messages.xml Show resolved Hide resolved

JuditKnoll reviewed Jun 22, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

spotbugs/etc/messages.xml Outdated Show resolved Hide resolved

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java Outdated Show resolved Hide resolved

sajtizsolt force-pushed the MET04-J branch 4 times, most recently from 7337de4 to c9827e5 Compare July 8, 2023 08:22

sajtizsolt requested a review from JuditKnoll July 8, 2023 08:24

JuditKnoll requested changes Jul 10, 2023

View reviewed changes

JuditKnoll requested changes Jul 12, 2023

View reviewed changes

spotbugs/etc/messages.xml Show resolved Hide resolved

sajtizsolt force-pushed the MET04-J branch 2 times, most recently from 841b50e to 109f07b Compare July 20, 2023 18:29

sajtizsolt requested a review from JuditKnoll July 21, 2023 06:30

sajtizsolt force-pushed the MET04-J branch from 109f07b to 85962c8 Compare July 21, 2023 12:23

sajtizsolt force-pushed the MET04-J branch from 85962c8 to 8cc9cf1 Compare July 22, 2023 16:20

hazendaz self-assigned this Jul 22, 2023

JuditKnoll requested changes Jul 24, 2023

View reviewed changes

sajtizsolt force-pushed the MET04-J branch 2 times, most recently from f7e75ce to b50003f Compare July 30, 2023 09:05

sajtizsolt requested a review from JuditKnoll August 3, 2023 07:36

sajtizsolt force-pushed the MET04-J branch 2 times, most recently from 726ddee to c50b170 Compare September 3, 2023 18:39

JuditKnoll requested changes Sep 4, 2023

View reviewed changes

sajtizsolt force-pushed the MET04-J branch from 3eb4357 to c5dbfa8 Compare September 5, 2023 19:38

JuditKnoll requested changes Jan 12, 2024

View reviewed changes

feat: create new detector for rule MET04-J

a483929

MET04-J

sajtizsolt force-pushed the MET04-J branch 2 times, most recently from 7b506b7 to eacaa5c Compare April 13, 2024 09:32

fix(IAOM): filter bugs which appear in super classes

912d9fe

MET04-J

sajtizsolt force-pushed the MET04-J branch from eacaa5c to 912d9fe Compare April 13, 2024 09:35

JuditKnoll requested changes Apr 17, 2024

View reviewed changes

Created a new detector for rule MET04-J #1994

Are you sure you want to change the base?

Created a new detector for rule MET04-J #1994

Conversation

sajtizsolt commented Mar 22, 2022 • edited

sajtizsolt commented Apr 1, 2022

KengoTODA left a comment

Choose a reason for hiding this comment

sajtizsolt commented Apr 7, 2022

ThrawnCA commented Apr 7, 2022

KengoTODA left a comment

Choose a reason for hiding this comment

sajtizsolt commented Apr 12, 2022

sajtizsolt commented Apr 20, 2022

sajtizsolt commented Sep 11, 2022

This comment was marked as resolved.

ThrawnCA left a comment

Choose a reason for hiding this comment

sajtizsolt commented Nov 16, 2022

sajtizsolt commented Feb 10, 2023

JuditKnoll commented Jul 21, 2023

sajtizsolt commented Jul 21, 2023

JuditKnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sajtizsolt commented Jul 30, 2023

JuditKnoll commented Jul 31, 2023

sajtizsolt commented Jul 31, 2023

JuditKnoll commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JuditKnoll commented Sep 6, 2023

JuditKnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sajtizsolt commented Mar 22, 2022 •

edited