From 4ea38495953093aabfbcaf8e02f5d6b7984f9c4f Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 11 Jan 2019 11:53:04 +0100 Subject: [PATCH 1/2] C++: Add failing test case for LargeParameter.ql --- .../Critical/LargeParameter/LargeParameter.expected | 1 + cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected b/cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected index 994bf3f86eae..75b6d5bf2390 100644 --- a/cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected +++ b/cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected @@ -1,3 +1,4 @@ | test.cpp:16:13:16:14 | _t | This parameter of type $@ is 4096 bytes - consider passing a 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 pointer/reference instead. | test.cpp:11:7:11:21 | myTemplateClass | myTemplateClass | | test.cpp:28:49:28:49 | b | This parameter of type $@ is 4096 bytes - consider passing a pointer/reference instead. | test.cpp:6:8:6:20 | myLargeStruct | myLargeStruct | +| test.cpp:52:52:52:54 | rhs | This parameter of type $@ is 4096 bytes - consider passing a pointer/reference instead. | test.cpp:48:8:48:25 | CustomAssignmentOp | CustomAssignmentOp | diff --git a/cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp b/cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp index 334eeadf4484..b45967b150d3 100644 --- a/cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp +++ b/cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp @@ -44,3 +44,11 @@ void myFunction2(mySmallStruct *a, myLargeStruct *b) // GOOD void myFunction3(mySmallStruct &a, myLargeStruct &b) // GOOD { } + +struct CustomAssignmentOp { + // GOOD: it's an accepted pattern to implement copy assignment via copy and + // swap. This delegates the resource management involved in copying to the + // copy constructor so that logic only has to be written once. + CustomAssignmentOp &operator=(CustomAssignmentOp rhs); // FALSE POSITIVE + char data[4096]; +}; From 1cc36dd96939f5fef24453acf738a3b262f31ed8 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Fri, 11 Jan 2019 11:57:45 +0100 Subject: [PATCH 2/2] C++: Exclude copy assignment in LargeParameter.ql The purpose of the copy assignment operator is to copy the object, so we should not complain that a copy happens when passing the parameter. See https://en.wikibooks.org/wiki/More_C++_Idioms/Copy-and-swap for details. --- cpp/ql/src/Critical/LargeParameter.ql | 1 + .../query-tests/Critical/LargeParameter/LargeParameter.expected | 1 - cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/Critical/LargeParameter.ql b/cpp/ql/src/Critical/LargeParameter.ql index 1571e96bbec9..c2fd1053ae07 100644 --- a/cpp/ql/src/Critical/LargeParameter.ql +++ b/cpp/ql/src/Critical/LargeParameter.ql @@ -18,6 +18,7 @@ where f.getAParameter() = p and t.getSize() = size and size > 64 and not t.getUnderlyingType() instanceof ArrayType + and not f instanceof CopyAssignmentOperator select p, "This parameter of type $@ is " + size.toString() + " bytes - consider passing a pointer/reference instead.", t, t.toString() diff --git a/cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected b/cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected index 75b6d5bf2390..994bf3f86eae 100644 --- a/cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected +++ b/cpp/ql/test/query-tests/Critical/LargeParameter/LargeParameter.expected @@ -1,4 +1,3 @@ | test.cpp:16:13:16:14 | _t | This parameter of type $@ is 4096 bytes - consider passing a 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 pointer/reference instead. | test.cpp:11:7:11:21 | myTemplateClass | myTemplateClass | | test.cpp:28:49:28:49 | b | This parameter of type $@ is 4096 bytes - consider passing a pointer/reference instead. | test.cpp:6:8:6:20 | myLargeStruct | myLargeStruct | -| test.cpp:52:52:52:54 | rhs | This parameter of type $@ is 4096 bytes - consider passing a pointer/reference instead. | test.cpp:48:8:48:25 | CustomAssignmentOp | CustomAssignmentOp | diff --git a/cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp b/cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp index b45967b150d3..cc9c217f9f9e 100644 --- a/cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp +++ b/cpp/ql/test/query-tests/Critical/LargeParameter/test.cpp @@ -49,6 +49,6 @@ struct CustomAssignmentOp { // GOOD: it's an accepted pattern to implement copy assignment via copy and // swap. This delegates the resource management involved in copying to the // copy constructor so that logic only has to be written once. - CustomAssignmentOp &operator=(CustomAssignmentOp rhs); // FALSE POSITIVE + CustomAssignmentOp &operator=(CustomAssignmentOp rhs); char data[4096]; };