Merge branch 'main' into mathiasvp/read-step-without-memory-operands

MathiasVP · MathiasVP · commit 208b85c7fc60 · 2020-09-04T18:04:15.000+02:00
diff --git a/change-notes/1.26/analysis-cpp.md b/change-notes/1.26/analysis-cpp.md
@@ -13,6 +13,7 @@ The following changes in version 1.26 affect C/C++ analysis in all applications.
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Declaration hides parameter (`cpp/declaration-hides-parameter`) | Fewer false positive results | False positives involving template functions have been fixed. |
 | Inconsistent direction of for loop (`cpp/inconsistent-loop-direction`) | Fewer false positive results | The query now accounts for intentional wrapping of an unsigned loop counter. |
 | Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | | The precision of this query has been decreased from "high" to "medium". As a result, the query is still run but results are no longer displayed on LGTM by default. |
 | Comparison result is always the same (`cpp/constant-comparison`) | More correct results | Bounds on expressions involving multiplication can now be determined in more cases. |
diff --git a/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql b/cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql
@@ -11,20 +11,35 @@
 
 import cpp
 
+/**
+ * Gets the template that a function `f` is constructed from, or just `f` if it
+ * is not from a template instantiation.
+ */
+Function getConstructedFrom(Function f) {
+  f.isConstructedFrom(result)
+  or
+  not f.isConstructedFrom(_) and
+  result = f
+}
+
 /**
  * Gets the parameter of `f` with name `name`, which has to come from the
  * _definition_ of `f` and not a prototype declaration.
  * We also exclude names from functions that have multiple definitions.
  * This should not happen in a single application but since we
  * have a system wide view it is likely to happen for instance for
  * the main function.
+ *
+ * Note: we use `getConstructedFrom` to ensure that we look at template
+ * functions rather than their instantiations. We get better results this way
+ * as the instantiation is artificial and may have inherited parameter names
+ * from the declaration rather than the definition.
  */
 ParameterDeclarationEntry functionParameterNames(Function f, string name) {
   exists(FunctionDeclarationEntry fe |
     result.getFunctionDeclarationEntry() = fe and
-    fe.getFunction() = f and
+    getConstructedFrom(f).getDefinition() = fe and
     fe.getLocation() = f.getDefinitionLocation() and
-    result.getFile() = fe.getFile() and // Work around CPP-331
     strictcount(f.getDefinitionLocation()) = 1 and
     result.getName() = name
   )
diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/StdString.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/StdString.qll
@@ -1,6 +1,6 @@
 /**
  * Provides implementation classes modeling `std::string` and other
- * instantiations of`std::basic_string`. See `semmle.code.cpp.models.Models`
+ * instantiations of `std::basic_string`. See `semmle.code.cpp.models.Models`
  * for usage information.
  */
 
@@ -82,6 +82,32 @@ class StdStringData extends TaintFunction {
   }
 }
 
+/**
+ * The `std::string` function `push_back`.
+ */
+class StdStringPush extends TaintFunction {
+  StdStringPush() { this.hasQualifiedName("std", "basic_string", "push_back") }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from parameter to qualifier
+    input.isParameterDeref(0) and
+    output.isQualifierObject()
+  }
+}
+
+/**
+ * The `std::string` functions `front` and `back`.
+ */
+class StdStringFrontBack extends TaintFunction {
+  StdStringFrontBack() { this.hasQualifiedName("std", "basic_string", ["front", "back"]) }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from object to returned reference
+    input.isQualifierObject() and
+    output.isReturnValueDeref()
+  }
+}
+
 /**
  * The `std::string` function `operator+`.
  */
@@ -138,6 +164,11 @@ class StdStringAppend extends TaintFunction {
       output.isQualifierObject() or
       output.isReturnValueDeref()
     )
+    or
+    // reverse flow from returned reference to the qualifier (for writes to
+    // the result)
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
   }
 }
 
@@ -173,6 +204,11 @@ class StdStringAssign extends TaintFunction {
       output.isQualifierObject() or
       output.isReturnValueDeref()
     )
+    or
+    // reverse flow from returned reference to the qualifier (for writes to
+    // the result)
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
   }
 }
 
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/stl.h b/cpp/ql/test/library-tests/dataflow/taint-tests/stl.h
@@ -91,6 +91,13 @@ namespace std
 		const_iterator cbegin() const;
 		const_iterator cend() const;
 
