Merge pull request #1279 from geoffw0/large-parameter

CPP: Tests and changes for LargeParameter.ql

cpp/ql/src/Critical/LargeParameter.ql

@@ -12,6 +12,7 @@
  */
 
 import cpp
+import semmle.code.cpp.dataflow.EscapesTree
 
 from Function f, Parameter p, Type t, int size
 where
@@ -20,7 +21,21 @@ where
   t.getSize() = size and
   size > 64 and
   not t.getUnderlyingType() instanceof ArrayType and
-  not f instanceof CopyAssignmentOperator
+  not f instanceof CopyAssignmentOperator and
+  // exception: p is written to, which may mean the copy is intended
+  not p.getAnAccess().isAddressOfAccessNonConst() and
+  not exists(Expr e |
+    variableAccessedAsValue(p.getAnAccess(), e.getFullyConverted()) and
+    (
+      exists(Assignment an | an.getLValue() = e)
+      or
+      exists(CrementOperation co | co.getOperand() = e)
+      or
+      exists(FunctionCall fc | fc.getQualifier() = e and not fc.getTarget().hasSpecifier("const"))
+    )
+  ) and
+  // if there's no block, we can't tell how the parameter is used
+  exists(f.getBlock())
 select p,
   "This parameter of type $@ is " + size.toString() +
     " bytes - consider passing a const pointer/reference instead.", t, t.toString()

cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected

@@ -1,3 +1,10 @@
 | test.cpp:16:13:16:14 | _t | This parameter of type $@ is 4096 bytes - consider passing a const pointer/reference instead. | test.cpp:6:8:6:20 | myLargeStruct | myLargeStruct |
 | test.cpp:24:44:24:48 | mtc_t | This parameter of type $@ is 4096 bytes - consider passing a const pointer/reference instead. | test.cpp:11:7:11:21 | myTemplateClass<myLargeStruct> | myTemplateClass<myLargeStruct> |
 | test.cpp:28:49:28:49 | b | This parameter of type $@ is 4096 bytes - consider passing a const pointer/reference instead. | test.cpp:6:8:6:20 | myLargeStruct | myLargeStruct |
+| test.cpp:104:16:104:16 | a | This parameter of type $@ is 4100 bytes - consider passing a const pointer/reference instead. | test.cpp:58:8:58:19 | MyLargeClass | MyLargeClass |
+| test.cpp:105:16:105:16 | b | This parameter of type $@ is 4100 bytes - consider passing a const pointer/reference instead. | test.cpp:58:8:58:19 | MyLargeClass | MyLargeClass |
+| test.cpp:106:16:106:16 | c | This parameter of type $@ is 4100 bytes - consider passing a const pointer/reference instead. | test.cpp:58:8:58:19 | MyLargeClass | MyLargeClass |
+| test.cpp:107:16:107:16 | d | This parameter of type $@ is 4100 bytes - consider passing a const pointer/reference instead. | test.cpp:58:8:58:19 | MyLargeClass | MyLargeClass |
+| test.cpp:108:16:108:16 | e | This parameter of type $@ is 4100 bytes - consider passing a const pointer/reference instead. | test.cpp:58:8:58:19 | MyLargeClass | MyLargeClass |
+| test.cpp:109:16:109:16 | f | This parameter of type $@ is 4100 bytes - consider passing a const pointer/reference instead. | test.cpp:58:8:58:19 | MyLargeClass | MyLargeClass |
+| test.cpp:161:7:161:7 | b | This parameter of type $@ is 3208 bytes - consider passing a const pointer/reference instead. | test.cpp:153:8:153:10 | big | big |

cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp

@@ -52,3 +52,115 @@ struct CustomAssignmentOp {
   CustomAssignmentOp &operator=(CustomAssignmentOp rhs);
   char data[4096];
 };
+
+// ---
+
+struct MyLargeClass {
+public:
+	MyLargeClass();
+
+	void myMethod();
+	void myConstMethod() const;
+
+	int value;
+	char data[4096];
+};
+
+void mlc_modify(MyLargeClass &c) {
+	c.value++;
+}
+
+int mlc_get(const MyLargeClass &c) {
+	return c.value;
+}
+
+void myFunction4(
+		MyLargeClass a, // GOOD: large, but the copy is written to so can't be trivially replaced with a reference
+		MyLargeClass b, // GOOD
+		MyLargeClass c, // GOOD
+		MyLargeClass d, // GOOD
+		MyLargeClass e, // GOOD
+		MyLargeClass f, // GOOD
+		MyLargeClass g // GOOD
+	)
+{
+	MyLargeClass *mlc_ptr;
+	int *i_ptr;
+	
+	a.value++;
+	b.value = 1;
+	c.data[0] += 1;
+	d.myMethod();
+	mlc_modify(e);
+
+	mlc_ptr = &f;
+	mlc_modify(*mlc_ptr);
+
+	i_ptr = &g.value;
+	*(i_ptr)++;
+}
+
+void myFunction5(
+		MyLargeClass a, // BAD
+		MyLargeClass b, // BAD
+		MyLargeClass c, // BAD
+		MyLargeClass d, // BAD
+		MyLargeClass e, // BAD
+		MyLargeClass f // BAD
+	)
+{
+	const MyLargeClass *mlc_ptr;
+	const int *i_ptr;
+	int i;
+	
+	i = a.value;
+	i += b.data[0];
+	c.myConstMethod();
+	i += mlc_get(d);
+
+	mlc_ptr = &e;
+	mlc_get(*mlc_ptr);
+
+	i_ptr = &f.value;
+	i += *i_ptr;
+}
+
+// ---
+
+class MyArithmeticClass {
+public:
+	MyArithmeticClass(int _value) : value(_value) {};
+
+	MyArithmeticClass &operator+=(const MyArithmeticClass &other) {
+		this->value += other.value;
+		return *this;
+	}
+
+private:
+	int value;
+	char data[1024];
+};
+
+MyArithmeticClass operator+(MyArithmeticClass lhs, const MyArithmeticClass &rhs) { // GOOD
+	lhs += rhs; // lhs is copied by design
+	return lhs;
+}
+
+void myFunction6(MyLargeClass a); // GOOD (no definition, so we can't tell what's done with `a`)
+
+// ---
+
+struct big
+{
+	int xs[800];
+	int *ptr;
+};
+
+void myFunction7(
+		big a, // GOOD
+		big b // BAD
+	)
+{
+	a.xs[0]++; // modifies a
+	b.ptr[0]++; // does not modify b
+}