Introduced detector for rule VNA-00 and VNA-02 with test cases #2077

gonczmisi · 2022-05-29T15:02:51Z

Introducing new detector for SEI CERT rules:

The detectors looks for classes that could be part of multithreaded code. This means that the detector only analyzes classes, if either of the following conditions are true for the context of the class:

the class implements the java.lang.Runnable interface (this includes extending the java.lang.Thread class as well, due to transitive cheking for the Runnable interface)
the class contains any synchronized method or block
the class contains any volatile or atomic type variables (the latter means the java.util.concurrent.atomic package)
there is a java.util.concurrent.locks.Lock interface call anywhere in it (lock(), trylock(), unlock())

If any of the above mentioned conditions are present, then the VNA00 detector looks for primitive variables that match all of the following conditions:

the variable gets compared in a not multithreaded method (contains loop conditions as well)
the variable is witten in any method, other then the one it got compared in
the variable is not declared as volatile, nor as an atomic type (java.util.concurrent.atomic package)
there are no manual lock calls in the method where the access is (java.util.concurrent.locks.Lock interface call: lock(), trylock(), unlock())

If any of the above mentioned conditions are present, then the VNA02 detector looks for primitive variables that match all of the following conditions:

the variable gets red in a not secured method
the variable is witten by a compound operation in a not secured method, other then the one it got compared in
the variable is not declared as an atomic type (java.util.concurrent.atomic package), important that volatile is not a guarantee in this rule
there are no manual lock calls in the method where the access is (java.util.concurrent.locks.Lock interface call: lock(), trylock(), unlock())

Note: a method is considered secured, if the following either of the following conditions are true:

the method is snychronized

the method contains any local variables that has an atomic type

the method contains a synchronized block

the method contains any manual lock calls

Both detectors were tested against the code of the following repositories:

I provieded unit tests for each detectors, that covers all possible test cases (multiple types of multithreaded classes, every kind of compund operations for VNA02 etc.).

Although the numbers of the found bugs were high (specially in the case of Jenkins), all bugs seemed to be a possible vulnerability described by the VNA00 or VNA02 rules.

gonczmisi · 2022-05-30T22:11:29Z

Sorry for the big PR, but these two rules are really in a close connection with each other, so it made more sense for me to develop them together.
Hope it's not gonna be a problem!

gtoison · 2022-06-09T20:12:18Z

Hello, this is not a proper review because all of this a above my pay-grade but here are a couple of comments and questions:

The detector is testing for Runnable, maybe other interfaces such as Callable should be suspects too?
Have you considered using XMethod.usesConcurrency()?
There seem to be typos in CompoundOperationsOnSharedVariables (wrote / red instead of written / read)
The license headers seem to be missing

...ts/src/test/java/edu/umd/cs/findbugs/detect/BadVisibilityOnSharedPrimitiveVariablesTest.java

gonczmisi · 2022-06-13T20:50:52Z

Hello, this is not a proper review because all of this a above my pay-grade but here are a couple of comments and questions:

The detector is testing for Runnable, maybe other interfaces such as Callable should be suspects too?

Have you considered using XMethod.usesConcurrency()?

There seem to be typos in CompoundOperationsOnSharedVariables (wrote / red instead of written / read)

The license headers seem to be missing

Thank you for your remarks @gtoison, I really appreciate them!

I will take a look at Callable, and maybe extend this detector in another PR, but this requires some more investigation on the results
Unfortunately, I check Method against concurrency, and I make use of its MethodGen
Thanks, really bad english by me 😅
I though they are added somehow automatically, I added them to make sure

…ed lincense headers

gonczmisi · 2022-06-21T20:55:56Z

@ThrawnCA could you review this again please?

...ts/src/test/java/edu/umd/cs/findbugs/detect/BadVisibilityOnSharedPrimitiveVariablesTest.java

...ts/src/test/java/edu/umd/cs/findbugs/detect/FindCompoundOperationsOnSharedVariablesTest.java

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

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

spotbugsTestCases/src/java/multithreaded/sharedPrimitiveVariables/SynchronizedBlock.java

...rc/java/multithreaded/sharedPrimitiveVariables/SynchronizedBlockAndBadVisibilityOnField.java

spotbugsTestCases/src/java/multithreaded/sharedPrimitiveVariables/SynchronizedMethod.java

...c/java/multithreaded/sharedPrimitiveVariables/SynchronizedMethodAndBadVisibilityOnField.java

spotbugsTestCases/src/java/multithreaded/sharedPrimitiveVariables/VolatileField.java

gonczmisi · 2022-06-24T13:57:16Z

@ThrawnCA thanks for the review, I pushed the fixes, please check again.

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

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

ThrawnCA · 2022-10-25T00:15:12Z

...ts/src/test/java/edu/umd/cs/findbugs/detect/BadVisibilityOnSharedPrimitiveVariablesTest.java

