Java: automodel application mode: generate models for overridden method candidates

Author: Stephan Brandauer

java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll

@@ -37,9 +37,9 @@ newtype TApplicationModeEndpoint =
     )
   } or
   TMethodCall(Call call) { not call instanceof ConstructorCall } or
-  TOverriddenParameter(Parameter p) {
+  TOverriddenParameter(Parameter p, Method overriddenMethod) {
     not p.getCallable().callsConstructor(_) and
-    p.getCallable().(Method).overrides(_)
+    p.getCallable().(Method).overrides(overriddenMethod)
   }
 
 /**
@@ -174,18 +174,16 @@ class MethodCall extends ApplicationModeEndpoint, TMethodCall {
 
 class OverriddenParameter extends ApplicationModeEndpoint, TOverriddenParameter {
   Parameter p;
+  Method overriddenMethod;
 
-  OverriddenParameter() { this = TOverriddenParameter(p) }
+  OverriddenParameter() { this = TOverriddenParameter(p, overriddenMethod) }
 
   override Callable getCallable() {
-    // XXX: we're returning the overriding callable here. This means that the
-    // candidate model will be about the overriding method, not the overridden
-    // method (which is more general).
-    // Doing it this way:
-    //  - (+) makes the decision easier for the user
-    //  - (+) is simplest, implementation-wise
-    //  - (-) produces a less general model
-    result = p.getCallable()
+    // NB: we're returning the overridden callable here. This means that the
+    // candidate model will be about the overridden method, not the overriding
+    // method. This is a more general model, that also applies to other
+    // subclasses of the overridden class.
+    result = overriddenMethod
   }
 
   override Call getCall() { none() }
@@ -269,8 +267,8 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig
   additional predicate sinkSpec(
     Endpoint e, string package, string type, string name, string signature, string ext, string input
   ) {
-    ApplicationModeGetCallable::getCallable(e).hasQualifiedName(package, type, name) and
-    signature = ExternalFlow::paramsString(ApplicationModeGetCallable::getCallable(e)) and
+    e.getCallable().hasQualifiedName(package, type, name) and
+    signature = ExternalFlow::paramsString(e.getCallable()) and
     ext = "" and
     input = e.getMaDInput()
   }
@@ -279,8 +277,8 @@ module ApplicationCandidatesImpl implements SharedCharacteristics::CandidateSig
     Endpoint e, string package, string type, string name, string signature, string ext,
     string output
   ) {
-    ApplicationModeGetCallable::getCallable(e).hasQualifiedName(package, type, name) and
-    signature = ExternalFlow::paramsString(ApplicationModeGetCallable::getCallable(e)) and
+    e.getCallable().hasQualifiedName(package, type, name) and
+    signature = ExternalFlow::paramsString(e.getCallable()) and
     ext = "" and
     output = e.getMaDOutput()
   }

java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractCandidates.expected

@@ -1,3 +1,4 @@
-| Test.java:46:10:48:3 | compareTo(...) | known sanitizer\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:46:10:48:3 | compareTo(...) | CallContext | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray |
-| Test.java:47:4:47:5 | f2 | known non-sink\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:46:10:48:3 | compareTo(...) | CallContext | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
-| Test.java:53:4:53:4 | p | taint step\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:52:3:57:3 | walk(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://walk:1:1:1:1 | walk | name | file://(Path,FileVisitOption[]):1:1:1:1 | (Path,FileVisitOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
+| Test.java:47:10:49:3 | compareTo(...) | known sanitizer\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:47:10:49:3 | compareTo(...) | CallContext | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray |
+| Test.java:48:4:48:5 | f2 | known non-sink\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:47:10:49:3 | compareTo(...) | CallContext | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
+| Test.java:54:4:54:4 | p | taint step\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:53:3:58:3 | walk(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://walk:1:1:1:1 | walk | name | file://(Path,FileVisitOption[]):1:1:1:1 | (Path,FileVisitOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
+| Test.java:66:7:66:18 | this <constr(this)> | exception\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:66:7:66:18 | super(...) | CallContext | file://java.lang:1:1:1:1 | java.lang | package | file://Exception:1:1:1:1 | Exception | type | file://true:1:1:1:1 | true | subtypes | file://Exception:1:1:1:1 | Exception | name | file://():1:1:1:1 | () | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |

java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractNegativeExamples.expected

@@ -1,4 +1,4 @@
-| Test.java:27:4:27:9 | source | path-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:26:3:30:3 | copy(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://copy:1:1:1:1 | copy | name | file://(Path,Path,CopyOption[]):1:1:1:1 | (Path,Path,CopyOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
-| Test.java:28:4:28:9 | target | path-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:26:3:30:3 | copy(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://copy:1:1:1:1 | copy | name | file://(Path,Path,CopyOption[]):1:1:1:1 | (Path,Path,CopyOption[]) | signature | file://Argument[1]:1:1:1:1 | Argument[1] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
-| Test.java:35:4:35:11 | openPath | path-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:34:10:36:3 | newInputStream(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://newInputStream:1:1:1:1 | newInputStream | name | file://(Path,OpenOption[]):1:1:1:1 | (Path,OpenOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
-| Test.java:61:3:61:20 | getInputStream(...) | remote\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:61:3:61:20 | getInputStream(...) | CallContext | file://java.net:1:1:1:1 | java.net | package | file://URLConnection:1:1:1:1 | URLConnection | type | file://true:1:1:1:1 | true | subtypes | file://getInputStream:1:1:1:1 | getInputStream | name | file://():1:1:1:1 | () | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray |
+| Test.java:28:4:28:9 | source | path-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:27:3:31:3 | copy(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://copy:1:1:1:1 | copy | name | file://(Path,Path,CopyOption[]):1:1:1:1 | (Path,Path,CopyOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
+| Test.java:29:4:29:9 | target | path-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:27:3:31:3 | copy(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://copy:1:1:1:1 | copy | name | file://(Path,Path,CopyOption[]):1:1:1:1 | (Path,Path,CopyOption[]) | signature | file://Argument[1]:1:1:1:1 | Argument[1] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
+| Test.java:36:4:36:11 | openPath | path-injection\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:35:10:37:3 | newInputStream(...) | CallContext | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://newInputStream:1:1:1:1 | newInputStream | name | file://(Path,OpenOption[]):1:1:1:1 | (Path,OpenOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray |
+| Test.java:62:3:62:20 | getInputStream(...) | remote\nrelated locations: $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:62:3:62:20 | getInputStream(...) | CallContext | file://java.net:1:1:1:1 | java.net | package | file://URLConnection:1:1:1:1 | URLConnection | type | file://true:1:1:1:1 | true | subtypes | file://getInputStream:1:1:1:1 | getInputStream | name | file://():1:1:1:1 | () | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray |

java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractPositiveExamples.expected

@@ -1,6 +1,6 @@
 import hudson.Plugin;
 
-public class PluginImpl implements Plugin {
+public class PluginImpl extends Plugin {
     @Override
     public void configure(String name, String value) {
         // ...

java/ql/automodel/test/AutomodelApplicationModeExtraction/PluginImpl.java

@@ -1,6 +1,7 @@
 package com.github.codeql.test;
 
 import java.io.InputStream;
+import java.io.PrintWriter;
 import java.nio.file.CopyOption;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -47,7 +48,7 @@ public static int compareFiles(File f1, File f2) {
 			f2 // negative example (modeled as not a sink)
 		);
 	}
-		
+
 	public static void FilesWalkExample(Path p, FileVisitOption o) throws Exception {
 		Files.walk( // the call is a source candidate
 			p, // negative example (modeled as a taint step)
@@ -62,8 +63,8 @@ public static void WebSocketExample(URLConnection c) throws Exception {
 	}
 }
 
-class OverrideTest {
-	public boolean equals(Object o) { // o is a source candidate because it overrides an existing method
-		return false;
+class OverrideTest extends Exception {
+	public void printStackTrace(PrintWriter writer) { // writer is a source candidate because it overrides an existing method
+		return;
 	}
 }

java/ql/automodel/test/AutomodelApplicationModeExtraction/Test.java

@@ -1,5 +1,5 @@
 package hudson;
 
-public interface Plugin {
-    public void configure(String name, String value);
+public class Plugin {
+    public void configure(String name, String value) {}
 }