diff --git a/java/change-notes/2021-11-15-overrides.md b/java/change-notes/2021-11-15-overrides.md new file mode 100644 index 000000000000..24ecad0c48b6 --- /dev/null +++ b/java/change-notes/2021-11-15-overrides.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The predicate `Method.overrides(Method)` was accidentally transitive. This has been fixed. This fix also affects `Method.overridesOrInstantiates(Method)` and `Method.getASourceOverriddenMethod()`. diff --git a/java/ql/lib/semmle/code/java/Member.qll b/java/ql/lib/semmle/code/java/Member.qll index fee8d3f24ee2..bbafde2c9ba0 100755 --- a/java/ql/lib/semmle/code/java/Member.qll +++ b/java/ql/lib/semmle/code/java/Member.qll @@ -285,7 +285,20 @@ private predicate overrides(Method m1, Method m2) { or m2.isProtected() or - m2.isPackageProtected() and t1.getPackage() = t2.getPackage() + m2.isPackageProtected() and + pragma[only_bind_out](t1.getPackage()) = pragma[only_bind_out](t2.getPackage()) + ) +} + +pragma[nomagic] +private predicate overridesCandidateType(RefType tsup, string sig, RefType t, Method m) { + virtualMethodWithSignature(sig, t, m) and + t.extendsOrImplements(tsup) + or + exists(RefType mid | + overridesCandidateType(mid, sig, t, m) and + mid.extendsOrImplements(tsup) and + not virtualMethodWithSignature(sig, mid, _) ) } @@ -294,11 +307,10 @@ private predicate overrides(Method m1, Method m2) { * ignoring any access modifiers. Additionally, this predicate binds * `t1` to the type declaring `m1` and `t2` to the type declaring `m2`. */ -pragma[noopt] +cached predicate overridesIgnoringAccess(Method m1, RefType t1, Method m2, RefType t2) { exists(string sig | - virtualMethodWithSignature(sig, t1, m1) and - t1.extendsOrImplements+(t2) and + overridesCandidateType(t2, sig, t1, m1) and virtualMethodWithSignature(sig, t2, m2) ) } diff --git a/java/ql/lib/semmle/code/java/deadcode/EntryPoints.qll b/java/ql/lib/semmle/code/java/deadcode/EntryPoints.qll index 0f4d493b06ec..9945414745e8 100644 --- a/java/ql/lib/semmle/code/java/deadcode/EntryPoints.qll +++ b/java/ql/lib/semmle/code/java/deadcode/EntryPoints.qll @@ -262,7 +262,7 @@ class ManagedBeanImplEntryPoint extends EntryPoint, RegisteredManagedBeanImpl { // Find the method that will be called for each method on each managed bean that this class // implements. this.inherits(result) and - result.overrides(this.getAnImplementedManagedBean().getAMethod()) + result.overrides+(this.getAnImplementedManagedBean().getAMethod()) } } diff --git a/java/ql/lib/semmle/code/java/deadcode/StrutsEntryPoints.qll b/java/ql/lib/semmle/code/java/deadcode/StrutsEntryPoints.qll index 953fe6f93ef2..28306895e07d 100644 --- a/java/ql/lib/semmle/code/java/deadcode/StrutsEntryPoints.qll +++ b/java/ql/lib/semmle/code/java/deadcode/StrutsEntryPoints.qll @@ -19,7 +19,7 @@ class Struts1ActionEntryPoint extends EntryPoint, Class { exists(Method methodFromAction | methodFromAction.getDeclaringType().hasQualifiedName("org.apache.struts.action", "Action") | - result.(Method).overrides(methodFromAction) + result.(Method).overrides+(methodFromAction) ) or this.getASupertype*().hasQualifiedName("org.apache.struts.actions", "DispatchAction") and diff --git a/java/ql/lib/semmle/code/java/frameworks/Thrift.qll b/java/ql/lib/semmle/code/java/frameworks/Thrift.qll index 9b2e2fcd5a25..bf826cfe47ea 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Thrift.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Thrift.qll @@ -27,7 +27,7 @@ class ThriftIface extends Interface { Method getAnImplementingMethod() { result.getDeclaringType().(Class).getASupertype+() = this and - result.overrides(this.getAMethod()) and + result.overrides+(this.getAMethod()) and not result.getFile() = this.getFile() } } diff --git a/java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJB.qll b/java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJB.qll index 8d200a98c54e..ccdea7a25b29 100644 --- a/java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJB.qll +++ b/java/ql/lib/semmle/code/java/frameworks/javaee/ejb/EJB.qll @@ -572,7 +572,7 @@ class RemoteInterface extends Interface { * abstract methods or overriding within an interface hierarchy. */ Method getARemoteMethodImplementationChecked() { - result.overrides(this.getARemoteMethod()) and + result.overrides+(this.getARemoteMethod()) and exists(result.getBody()) } diff --git a/java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql b/java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql index b6518ba9bdbd..966762326b6d 100644 --- a/java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql +++ b/java/ql/src/Likely Bugs/Concurrency/NonSynchronizedOverride.ql @@ -54,7 +54,6 @@ where sup.isSynchronized() and not sub.isSynchronized() and not delegatingOverride(sub, sup) and - not exists(Method mid | sub.overrides(mid) and mid.overrides(sup)) and supSrc = sup.getDeclaringType().getSourceDeclaration() select sub, "Method '" + sub.getName() + "' overrides a synchronized method in $@ but is not synchronized.", diff --git a/java/ql/src/Likely Bugs/Inheritance/NoNonFinalInConstructor.ql b/java/ql/src/Likely Bugs/Inheritance/NoNonFinalInConstructor.ql index bc37d39c1b28..990bebe3f573 100644 --- a/java/ql/src/Likely Bugs/Inheritance/NoNonFinalInConstructor.ql +++ b/java/ql/src/Likely Bugs/Inheritance/NoNonFinalInConstructor.ql @@ -45,7 +45,7 @@ where // which is an access to the object being initialized, ... ma = unqualifiedCallToNonAbstractMethod(c, m) and // ... there exists an overriding method in a subtype, - n.overrides(m) and + n.overrides+(m) and n.getDeclaringType().getASupertype+() = c.getDeclaringType() and // ... the method is in a supertype of c, m.getDeclaringType() = c.getDeclaringType().getASupertype*() and diff --git a/java/ql/test/library-tests/overrides/ConstructedOverrides2.expected b/java/ql/test/library-tests/overrides/ConstructedOverrides2.expected index da4d640bf469..af58fbd82e03 100644 --- a/java/ql/test/library-tests/overrides/ConstructedOverrides2.expected +++ b/java/ql/test/library-tests/overrides/ConstructedOverrides2.expected @@ -3,4 +3,3 @@ | ConstructedOverrides.java:17:7:17:9 | Sub | usedGeneric(U, String) | Super.class:0:0:0:0 | Super | usedGeneric(U, String) | | ConstructedOverrides.java:23:7:23:10 | Sub2 | unusedGeneric(V, String) | Super.class:0:0:0:0 | Super | unusedGeneric(U, String) | | ConstructedOverrides.java:23:7:23:10 | Sub2 | usedGeneric(V, String) | ConstructedOverrides.java:17:7:17:9 | Sub | usedGeneric(U, String) | -| ConstructedOverrides.java:23:7:23:10 | Sub2 | usedGeneric(V, String) | Super.class:0:0:0:0 | Super | usedGeneric(U, String) |