+		void push_back(charT c);
+
+		const charT& front() const;
+		charT& front();
+		const charT& back() const;
+		charT& back();
+
 		const_reference operator[](size_type pos) const;
 		reference operator[](size_type pos);
 		const_reference at(size_type n) const;
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/string.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/string.cpp
@@ -158,12 +158,12 @@ void test_string_append() {
 		sink(s5); // tainted
 
 		s6 = s3;
-		s6 += s4;
+		sink(s6 += s4); // tainted
 		sink(s6); // tainted
 
 		s7 = s3;
-		s7 += source();
-		s7 += " ";
+		sink(s7 += source()); // tainted
+		sink(s7 += " "); // tainted
 		sink(s7); // tainted
 
 		s8 = s3;
@@ -505,3 +505,55 @@ void test_constructors_more() {
 	sink(s3);
 	sink(s4); // tainted
 }
+
+void test_string_front_back() {
+	std::string a("aa");
+
+	sink(a.front());
+	sink(a.back());
+	a.push_back(ns_char::source());
+	sink(a.front()); // [FALSE POSITIVE]
+	sink(a.back()); // tainted
+}
+
+void test_string_return_assign() {
+	{
+		std::string a("aa");
+		std::string b("bb");
+		std::string c("cc");
+		std::string d("dd");
+		std::string e("ee");
+		std::string f("ff");
+
+		sink( a += (b += "bb") );
+		sink( c += (d += source()) ); // tainted
+		sink( (e += "ee") += source() ); // tainted
+		sink( (f += source()) += "ff" ); // tainted
+		sink(a);
+		sink(b);
+		sink(c); // tainted
+		sink(d); // tainted
+		sink(e); // tainted
+		sink(f); // tainted
+	}
+
+	{
+		std::string a("aa");
+		std::string b("bb");
+		std::string c("cc");
+		std::string d("dd");
+		std::string e("ee");
+		std::string f("ff");
+
+		sink( a.assign(b.assign("bb")) );
+		sink( c.assign(d.assign(source())) ); // tainted
+		sink( e.assign("ee").assign(source()) ); // tainted
+		sink( f.assign(source()).assign("ff") );
+		sink(a);
+		sink(b);
+		sink(c); // tainted
+		sink(d); // tainted
+		sink(e); // tainted
+		sink(f); // [FALSE POSITIVE]
+	}
+}
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected
@@ -67,8 +67,11 @@
 | string.cpp:146:11:146:11 | call to operator+ | string.cpp:141:18:141:23 | call to source |
 | string.cpp:149:11:149:11 | call to operator+ | string.cpp:149:13:149:18 | call to source |
 | string.cpp:158:8:158:9 | s5 | string.cpp:154:18:154:23 | call to source |
+| string.cpp:161:11:161:11 | call to operator+= | string.cpp:154:18:154:23 | call to source |
 | string.cpp:162:8:162:9 | s6 | string.cpp:154:18:154:23 | call to source |
-| string.cpp:167:8:167:9 | s7 | string.cpp:165:9:165:14 | call to source |
+| string.cpp:165:11:165:11 | call to operator+= | string.cpp:165:14:165:19 | call to source |
+| string.cpp:166:11:166:11 | call to operator+= | string.cpp:165:14:165:19 | call to source |
+| string.cpp:167:8:167:9 | s7 | string.cpp:165:14:165:19 | call to source |
 | string.cpp:171:8:171:9 | s8 | string.cpp:154:18:154:23 | call to source |
 | string.cpp:176:8:176:9 | s9 | string.cpp:174:13:174:18 | call to source |
 | string.cpp:184:8:184:10 | s10 | string.cpp:181:12:181:26 | call to source |
@@ -138,6 +141,21 @@
 | string.cpp:491:8:491:9 | s6 | string.cpp:482:18:482:23 | call to source |
 | string.cpp:504:7:504:8 | s2 | string.cpp:497:14:497:19 | call to source |
 | string.cpp:506:7:506:8 | s4 | string.cpp:497:14:497:19 | call to source |
+| string.cpp:515:9:515:13 | call to front | string.cpp:514:14:514:28 | call to source |
+| string.cpp:516:9:516:12 | call to back | string.cpp:514:14:514:28 | call to source |
+| string.cpp:529:11:529:11 | call to operator+= | string.cpp:529:20:529:25 | call to source |
+| string.cpp:530:21:530:21 | call to operator+= | string.cpp:530:24:530:29 | call to source |
+| string.cpp:531:25:531:25 | call to operator+= | string.cpp:531:15:531:20 | call to source |
+| string.cpp:534:8:534:8 | c | string.cpp:529:20:529:25 | call to source |
+| string.cpp:535:8:535:8 | d | string.cpp:529:20:529:25 | call to source |
+| string.cpp:536:8:536:8 | e | string.cpp:530:24:530:29 | call to source |
+| string.cpp:537:8:537:8 | f | string.cpp:531:15:531:20 | call to source |
+| string.cpp:549:11:549:16 | call to assign | string.cpp:549:27:549:32 | call to source |
+| string.cpp:550:24:550:29 | call to assign | string.cpp:550:31:550:36 | call to source |
+| string.cpp:554:8:554:8 | c | string.cpp:549:27:549:32 | call to source |
+| string.cpp:555:8:555:8 | d | string.cpp:549:27:549:32 | call to source |
+| string.cpp:556:8:556:8 | e | string.cpp:550:31:550:36 | call to source |
+| string.cpp:557:8:557:8 | f | string.cpp:551:18:551:23 | call to source |
 | structlikeclass.cpp:35:8:35:9 | s1 | structlikeclass.cpp:29:22:29:27 | call to source |
 | structlikeclass.cpp:36:8:36:9 | s2 | structlikeclass.cpp:30:24:30:29 | call to source |
 | structlikeclass.cpp:37:8:37:9 | s3 | structlikeclass.cpp:29:22:29:27 | call to source |
diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected
@@ -67,8 +67,11 @@
 | string.cpp:146:11:146:11 | string.cpp:141:18:141:23 | AST only |
 | string.cpp:149:11:149:11 | string.cpp:149:13:149:18 | AST only |
 | string.cpp:158:8:158:9 | string.cpp:154:18:154:23 | AST only |
+| string.cpp:161:11:161:11 | string.cpp:154:18:154:23 | AST only |
 | string.cpp:162:8:162:9 | string.cpp:154:18:154:23 | AST only |
-| string.cpp:167:8:167:9 | string.cpp:165:9:165:14 | AST only |
+| string.cpp:165:11:165:11 | string.cpp:165:14:165:19 | AST only |
+| string.cpp:166:11:166:11 | string.cpp:165:14:165:19 | AST only |
+| string.cpp:167:8:167:9 | string.cpp:165:14:165:19 | AST only |
 | string.cpp:171:8:171:9 | string.cpp:154:18:154:23 | AST only |
 | string.cpp:176:8:176:9 | string.cpp:174:13:174:18 | AST only |
 | string.cpp:184:8:184:10 | string.cpp:181:12:181:26 | AST only |
@@ -138,6 +141,21 @@
 | string.cpp:491:8:491:9 | string.cpp:482:18:482:23 | AST only |
 | string.cpp:504:7:504:8 | string.cpp:497:14:497:19 | AST only |
 | string.cpp:506:7:506:8 | string.cpp:497:14:497:19 | AST only |
+| string.cpp:515:9:515:13 | string.cpp:514:14:514:28 | AST only |
+| string.cpp:516:9:516:12 | string.cpp:514:14:514:28 | AST only |
+| string.cpp:529:11:529:11 | string.cpp:529:20:529:25 | AST only |
+| string.cpp:530:21:530:21 | string.cpp:530:24:530:29 | AST only |
+| string.cpp:531:25:531:25 | string.cpp:531:15:531:20 | AST only |
+| string.cpp:534:8:534:8 | string.cpp:529:20:529:25 | AST only |
+| string.cpp:535:8:535:8 | string.cpp:529:20:529:25 | AST only |
+| string.cpp:536:8:536:8 | string.cpp:530:24:530:29 | AST only |
+| string.cpp:537:8:537:8 | string.cpp:531:15:531:20 | AST only |
+| string.cpp:549:11:549:16 | string.cpp:549:27:549:32 | AST only |
+| string.cpp:550:24:550:29 | string.cpp:550:31:550:36 | AST only |
+| string.cpp:554:8:554:8 | string.cpp:549:27:549:32 | AST only |
+| string.cpp:555:8:555:8 | string.cpp:549:27:549:32 | AST only |
+| string.cpp:556:8:556:8 | string.cpp:550:31:550:36 | AST only |
+| string.cpp:557:8:557:8 | string.cpp:551:18:551:23 | AST only |
 | swap1.cpp:78:12:78:16 | swap1.cpp:69:23:69:23 | AST only |
 | swap1.cpp:87:13:87:17 | swap1.cpp:82:16:82:21 | AST only |
 | swap1.cpp:88:13:88:17 | swap1.cpp:81:27:81:28 | AST only |
diff --git a/cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/DeclarationHidesParameter.expected b/cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/DeclarationHidesParameter.expected
@@ -1,2 +1,7 @@
 | hiding.cpp:4:17:4:18 | ii | Local variable 'ii' hides a $@. | hiding.cpp:2:12:2:13 | definition of ii | parameter of the same name |
 | hiding.cpp:15:15:15:16 | kk | Local variable 'kk' hides a $@. | hiding.cpp:12:25:12:26 | definition of kk | parameter of the same name |
+| hiding.cpp:28:7:28:7 | a | Local variable 'a' hides a $@. | hiding.cpp:26:21:26:21 | definition of a | parameter of the same name |
+| hiding.cpp:45:7:45:7 | a | Local variable 'a' hides a $@. | hiding.cpp:43:41:43:41 | definition of a | parameter of the same name |
+| hiding.cpp:64:11:64:11 | i | Local variable 'i' hides a $@. | hiding.cpp:61:20:61:20 | definition of i | parameter of the same name |
+| hiding.cpp:78:7:78:10 | arg1 | Local variable 'arg1' hides a $@. | hiding.cpp:74:28:74:31 | definition of arg1 | parameter of the same name |
+| hiding.cpp:79:5:79:8 | arg2 | Local variable 'arg2' hides a $@. | hiding.cpp:74:36:74:39 | definition of arg2 | parameter of the same name |
diff --git a/cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/hiding.cpp b/cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/hiding.cpp
@@ -1,7 +1,7 @@
 
 void f(int ii) {
     if (1) {
-        for(int ii = 1; ii < 10; ii++) {
+        for(int ii = 1; ii < 10; ii++) { // local variable hides parameter of the same name
             ;
         }
     }
@@ -12,7 +12,7 @@ namespace foo {
     void f2(int ii, int kk) {
       try {
         for (ii = 0; ii < 3; ii++) {
-          int kk;
+          int kk; // local variable hides parameter of the same name
         }
       }
       catch (int ee) {
@@ -21,4 +21,61 @@ namespace foo {
   }
 }
 
+void myFunction(int a, int b, int c);
 
+void myFunction(int a, int b, int _c) {
+	{
+		int a = a; // local variable hides parameter of the same name
+		int _b = b;
+		int c = _c;
+
+		// ...
+	}
+}
+
+template<class T>
+class MyTemplateClass {
+public:
+	void myMethod(int a, int b, int c);
+};
+
+template<class T>
+void MyTemplateClass<T> :: myMethod(int a, int b, int _c) {
+	{
+		int a = a; // local variable hides parameter of the same name
+		int _b = b;
+		int c = _c;
+
+		// ...
+	}
+}
+
+MyTemplateClass<int> mtc_i;
+
+void test() {
+	mtc_i.myMethod(0, 0, 0);
+}
+
+#define MYMACRO for (int i = 0; i < 10; i++) {}
+
+void testMacro(int i) {
+	MYMACRO;
+
+	for (int i = 0; i < 10; i++) {}; // local variable hides parameter of the same name
+}
+
+#include "hiding.h"
+
+void myClass::myCaller(void) {
+  this->myMethod(5, 6);
+}
+
+template <typename T>
+void myClass::myMethod(int arg1, T arg2) {
+	{
+		int protoArg1;
+		T protoArg2;
+		int arg1; // local variable hides parameter of the same name
+		T arg2; // local variable hides parameter of the same name
+	}
+}
diff --git a/cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/hiding.h b/cpp/ql/test/query-tests/Best Practices/Hiding/DeclarationHidesParameter/hiding.h
@@ -0,0 +1,7 @@
+
+class myClass {
+public:
+  template <typename T>
+  void myMethod(int protoArg1, T protoArg2);
+  void myCaller(void);
+};
