Skip to content

Java: Add java/javautilconcurrentscheduledthreadpoolexecutor query for zero thread pool size #19844

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+77 −0
Open

Java: Add java/javautilconcurrentscheduledthreadpoolexecutor query for zero thread pool size #19844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ ql/java/ql/src/Likely Bugs/Concurrency/CallsToRunnableRun.ql
ql/java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLocking.ql
ql/java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLockingWithInitRace.ql
ql/java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql
ql/java/ql/src/Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.ql
ql/java/ql/src/Likely Bugs/Concurrency/SynchOnBoxedType.ql
ql/java/ql/src/Likely Bugs/Concurrency/SynchSetUnsynchGet.ql
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql
Expand Down
1 change: 1 addition & 0 deletions java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ ql/java/ql/src/Likely Bugs/Concurrency/CallsToRunnableRun.ql
ql/java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLocking.ql
ql/java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLockingWithInitRace.ql
ql/java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql
ql/java/ql/src/Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.ql
ql/java/ql/src/Likely Bugs/Concurrency/SynchOnBoxedType.ql
ql/java/ql/src/Likely Bugs/Concurrency/SynchSetUnsynchGet.ql
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql
Expand Down
24 changes: 24 additions & 0 deletions java/ql/src/Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
## Overview

According the Java documentation on `ScheduledThreadPoolExecutor`, it is not a good idea to set `corePoolSize` to zero, since doing so indicates the executor to keep 0 threads in its pool and the executor will serve no purpose.

## Recommendation

Set the `ScheduledThreadPoolExecutor` to have 1 or more threads in its thread pool and use the class's other methods to create a thread execution schedule.

## Example

```java
public class Test {
void f() {
int i = 0;
ScheduledThreadPoolExecutor s = new ScheduledThreadPoolExecutor(1); // COMPLIANT
ScheduledThreadPoolExecutor s1 = new ScheduledThreadPoolExecutor(0); // NON_COMPLIANT
s.setCorePoolSize(0); // NON_COMPLIANT
s.setCorePoolSize(i); // NON_COMPLIANT
}
}
```

## References
- [ScheduledThreadPoolExecutor](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/util/concurrent/ScheduledThreadPoolExecutor.html)
35 changes: 35 additions & 0 deletions java/ql/src/Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* @id java/javautilconcurrentscheduledthreadpoolexecutor
* @name Zero threads set for `java.util.concurrent.ScheduledThreadPoolExecutor`
* @description Setting `java.util.concurrent.ScheduledThreadPoolExecutor` to have 0 threads serves
* no purpose and may indicate programmer error.
* @kind problem
* @precision very-high
* @problem.severity recommendation
* @tags quality
* reliability
* correctness
* concurrency
*/

import java
import semmle.code.java.dataflow.DataFlow

/**
* A `Call` that has the ability to set or modify the `corePoolSize` of the `java.util.concurrent.ScheduledThreadPoolExecutor` type.
*/
class Sink extends Call {
Sink() {
this.getCallee()
.hasQualifiedName("java.util.concurrent", "ThreadPoolExecutor", "setCorePoolSize") or
this.getCallee()
.hasQualifiedName("java.util.concurrent", "ScheduledThreadPoolExecutor",
"ScheduledThreadPoolExecutor")
}
}

from IntegerLiteral zero, Sink set
where
DataFlow::localFlow(DataFlow::exprNode(zero), DataFlow::exprNode(set.getArgument(0))) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason why only local flow is used (instead of making a data flow configuration and using global flow)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if there's any reason. I simply migrated the query as is from the private repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried the below:

private module FlowConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) { source.asExpr().(IntegerLiteral).getIntValue() = 0 }

  predicate isSink(DataFlow::Node sink) { exists(Sink s | s.getArgument(0) = sink.asExpr()) }
}

private module Flow = DataFlow::Global<FlowConfig>;

from DataFlow::Node sinkNode, Sink s
where Flow::flow(_, sinkNode) and s.getArgument(0) = sinkNode.asExpr()
select s, "ScheduledThreadPoolExecutor.corePoolSize is set to have 0 threads."

It resulted in some new findings, for example this, which is a FP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Where is the source of the FP located?
  • Are you categorising it as an FP due to the threads > 0 check?

zero.getIntValue() = 0
select set, "ScheduledThreadPoolExecutor.corePoolSize is set to have 0 threads."
3 changes: 3 additions & 0 deletions ...ests/ScheduledThreadPoolExecutorZeroThread/ScheduledThreadPoolExecutorZeroThread.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
| Test.java:7:42:7:75 | new ScheduledThreadPoolExecutor(...) | ScheduledThreadPoolExecutor.corePoolSize is set to have 0 threads. |
| Test.java:8:9:8:28 | setCorePoolSize(...) | ScheduledThreadPoolExecutor.corePoolSize is set to have 0 threads. |
| Test.java:9:9:9:28 | setCorePoolSize(...) | ScheduledThreadPoolExecutor.corePoolSize is set to have 0 threads. |
2 changes: 2 additions & 0 deletions ...y-tests/ScheduledThreadPoolExecutorZeroThread/ScheduledThreadPoolExecutorZeroThread.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
query: Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql
11 changes: 11 additions & 0 deletions java/ql/test/query-tests/ScheduledThreadPoolExecutorZeroThread/Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import java.util.concurrent.ScheduledThreadPoolExecutor;

public class Test {
void f() {
int i = 0;
ScheduledThreadPoolExecutor s = new ScheduledThreadPoolExecutor(1); // Compliant
ScheduledThreadPoolExecutor s1 = new ScheduledThreadPoolExecutor(0); // $ Alert
s.setCorePoolSize(0); // $ Alert
s.setCorePoolSize(i); // $ Alert
}
}
Loading