+
+    @Before
+    public void setup() {
+        SystemProperties.setProperty("ful.debug", "true");


What is "ful"?

I assume it stands for FindUnreleasedLock, it is the system property that defines the DEBUG flag in the class.

Maybe it would be worth to comment it here, so later the codereader will know it.

Also, IMO the tests shouldn't modify the state of the system, so it would be worth to set it back to the original setting after the tests.

What does the FindUnreleasedLock have to do with this test? Why do you need to set this value?

spotbugs/etc/messages.xml

gonczmisi · 2023-04-13T20:57:08Z

Hi @baloghadamsoftware!
I have checked your PR, as I see only the github actions are the difference.
There are multiple unresolved comments on them, so I didn't integrate them into this PR yet.

Let me know if you wish for that commit to be in this PR, and if so, how would you like to proceed.

baloghadamsoftware · 2023-05-26T09:44:19Z

The github actions in my PR are a mistake, I did not write them and did not intend to put them into this PR. I only tried to address the comments that you got on your PR. So please continue your work.

JuditKnoll · 2023-06-22T15:56:03Z

CHANGELOG.md

+### Added
+* New detector `BadVisibilityOnSharedPrimitiveVariables` for new bug type `SPV_BAD_VISIBILITY_ON_SHARED_PRIMITIVE_VARIABLES` (See [SEI CERT rule VNA00-J](https://wiki.sei.cmu.edu/confluence/display/java/VNA00-J.+Ensure+visibility+when+accessing+shared+primitive+variables))
+* New detector `CompoundOperationsOnSharedVariables` for new bug type `COSV_COMPOUND_OPERATIONS_ON_SHARED_VARIABLES` (See [SEI CERT rule VNA02-J](https://wiki.sei.cmu.edu/confluence/display/java/VNA02-J.+Ensure+that+compound+operations+on+shared+variables+are+atomic))
+


There is already an Added section in Unreleased. Can you please merge the two?

JuditKnoll · 2023-06-23T07:51:10Z

...ts/src/test/java/edu/umd/cs/findbugs/detect/BadVisibilityOnSharedPrimitiveVariablesTest.java

+
+    @Before
+    public void setup() {
+        SystemProperties.setProperty("ful.debug", "true");


Maybe it would be worth to comment it here, so later the codereader will know it.

JuditKnoll · 2023-06-23T07:53:39Z

...ts/src/test/java/edu/umd/cs/findbugs/detect/BadVisibilityOnSharedPrimitiveVariablesTest.java

+
+    @Before
+    public void setup() {
+        SystemProperties.setProperty("ful.debug", "true");


Also, IMO the tests shouldn't modify the state of the system, so it would be worth to set it back to the original setting after the tests.

JuditKnoll · 2023-06-23T08:08:24Z

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

+                return;
+            }
+
+            if (seen == Const.IFGE || seen == Const.IFGT || seen == Const.IFLT || seen == Const.IFLE || seen == Const.IFNE || seen == Const.IFEQ) {


Since this and the conditions for the if in line 92 are disjunct, it would be worth to have this in an else block, so this if isn't evaluated unnecessarily.

JuditKnoll · 2023-06-23T08:13:31Z

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

+    }
+
+    private boolean nameIsConstructor(String name) {
+        return name.equals(Const.CONSTRUCTOR_NAME);


It's better to compare to the const value to avoid NPE.

JuditKnoll · 2023-06-23T10:06:24Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

+    public static boolean implementsRunnable(JavaClass javaClass) {
+        try {
+            return Stream.of(javaClass.getAllInterfaces())
+                    .anyMatch(ifc -> ifc.getClassName().equals(RUNNABLE_SIGNATURE));


It's better to compare to the const value to avoid NPE.

JuditKnoll · 2023-06-23T10:12:55Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

+
+        LocalVariableTable lvt = method.getLocalVariableTable();
+        if (lvt != null && Stream.of(lvt.getLocalVariableTable())
+                .anyMatch(lv -> signatureIsFromAtomicPackage(lv.getSignature()))) {


Maybe it would help the readability if you had the condition in a separate variable.

JuditKnoll · 2023-07-26T08:50:05Z

...ts/src/test/java/edu/umd/cs/findbugs/detect/FindCompoundOperationsOnSharedVariablesTest.java

+
+    @Before
+    public void setup() {
+        SystemProperties.setProperty("ful.debug", "true");


IMO the tests shouldn't modify the state of the system, so it would be worth to set it back to the original setting after the tests.

JuditKnoll · 2023-07-27T14:12:01Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

+        return Stream.of(classToCheck.getMethods()).anyMatch(method -> isMethodMultiThreaded(method, classContext));
+    }
+
+    public static boolean implementsRunnable(JavaClass javaClass) {


As far as I can see, this function is only called from inside this class. Wouldn't it be better to restrict the visibility?

JuditKnoll · 2023-07-27T14:14:19Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

+        return hasMultiThreadedInstruction(methodGen);
+    }
+
+    public static boolean hasMultiThreadedInstruction(MethodGen methodGen) {


As far as I can see, this function is only called from inside this class. Wouldn't it be better to restrict the visibility?

JuditKnoll · 2023-07-27T14:15:19Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

+        return false;
+    }
+
+    public static boolean isConcurrentLockInterfaceCall(String methodName) {


As far as I can see, this function is only called from inside this class. Wouldn't it be better to restrict the visibility?

gonczmisi and others added 4 commits May 28, 2022 13:54

Introduced detector for rule VNA-00 and VNA-02 with test cases

3722f90

Merge branch 'master' into vna-00

18c633d

Description for COSV bug and cleanup

58743f9

Fixed packaging error

7be029f

gonczmisi force-pushed the vna-00 branch from 83427a7 to 7be029f Compare May 30, 2022 22:05

ThrawnCA reviewed Jun 10, 2022

View reviewed changes

...ts/src/test/java/edu/umd/cs/findbugs/detect/BadVisibilityOnSharedPrimitiveVariablesTest.java Outdated Show resolved Hide resolved

gonczmisi and others added 2 commits June 13, 2022 23:08

Merge branch 'spotbugs:master' into vna-00

1745125

PR review fixes: pre-test ful debug logging command, fixed typos, add…

c6900e1

…ed lincense headers

gonczmisi requested a review from ThrawnCA June 13, 2022 21:12

Aligned line numbers for tests with linceses

eea20b0

ThrawnCA requested changes Jun 22, 2022

View reviewed changes

gonczmisi and others added 2 commits June 24, 2022 15:53

PR review fixes v2

5c9ce1b

Merge branch 'spotbugs:master' into vna-00

866ba78

gonczmisi requested a review from ThrawnCA June 24, 2022 13:56

ThrawnCA previously approved these changes Jun 27, 2022

View reviewed changes

Merge branch 'master' into vna-00

095e22e

KengoTODA dismissed ThrawnCA’s stale review via 095e22e September 8, 2022 00:51

KengoTODA requested changes Sep 8, 2022

View reviewed changes

ThrawnCA approved these changes Sep 8, 2022

View reviewed changes

baloghadamsoftware mentioned this pull request Oct 11, 2022

SEI_CERT_VNA00_J_VNA02_J #2208

Closed

1 task

Missing diamond operators, fixed misuse of Optional type

a7efab4

gonczmisi requested a review from KengoTODA October 24, 2022 17:59

Merge branch 'master' into vna-00

48b79a5

ThrawnCA requested changes Oct 25, 2022

View reviewed changes

Rephrased bugpattern details

3a60795

Merge branch 'master' into vna-00

13674fc

gonczmisi requested review from ThrawnCA and KengoTODA and removed request for KengoTODA and ThrawnCA February 23, 2023 20:34

gonczmisi requested a review from ThrawnCA April 13, 2023 20:51

Merge branch 'spotbugs:master' into vna-00

11bf124

JuditKnoll requested changes Jun 23, 2023

View reviewed changes

JuditKnoll requested changes Jul 27, 2023

View reviewed changes

github-actions bot added the Stale label Mar 4, 2024

github-actions bot closed this Apr 8, 2024

hazendaz reopened this Apr 12, 2024

github-actions bot removed the Stale label Apr 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced detector for rule VNA-00 and VNA-02 with test cases #2077

Introduced detector for rule VNA-00 and VNA-02 with test cases #2077

gonczmisi commented May 29, 2022 •

edited

gonczmisi commented May 30, 2022

gtoison commented Jun 9, 2022

gonczmisi commented Jun 13, 2022

gonczmisi commented Jun 21, 2022

gonczmisi commented Jun 24, 2022

ThrawnCA Oct 25, 2022

gonczmisi Feb 23, 2023

JuditKnoll Jun 23, 2023

JuditKnoll Jun 23, 2023

JuditKnoll Oct 3, 2023

gonczmisi commented Apr 13, 2023

baloghadamsoftware commented May 26, 2023

JuditKnoll Jun 22, 2023

JuditKnoll Jun 23, 2023

JuditKnoll Jun 23, 2023

JuditKnoll Jun 23, 2023

JuditKnoll Jun 23, 2023

JuditKnoll Jun 23, 2023

JuditKnoll Jun 23, 2023

JuditKnoll Jul 26, 2023

JuditKnoll Jul 27, 2023

JuditKnoll Jul 27, 2023

JuditKnoll Jul 27, 2023

Introduced detector for rule VNA-00 and VNA-02 with test cases #2077

Are you sure you want to change the base?

Introduced detector for rule VNA-00 and VNA-02 with test cases #2077

Conversation

gonczmisi commented May 29, 2022 • edited

gonczmisi commented May 30, 2022

gtoison commented Jun 9, 2022

gonczmisi commented Jun 13, 2022

gonczmisi commented Jun 21, 2022

gonczmisi commented Jun 24, 2022

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

gonczmisi commented Apr 13, 2023

baloghadamsoftware commented May 26, 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

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

gonczmisi commented May 29, 2022 •

edited