C++: Change 'annotate_path_to_sink' so that you now annotate a ir-path with the previous node (instead of its source). This gives a better overview of the path.

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -484,8 +484,9 @@ module TaintedWithPath {
     /** Gets the element that `pathNode` wraps, if any. */
     Element getElementFromPathNode(PathNode pathNode) {
       exists(DataFlow::Node node | node = pathNode.(WrapPathNode).inner().getNode() |
-        result = node.asExpr() or
-        result = node.asParameter()
+        result = node.asInstruction().getAST()
+        or
+        result = node.asOperand().getDef().getAST()
       )
       or
       result = pathNode.(EndpointPathNode).inner()

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_path_to_sink/dispatch.cpp

@@ -4,8 +4,8 @@ using SinkFunction = void (*)(int);
 
 void notSink(int notSinkParam);
 
-void callsSink(int sinkParam) { // $ ir-path=31:28 ir-path=32:31 ir-path=34:22
-  sink(sinkParam); // $ ir-sink=31:28 ir-sink=32:31 ir-sink=34:22 ast=31:28 ast=32:31 ast=34:22 MISSING: ast,ir=28
+void callsSink(int sinkParam) { // $ ir-path=31:23 ir-path=32:26 ir-path=34:17
+  sink(sinkParam); // $ ast=31:28 ast=32:31 ast=34:22 ir-sink
 }
 
 struct {
@@ -25,7 +25,7 @@ void assignGlobals() {
 };
 
 void testStruct() {
-  globalStruct.sinkPtr(atoi(getenv("TAINTED"))); // $ ir MISSING: ast
+  globalStruct.sinkPtr(atoi(getenv("TAINTED"))); // $ MISSING: ir-path,ast
   globalStruct.notSinkPtr(atoi(getenv("TAINTED"))); // clean
 
   globalUnion.sinkPtr(atoi(getenv("TAINTED"))); // $ ast ir-path
@@ -48,8 +48,8 @@ class D2 : public D1 {
 
 class D3 : public D2 {
     public:
-    void f(const char* p) override { // $ ir-path=58:10 ir-path=60:17 ir-path=61:28 ir-path=62:29 ir-path=63:33 ir-path=73:30
-        sink(p); // $ ir-sink=58:10 ir-sink=60:17 ir-sink=61:28 ir-sink=62:29 ir-sink=63:33 ast=58:10 ast=60:17 ast=61:28 ast=62:29 ast=63:33 SPURIOUS: ast=73:30 ir-sink=73:30
+    void f(const char* p) override { // $ ir-path=58:10 ir-path=60:17 ir-path=61:28 ir-path=62:29 ir-path=63:33 SPURIOUS: ir-path=73:30
+        sink(p); // $ ast=58:10 ast=60:17 ast=61:28 ast=62:29 ast=63:33 ir-sink SPURIOUS: ast=73:30
     }
 };
 

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_path_to_sink/tainted.ql

@@ -23,19 +23,18 @@ class SourceConfiguration extends TaintedWithPath::TaintTrackingConfiguration {
   override predicate isSink(Element e) { isSinkArgument(e) }
 }
 
-predicate irTaint(Element source, Element sink, string tag) {
-  exists(TaintedWithPath::PathNode sinkNode, TaintedWithPath::PathNode predNode |
+predicate irTaint(Element source, TaintedWithPath::PathNode predNode, string tag) {
+  exists(TaintedWithPath::PathNode sinkNode |
     TaintedWithPath::taintedWithPath(source, _, _, sinkNode) and
     predNode = getAPredecessor*(sinkNode) and
-    sink = getElementFromPathNode(predNode) and
     // Make sure the path is actually reachable from this predecessor.
     // Otherwise, we could pick `predNode` to be b when `source` is
     // `source1` in this dataflow graph:
     // source1 ---> a ---> c ---> sinkNode
     //                   ^
     // source2 ---> b --/
     source = getElementFromPathNode(getAPredecessor*(predNode)) and
-    if sinkNode = predNode then tag = "ir-sink" else tag = "ir-path"
+    if predNode = sinkNode then tag = "ir-sink" else tag = "ir-path"
   )
 }
 
@@ -45,21 +44,25 @@ class IRDefaultTaintTrackingTest extends InlineExpectationsTest {
   override string getARelevantTag() { result = ["ir-path", "ir-sink"] }
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
-    exists(Element source, Element tainted, int n |
-      irTaint(source, tainted, tag) and
-      n = strictcount(Element otherSource | irTaint(otherSource, tainted, _)) and
-      (
-        n = 1 and value = ""
-        or
-        // If there is more than one source for this sink
-        // we specify the source location explicitly.
-        n > 1 and
+    exists(Element source, Element elem, TaintedWithPath::PathNode node, int n |
+      irTaint(source, node, tag) and
+      elem = getElementFromPathNode(node) and
+      n = count(int startline | getAPredecessor(node).hasLocationInfo(_, startline, _, _, _)) and
+      location = elem.getLocation() and
+      element = elem.toString()
+    |
+      // Zero predecessors means it's a source, and 1 predecessor means it has a unique predecessor.
+      // In either of these cases we leave out the location.
+      n = [0, 1] and value = ""
+      or
+      // If there is more than one predecessor for this node
+      // we specify the source location explicitly.
+      n > 1 and
+      exists(TaintedWithPath::PathNode pred | pred = getAPredecessor(node) |
         value =
-          source.getLocation().getStartLine().toString() + ":" +
-            source.getLocation().getStartColumn()
-      ) and
-      location = tainted.getLocation() and
-      element = tainted.toString()
+          getElementFromPathNode(pred).getLocation().getStartLine().toString() + ":" +
+            getElementFromPathNode(pred).getLocation().getStartColumn()
+      )
     )
   }
 }

cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/annotate_path_to_sink/test_diff.cpp

@@ -13,8 +13,8 @@ struct S {
     }
 };
 
-void calls_sink_with_argv(const char* a) { // $ ir-path=96:26 ir-path=98:18
-    sink(a); // $ ast=96:26 ast=98:18 ir-sink=96:26 ir-sink=98:18
+void calls_sink_with_argv(const char* a) { // $ ir-path=96:26 ir-path=102:26
+    sink(a); // $ ast=96:26 ast=98:18 ir-sink
 }
 
 extern int i;
@@ -27,7 +27,7 @@ class BaseWithPureVirtual {
 class DerivedCallsSink : public BaseWithPureVirtual {
 public:
     void f(const char* p) override { // $ ir-path
-        sink(p); // $ ir-sink ast=108:10 SPURIOUS: ast=111:10
+        sink(p); // $ ast=108:10 ir-sink SPURIOUS: ast=111:10
     }
 };
 
@@ -49,16 +49,16 @@ class DerivedDoesNotCallSinkDiamond2 : virtual public BaseWithPureVirtual {
 };
 
 class DerivesMultiple : public DerivedCallsSinkDiamond1, public DerivedDoesNotCallSinkDiamond2 {
-    void f(const char* p) override { // $ ir-path
-        DerivedCallsSinkDiamond1::f(p);
+    void f(const char* p) override { // $ ir-path=53:37 ir-path=115:11
+        DerivedCallsSinkDiamond1::f(p); // $ ir-path
     }
 };
 
 template<typename T>
 class CRTP {
 public:
     void f(const char* p) { // $ ir-path
-        static_cast<T*>(this)->g(p);
+        static_cast<T*>(this)->g(p); // $ ir-path
     }
 };
 
@@ -79,7 +79,7 @@ class Derived2 : public Derived1 {
 class Derived3 : public Derived2 {
     public:
     void f(const char* p) override { // $ ir-path=124:19 ir-path=126:43 ir-path=128:44
-        sink(p); // $ ast,ir-sink=124:19 ast,ir-sink=126:43 ast,ir-sink=128:44
+        sink(p); // $ ast=124:19 ast=126:43 ast=128:44 ir-sink
     }
 };
 
@@ -99,7 +99,7 @@ int main(int argc, char *argv[]) {
 
     sink(*p[0]); // $ ast,ir-sink
 
-    calls_sink_with_argv(*p[i]); // $ MISSING: ast,ir-path
+    calls_sink_with_argv(*p[i]); // $ ir-path MISSING:ast
 
     sink(*(argv + 1)); // $ ast,ir-path ir-sink