Skip to content

Commit 4461be1

Browse files
authored
Merge pull request #19539 from yoff/java/conflicting-access
2 parents f2380d3 + 406e48b commit 4461be1

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

44 files changed

+1692
-41
lines changed

java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,18 @@ ql/java/ql/src/Likely Bugs/Concurrency/CallsToRunnableRun.ql
6767
ql/java/ql/src/Likely Bugs/Concurrency/DateFormatThreadUnsafe.ql
6868
ql/java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLocking.ql
6969
ql/java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLockingWithInitRace.ql
70+
ql/java/ql/src/Likely Bugs/Concurrency/Escaping.ql
7071
ql/java/ql/src/Likely Bugs/Concurrency/FutileSynchOnField.ql
7172
ql/java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql
7273
ql/java/ql/src/Likely Bugs/Concurrency/NotifyNotNotifyAll.ql
74+
ql/java/ql/src/Likely Bugs/Concurrency/SafePublication.ql
7375
ql/java/ql/src/Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.ql
7476
ql/java/ql/src/Likely Bugs/Concurrency/SleepWithLock.ql
7577
ql/java/ql/src/Likely Bugs/Concurrency/StartInConstructor.ql
7678
ql/java/ql/src/Likely Bugs/Concurrency/SynchOnBoxedType.ql
7779
ql/java/ql/src/Likely Bugs/Concurrency/SynchSetUnsynchGet.ql
7880
ql/java/ql/src/Likely Bugs/Concurrency/SynchWriteObject.ql
81+
ql/java/ql/src/Likely Bugs/Concurrency/ThreadSafe.ql
7982
ql/java/ql/src/Likely Bugs/Finalization/NullifiedSuperFinalize.ql
8083
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/BadSuiteMethod.ql
8184
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql

java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,13 @@ ql/java/ql/src/Likely Bugs/Comparison/WrongNanComparison.ql
3030
ql/java/ql/src/Likely Bugs/Concurrency/CallsToRunnableRun.ql
3131
ql/java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLocking.ql
3232
ql/java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLockingWithInitRace.ql
33+
ql/java/ql/src/Likely Bugs/Concurrency/Escaping.ql
3334
ql/java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql
35+
ql/java/ql/src/Likely Bugs/Concurrency/SafePublication.ql
3436
ql/java/ql/src/Likely Bugs/Concurrency/ScheduledThreadPoolExecutorZeroThread.ql
3537
ql/java/ql/src/Likely Bugs/Concurrency/SynchOnBoxedType.ql
3638
ql/java/ql/src/Likely Bugs/Concurrency/SynchSetUnsynchGet.ql
39+
ql/java/ql/src/Likely Bugs/Concurrency/ThreadSafe.ql
3740
ql/java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql
3841
ql/java/ql/src/Likely Bugs/Inheritance/NoNonFinalInConstructor.ql
3942
ql/java/ql/src/Likely Bugs/Likely Typos/ContainerSizeCmpZero.ql

java/ql/lib/semmle/code/java/Concurrency.qll

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,57 @@ overlay[local?]
22
module;
33

44
import java
5+
import semmle.code.java.frameworks.Mockito
6+
7+
/**
8+
* A Java type representing a lock.
9+
* We identify a lock type as one that has both `lock` and `unlock` methods.
10+
*/
11+
class LockType extends RefType {
12+
LockType() {
13+
this.getAMethod().hasName("lock") and
14+
this.getAMethod().hasName("unlock")
15+
}
16+
17+
/** Gets a method that is locking this lock type. */
18+
private Method getLockMethod() {
19+
result.getDeclaringType() = this and
20+
result.hasName(["lock", "lockInterruptibly", "tryLock"])
21+
}
22+
23+
/** Gets a method that is unlocking this lock type. */
24+
private Method getUnlockMethod() {
25+
result.getDeclaringType() = this and
26+
result.hasName("unlock")
27+
}
28+
29+
/** Gets an `isHeldByCurrentThread` method of this lock type. */
30+
private Method getIsHeldByCurrentThreadMethod() {
31+
result.getDeclaringType() = this and
32+
result.hasName("isHeldByCurrentThread")
33+
}
34+
35+
/** Gets a call to a method that is locking this lock type. */
36+
MethodCall getLockAccess() {
37+
result.getMethod() = this.getLockMethod() and
38+
// Not part of a Mockito verification call
39+
not result instanceof MockitoVerifiedMethodCall
40+
}
41+
42+
/** Gets a call to a method that is unlocking this lock type. */
43+
MethodCall getUnlockAccess() {
44+
result.getMethod() = this.getUnlockMethod() and
45+
// Not part of a Mockito verification call
46+
not result instanceof MockitoVerifiedMethodCall
47+
}
48+
49+
/** Gets a call to a method that checks if the lock is held by the current thread. */
50+
MethodCall getIsHeldByCurrentThreadAccess() {
51+
result.getMethod() = this.getIsHeldByCurrentThreadMethod() and
52+
// Not part of a Mockito verification call
53+
not result instanceof MockitoVerifiedMethodCall
54+
}
55+
}
556

