mirror of
https://github.com/github/codeql.git
synced 2025-12-16 16:53:25 +01:00
java: address review comments
- do not use `getQualifiedName` - use camelCase - rework alert predicates
This commit is contained in:
yoff
@@ -44,11 +44,11 @@ predicate isThreadSafeType(Type t) {
|
||||
|
||||
/** Holds if the expression `e` is a thread-safe initializer. */
|
||||
predicate isThreadSafeInitializer(Expr e) {
|
||||
e.(Call)
|
||||
.getCallee()
|
||||
.getSourceDeclaration()
|
||||
.getQualifiedName()
|
||||
.matches("java.util.Collections.synchronized%")
|
||||
exists(string name |
|
||||
e.(Call).getCallee().getSourceDeclaration().hasQualifiedName("java.util", "Collections", name)
|
||||
|
|
||||
name.matches("synchronized%")
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -132,7 +132,7 @@ class ClassAnnotatedAsThreadSafe extends Class {
|
||||
* Holds if the field access `a` to the field `f` is not protected by any monitor, and it can be reached via the expression `e` in the method `m`.
|
||||
* We maintain the invariant that `m = e.getEnclosingCallable()`.
|
||||
*/
|
||||
predicate unlocked_access(ExposedField f, Expr e, Method m, ExposedFieldAccess a, boolean write) {
|
||||
predicate unlockedAccess(ExposedField f, Expr e, Method m, ExposedFieldAccess a, boolean write) {
|
||||
m.getDeclaringType() = this and
|
||||
(
|
||||
// base case
|
||||
@@ -143,7 +143,7 @@ class ClassAnnotatedAsThreadSafe extends Class {
|
||||
(if Modification::isModifying(a) then write = true else write = false)
|
||||
or
|
||||
// recursive case
|
||||
exists(MethodCall c, Expr e0, Method m0 | this.unlocked_access(f, e0, m0, a, write) |
|
||||
exists(MethodCall c, Expr e0, Method m0 | this.unlockedAccess(f, e0, m0, a, write) |
|
||||
m = c.getEnclosingCallable() and
|
||||
not m0.isPublic() and
|
||||
c.getCallee().getSourceDeclaration() = m0 and
|
||||
@@ -154,22 +154,22 @@ class ClassAnnotatedAsThreadSafe extends Class {
|
||||
}
|
||||
|
||||
/** Holds if the class has an unlocked access to the field `f` via the expression `e` in the method `m`. */
|
||||
predicate has_unlocked_access(ExposedField f, Expr e, Method m, boolean write) {
|
||||
this.unlocked_access(f, e, m, _, write)
|
||||
predicate hasUnlockedAccess(ExposedField f, Expr e, Method m, boolean write) {
|
||||
this.unlockedAccess(f, e, m, _, write)
|
||||
}
|
||||
|
||||
/** Holds if the field access `a` to the field `f` is not protected by any monitor, and it can be reached via the expression `e` in the public method `m`. */
|
||||
predicate unlocked_public_access(
|
||||
predicate unlockedPublicAccess(
|
||||
ExposedField f, Expr e, Method m, ExposedFieldAccess a, boolean write
|
||||
) {
|
||||
this.unlocked_access(f, e, m, a, write) and
|
||||
this.unlockedAccess(f, e, m, a, write) and
|
||||
m.isPublic() and
|
||||
not Monitors::locallyMonitors(e, _)
|
||||
}
|
||||
|
||||
/** Holds if the class has an unlocked access to the field `f` via the expression `e` in the public method `m`. */
|
||||
predicate has_unlocked_public_access(ExposedField f, Expr e, Method m, boolean write) {
|
||||
this.unlocked_public_access(f, e, m, _, write)
|
||||
predicate hasUnlockedPublicAccess(ExposedField f, Expr e, Method m, boolean write) {
|
||||
this.unlockedPublicAccess(f, e, m, _, write)
|
||||
}
|
||||
|
||||
// Cases where all accesses to a field are protected by exactly one monitor
|
||||
@@ -177,15 +177,15 @@ class ClassAnnotatedAsThreadSafe extends Class {
|
||||
/**
|
||||
* Holds if the class has an access, locked by exactly one monitor, to the field `f` via the expression `e` in the method `m`.
|
||||
*/
|
||||
predicate has_onelocked_access(
|
||||
predicate hasOnelockedAccess(
|
||||
ExposedField f, Expr e, Method m, boolean write, Monitors::Monitor monitor
|
||||
) {
|
||||
//base
|
||||
this.has_unlocked_access(f, e, m, write) and
|
||||
this.hasUnlockedAccess(f, e, m, write) and
|
||||
Monitors::locallyMonitors(e, monitor)
|
||||
or
|
||||
// recursive case
|
||||
exists(MethodCall c, Method m0 | this.has_onelocked_access(f, _, m0, write, monitor) |
|
||||
exists(MethodCall c, Method m0 | this.hasOnelockedAccess(f, _, m0, write, monitor) |
|
||||
m = c.getEnclosingCallable() and
|
||||
not m0.isPublic() and
|
||||
c.getCallee().getSourceDeclaration() = m0 and
|
||||
@@ -197,34 +197,33 @@ class ClassAnnotatedAsThreadSafe extends Class {
|
||||
}
|
||||
|
||||
/** Holds if the class has an access, locked by exactly one monitor, to the field `f` via the expression `e` in the public method `m`. */
|
||||
predicate has_onelocked_public_access(
|
||||
predicate hasOnelockedPublicAccess(
|
||||
ExposedField f, Expr e, Method m, boolean write, Monitors::Monitor monitor
|
||||
) {
|
||||
this.has_onelocked_access(f, e, m, write, monitor) and
|
||||
this.hasOnelockedAccess(f, e, m, write, monitor) and
|
||||
m.isPublic() and
|
||||
not this.has_unlocked_public_access(f, e, m, write)
|
||||
not this.hasUnlockedPublicAccess(f, e, m, write)
|
||||
}
|
||||
|
||||
/** Holds if the field `f` has more than one access, all locked by a single monitor, but different monitors are used. */
|
||||
predicate single_monitor_mismatch(ExposedField f) {
|
||||
2 <=
|
||||
strictcount(Monitors::Monitor monitor | this.has_onelocked_public_access(f, _, _, _, monitor))
|
||||
predicate singleMonitorMismatch(ExposedField f) {
|
||||
2 <= strictcount(Monitors::Monitor monitor | this.hasOnelockedPublicAccess(f, _, _, _, monitor))
|
||||
}
|
||||
|
||||
// Cases where all accesses to a field are protected by at least one monitor
|
||||
//
|
||||
/** Holds if the class has an access, locked by at least one monitor, to the field `f` via the expression `e` in the method `m`. */
|
||||
predicate has_onepluslocked_access(
|
||||
predicate hasOnepluslockedAccess(
|
||||
ExposedField f, Expr e, Method m, boolean write, Monitors::Monitor monitor
|
||||
) {
|
||||
//base
|
||||
this.has_onelocked_access(f, e, m, write, monitor) and
|
||||
not this.single_monitor_mismatch(f) and
|
||||
not this.has_unlocked_public_access(f, _, _, _)
|
||||
this.hasOnelockedAccess(f, e, m, write, monitor) and
|
||||
not this.singleMonitorMismatch(f) and
|
||||
not this.hasUnlockedPublicAccess(f, _, _, _)
|
||||
or
|
||||
// recursive case
|
||||
exists(MethodCall c, Method m0, Monitors::Monitor monitor0 |
|
||||
this.has_onepluslocked_access(f, _, m0, write, monitor0) and
|
||||
this.hasOnepluslockedAccess(f, _, m0, write, monitor0) and
|
||||
m = c.getEnclosingCallable() and
|
||||
not m0.isPublic() and
|
||||
c.getCallee().getSourceDeclaration() = m0 and
|
||||
@@ -238,27 +237,27 @@ class ClassAnnotatedAsThreadSafe extends Class {
|
||||
}
|
||||
|
||||
/** Holds if the class has a write access to the field `f` that can be reached via a public method. */
|
||||
predicate has_public_write_access(ExposedField f) {
|
||||
this.has_unlocked_public_access(f, _, _, true)
|
||||
predicate hasPublicWriteAccess(ExposedField f) {
|
||||
this.hasUnlockedPublicAccess(f, _, _, true)
|
||||
or
|
||||
this.has_onelocked_public_access(f, _, _, true, _)
|
||||
this.hasOnelockedPublicAccess(f, _, _, true, _)
|
||||
or
|
||||
exists(Method m | m.getDeclaringType() = this and m.isPublic() |
|
||||
this.has_onepluslocked_access(f, _, m, true, _)
|
||||
this.hasOnepluslockedAccess(f, _, m, true, _)
|
||||
)
|
||||
}
|
||||
|
||||
/** Holds if the class has an access, not protected by the monitor `m`, to the field `f` via the expression `e` in the method `m`. */
|
||||
predicate escapes_monitor(
|
||||
predicate escapesMonitor(
|
||||
ExposedField f, Expr e, Method m, boolean write, Monitors::Monitor monitor
|
||||
) {
|
||||
//base
|
||||
this.has_onepluslocked_access(f, _, _, _, monitor) and
|
||||
this.has_unlocked_access(f, e, m, write) and
|
||||
this.hasOnepluslockedAccess(f, _, _, _, monitor) and
|
||||
this.hasUnlockedAccess(f, e, m, write) and
|
||||
not Monitors::locallyMonitors(e, monitor)
|
||||
or
|
||||
// recursive case
|
||||
exists(MethodCall c, Method m0 | this.escapes_monitor(f, _, m0, write, monitor) |
|
||||
exists(MethodCall c, Method m0 | this.escapesMonitor(f, _, m0, write, monitor) |
|
||||
m = c.getEnclosingCallable() and
|
||||
not m0.isPublic() and
|
||||
c.getCallee().getSourceDeclaration() = m0 and
|
||||
@@ -269,17 +268,17 @@ class ClassAnnotatedAsThreadSafe extends Class {
|
||||
}
|
||||
|
||||
/** Holds if the class has an access, not protected by the monitor `m`, to the field `f` via the expression `e` in the public method `m`. */
|
||||
predicate escapes_monitor_public(
|
||||
predicate escapesMonitorPublic(
|
||||
ExposedField f, Expr e, Method m, boolean write, Monitors::Monitor monitor
|
||||
) {
|
||||
this.escapes_monitor(f, e, m, write, monitor) and
|
||||
this.escapesMonitor(f, e, m, write, monitor) and
|
||||
m.isPublic()
|
||||
}
|
||||
|
||||
/** Holds if no monitor protects all accesses to the field `f`. */
|
||||
predicate not_fully_monitored(ExposedField f) {
|
||||
forex(Monitors::Monitor monitor | this.has_onepluslocked_access(f, _, _, _, monitor) |
|
||||
this.escapes_monitor_public(f, _, _, _, monitor)
|
||||
predicate notFullyMonitored(ExposedField f) {
|
||||
forex(Monitors::Monitor monitor | this.hasOnepluslockedAccess(f, _, _, _, monitor) |
|
||||
this.escapesMonitorPublic(f, _, _, _, monitor)
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,43 +14,38 @@
|
||||
import java
|
||||
import semmle.code.java.ConflictingAccess
|
||||
|
||||
predicate unmonitored_access(
|
||||
ClassAnnotatedAsThreadSafe cls, ExposedFieldAccess a, Expr entry, string msg, string entry_desc
|
||||
) {
|
||||
exists(ExposedField f |
|
||||
cls.unlocked_public_access(f, entry, _, a, true)
|
||||
predicate unmonitoredAccess(ExposedFieldAccess a, string msg, Expr entry, string entry_desc) {
|
||||
exists(ClassAnnotatedAsThreadSafe cls, ExposedField f |
|
||||
cls.unlockedPublicAccess(f, entry, _, a, true)
|
||||
or
|
||||
cls.unlocked_public_access(f, entry, _, a, false) and
|
||||
cls.has_public_write_access(f)
|
||||
cls.unlockedPublicAccess(f, entry, _, a, false) and
|
||||
cls.hasPublicWriteAccess(f)
|
||||
) and
|
||||
msg =
|
||||
"This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe." and
|
||||
entry_desc = "this expression"
|
||||
}
|
||||
|
||||
predicate not_fully_monitored_field(
|
||||
ClassAnnotatedAsThreadSafe cls, ExposedField f, string msg, string cls_name
|
||||
predicate notFullyMonitoredField(
|
||||
ExposedField f, string msg, ClassAnnotatedAsThreadSafe cls, string cls_name
|
||||
) {
|
||||
(
|
||||
// Technically there has to be a write access for a conflict to exist.
|
||||
// But if you are locking your reads with different locks, you likely made a typo,
|
||||
// so in this case we alert without requiring `cls.has_public_write_access(f)`
|
||||
cls.single_monitor_mismatch(f)
|
||||
cls.singleMonitorMismatch(f)
|
||||
or
|
||||
cls.not_fully_monitored(f) and
|
||||
cls.has_public_write_access(f)
|
||||
cls.notFullyMonitored(f) and
|
||||
cls.hasPublicWriteAccess(f)
|
||||
) and
|
||||
msg =
|
||||
"This field is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe." and
|
||||
cls_name = cls.getName()
|
||||
}
|
||||
|
||||
from
|
||||
ClassAnnotatedAsThreadSafe cls, Top alert_element, Top alert_context, string alert_msg,
|
||||
string context_desc
|
||||
from Top alert_element, Top alert_context, string alert_msg, string context_desc
|
||||
where
|
||||
unmonitored_access(cls, alert_element, alert_context, alert_msg, context_desc)
|
||||
unmonitoredAccess(alert_element, alert_msg, alert_context, context_desc)
|
||||
or
|
||||
not_fully_monitored_field(cls, alert_element, alert_msg, context_desc) and
|
||||
alert_context = cls
|
||||
notFullyMonitoredField(alert_element, alert_msg, alert_context, context_desc)
|
||||
select alert_element, alert_msg, alert_context, context_desc
|
||||
|
||||
@@ -10,12 +10,12 @@
|
||||
| examples/C.java:30:13:30:13 | y | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/C.java:30:13:30:13 | y | this expression |
|
||||
| examples/C.java:33:9:33:9 | y | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/C.java:33:9:33:9 | y | this expression |
|
||||
| examples/FaultyTurnstileExample.java:18:5:18:9 | count | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/FaultyTurnstileExample.java:18:5:18:9 | count | this expression |
|
||||
| examples/FaultyTurnstileExample.java:26:15:26:19 | count | The field $@ is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/FaultyTurnstileExample.java:23:7:23:29 | FaultyTurnstileExample2 | FaultyTurnstileExample2 |
|
||||
| examples/FaultyTurnstileExample.java:26:15:26:19 | count | This field is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/FaultyTurnstileExample.java:23:7:23:29 | FaultyTurnstileExample2 | FaultyTurnstileExample2 |
|
||||
| examples/FlawedSemaphore.java:15:14:15:18 | state | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/FlawedSemaphore.java:15:14:15:18 | state | this expression |
|
||||
| examples/FlawedSemaphore.java:18:7:18:11 | state | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/FlawedSemaphore.java:18:7:18:11 | state | this expression |
|
||||
| examples/LockExample.java:18:15:18:20 | length | The field $@ is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/LockExample.java:14:14:14:24 | LockExample | LockExample |
|
||||
| examples/LockExample.java:19:15:19:31 | notRelatedToOther | The field $@ is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/LockExample.java:14:14:14:24 | LockExample | LockExample |
|
||||
| examples/LockExample.java:20:17:20:23 | content | The field $@ is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/LockExample.java:14:14:14:24 | LockExample | LockExample |
|
||||
| examples/LockExample.java:18:15:18:20 | length | This field is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/LockExample.java:14:14:14:24 | LockExample | LockExample |
|
||||
| examples/LockExample.java:19:15:19:31 | notRelatedToOther | This field is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/LockExample.java:14:14:14:24 | LockExample | LockExample |
|
||||
| examples/LockExample.java:20:17:20:23 | content | This field is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/LockExample.java:14:14:14:24 | LockExample | LockExample |
|
||||
| examples/LockExample.java:44:5:44:10 | length | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/LockExample.java:44:5:44:10 | length | this expression |
|
||||
| examples/LockExample.java:45:5:45:11 | content | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/LockExample.java:45:5:45:11 | content | this expression |
|
||||
| examples/LockExample.java:45:13:45:18 | length | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/LockExample.java:45:13:45:18 | length | this expression |
|
||||
@@ -36,7 +36,7 @@
|
||||
| examples/LockExample.java:153:5:153:21 | notRelatedToOther | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/LockExample.java:153:5:153:21 | notRelatedToOther | this expression |
|
||||
| examples/SyncLstExample.java:45:5:45:7 | lst | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/SyncLstExample.java:45:5:45:7 | lst | this expression |
|
||||
| examples/SyncStackExample.java:37:5:37:7 | stc | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/SyncStackExample.java:37:5:37:7 | stc | this expression |
|
||||
| examples/SynchronizedAndLock.java:10:17:10:22 | length | The field $@ is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/SynchronizedAndLock.java:7:14:7:32 | SynchronizedAndLock | SynchronizedAndLock |
|
||||
| examples/SynchronizedAndLock.java:10:17:10:22 | length | This field is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe. | examples/SynchronizedAndLock.java:7:14:7:32 | SynchronizedAndLock | SynchronizedAndLock |
|
||||
| examples/Test.java:52:5:52:10 | this.y | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/Test.java:24:5:24:18 | setYPrivate(...) | this expression |
|
||||
| examples/Test.java:60:5:60:10 | this.y | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/Test.java:60:5:60:10 | this.y | this expression |
|
||||
| examples/Test.java:74:5:74:10 | this.y | This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe. | examples/Test.java:74:5:74:10 | this.y | this expression |
|
||||
|
||||
Reference in New Issue
Block a user