From de05bfbce3c703a6e5ef1e391819ee1c3125556f Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 21 Oct 2025 12:49:07 +0200 Subject: [PATCH] java: address review comments - do not use `getQualifiedName` - use camelCase - rework alert predicates --- .../semmle/code/java/ConflictingAccess.qll | 79 +++++++++---------- .../src/Likely Bugs/Concurrency/ThreadSafe.ql | 31 +++----- .../ThreadSafe/ThreadSafe.expected | 10 +-- 3 files changed, 57 insertions(+), 63 deletions(-) diff --git a/java/ql/lib/semmle/code/java/ConflictingAccess.qll b/java/ql/lib/semmle/code/java/ConflictingAccess.qll index eb4f7c0f0b7..39f8a5e1e9e 100644 --- a/java/ql/lib/semmle/code/java/ConflictingAccess.qll +++ b/java/ql/lib/semmle/code/java/ConflictingAccess.qll @@ -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) ) } } diff --git a/java/ql/src/Likely Bugs/Concurrency/ThreadSafe.ql b/java/ql/src/Likely Bugs/Concurrency/ThreadSafe.ql index bdb081ec4a6..fafa4701ed2 100644 --- a/java/ql/src/Likely Bugs/Concurrency/ThreadSafe.ql +++ b/java/ql/src/Likely Bugs/Concurrency/ThreadSafe.ql @@ -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 diff --git a/java/ql/test/query-tests/ThreadSafe/ThreadSafe.expected b/java/ql/test/query-tests/ThreadSafe/ThreadSafe.expected index ae2bd4ea5c8..d6005400129 100644 --- a/java/ql/test/query-tests/ThreadSafe/ThreadSafe.expected +++ b/java/ql/test/query-tests/ThreadSafe/ThreadSafe.expected @@ -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 |