657
/**
758
* Holds if `e` is synchronized by a local synchronized statement `sync` on the variable `v`.
@@ -49,3 +100,136 @@ class SynchronizedCallable extends Callable {
49100
)
50101
}
51102
}
103+
104+
/**
105+
* This module provides predicates, chiefly `locallyMonitors`, to check if a given expression is synchronized on a specific monitor.
106+
*/
107+
module Monitors {
108+
/**
109+
* A monitor is any object that is used to synchronize access to a shared resource.
110+
* This includes locks as well as variables used in synchronized blocks (including `this`).
111+
*/
112+
newtype TMonitor =
113+
/** Either a lock or a variable used in a synchronized block. */
114+
TVariableMonitor(Variable v) {
115+
v.getType() instanceof LockType or locallySynchronizedOn(_, _, v)
116+
} or
117+
/** An instance reference used as a monitor. */
118+
TInstanceMonitor(RefType thisType) { locallySynchronizedOnThis(_, thisType) } or
119+
/** A class used as a monitor. */
120+
TClassMonitor(RefType classType) { locallySynchronizedOnClass(_, classType) }
121+
122+
/**
123+
* A monitor is any object that is used to synchronize access to a shared resource.
124+
* This includes locks as well as variables used in synchronized blocks (including `this`).
125+
*/
126+
class Monitor extends TMonitor {
127+
/** Gets the location of this monitor. */
128+
abstract Location getLocation();
129+
130+
/** Gets a textual representation of this element. */
131+
abstract string toString();
132+
}
133+
134+
/**
135+
* A variable used as a monitor.
136+
* The variable is either a lock or is used in a synchronized block.
137+
* E.g `synchronized (m) { ... }` or `m.lock();`
138+
*/
139+
class VariableMonitor extends Monitor, TVariableMonitor {
140+
override Location getLocation() { result = this.getVariable().getLocation() }
141+
142+
override string toString() { result = "VariableMonitor(" + this.getVariable().toString() + ")" }
143+
144+
/** Gets the variable being used as a monitor. */
145+
Variable getVariable() { this = TVariableMonitor(result) }
146+
}
147+
148+
/**
149+
* An instance reference used as a monitor.
150+
* Either via `synchronized (this) { ... }` or by marking a non-static method as `synchronized`.
151+
*/
152+
class InstanceMonitor extends Monitor, TInstanceMonitor {
153+
override Location getLocation() { result = this.getThisType().getLocation() }
154+
155+
override string toString() { result = "InstanceMonitor(" + this.getThisType().toString() + ")" }
156+
157+
/** Gets the instance reference being used as a monitor. */
158+
RefType getThisType() { this = TInstanceMonitor(result) }
159+
}
160+
161+
/**
162+
* A class used as a monitor.
163+
* This is achieved by marking a static method as `synchronized`.
164+
*/
165+
class ClassMonitor extends Monitor, TClassMonitor {
166+
override Location getLocation() { result = this.getClassType().getLocation() }
167+
168+
override string toString() { result = "ClassMonitor(" + this.getClassType().toString() + ")" }
169+
170+
/** Gets the class being used as a monitor. */
171+
RefType getClassType() { this = TClassMonitor(result) }
172+
}
173+
174+
/** Holds if the expression `e` is synchronized on the monitor `m`. */
175+
predicate locallyMonitors(Expr e, Monitor m) {
176+
exists(Variable v | v = m.(VariableMonitor).getVariable() |
177+
locallyLockedOn(e, v)
178+
or
179+
locallySynchronizedOn(e, _, v)
180+
)
181+
or
182+
locallySynchronizedOnThis(e, m.(InstanceMonitor).getThisType())
183+
or
184+
locallySynchronizedOnClass(e, m.(ClassMonitor).getClassType())
185+
}
186+
187+
/** Gets the control flow node that must dominate `e` when `e` is synchronized on a lock. */
188+
ControlFlowNode getNodeToBeDominated(Expr e) {
189+
// If `e` is the LHS of an assignment, use the control flow node for the assignment
190+
exists(Assignment asgn | asgn.getDest() = e | result = asgn.getControlFlowNode())
191+
or
192+
// if `e` is not the LHS of an assignment, use the default control flow node
193+
not exists(Assignment asgn | asgn.getDest() = e) and
194+
result = e.getControlFlowNode()
195+
}
196+
197+
/** A field storing a lock. */
198+
class LockField extends Field {
199+
LockField() { this.getType() instanceof LockType }
200+
201+
/** Gets a call to a method locking the lock stored in this field. */
202+
MethodCall getLockCall() {
203+
result.getQualifier() = this.getRepresentative().getAnAccess() and
204+
result = this.getType().(LockType).getLockAccess()
205+
}
206+
207+
/** Gets a call to a method unlocking the lock stored in this field. */
208+
MethodCall getUnlockCall() {
209+
result.getQualifier() = this.getRepresentative().getAnAccess() and
210+
result = this.getType().(LockType).getUnlockAccess()
211+
}
212+
213+
/**
214+
* Gets a variable representing this field.
215+
* It can be the field itself or a local variable initialized to the field.
216+
*/
217+
private Variable getRepresentative() {
218+
result = this
219+
or
220+
result.getInitializer() = this.getAnAccess()
221+
}
222+
}
223+
224+
/** Holds if `e` is synchronized on the `Lock` `lock` by a locking call. */
225+
predicate locallyLockedOn(Expr e, LockField lock) {
226+
exists(MethodCall lockCall, MethodCall unlockCall |
227+
lockCall = lock.getLockCall() and
228+
unlockCall = lock.getUnlockCall()
229+
|
230+
dominates(lockCall.getControlFlowNode(), unlockCall.getControlFlowNode()) and
231+
dominates(lockCall.getControlFlowNode(), getNodeToBeDominated(e)) and
232+
postDominates(unlockCall.getControlFlowNode(), getNodeToBeDominated(e))
233+
)
234+
}
235+
}

0 commit comments

Comments
 (0)