From 1a7351ef6e7a3a33b9843dd4810cb616fa0060f3 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Sat, 16 Mar 2019 20:11:01 +0100 Subject: [PATCH 1/4] C++: Add tests for three FPs observed on lgtm.com --- .../ReturnStackAllocatedMemory.expected | 3 +++ .../ReturnStackAllocatedMemory/test.cpp | 21 +++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected index 3e7026704ce..90a9789729c 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected @@ -5,3 +5,6 @@ | test.cpp:92:2:92:12 | return ... | May return stack-allocated memory from $@. | test.cpp:89:10:89:11 | mc | mc | | test.cpp:112:2:112:12 | return ... | May return stack-allocated memory from $@. | test.cpp:112:9:112:11 | arr | arr | | test.cpp:119:2:119:19 | return ... | May return stack-allocated memory from $@. | test.cpp:119:11:119:13 | arr | arr | +| test.cpp:149:3:149:22 | return ... | May return stack-allocated memory from $@. | test.cpp:149:11:149:21 | threadLocal | threadLocal | +| test.cpp:155:3:155:18 | return ... | May return stack-allocated memory from $@. | test.cpp:154:19:154:26 | localInt | localInt | +| test.cpp:165:3:165:19 | return ... | May return stack-allocated memory from $@. | test.cpp:164:21:164:28 | localBuf | localBuf | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp index af9d5c44dd6..566c1cc860e 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp @@ -143,3 +143,24 @@ char *testArray5() return arr; // GOOD } + +int *returnThreadLocal() { + thread_local int threadLocal; + return &threadLocal; // GOOD [FALSE POSITIVE] +} + +int returnDereferenced() { + int localInt = 2; + int &localRef = localInt; + return localRef; // GOOD [FALSE POSITIVE] +} + +typedef unsigned long size_t; +void *memcpy(void *s1, const void *s2, size_t n); + +char *returnAfterCopy() { + char localBuf[] = "Data"; + static char staticBuf[sizeof(localBuf)]; + memcpy(staticBuf, localBuf, sizeof(staticBuf)); + return staticBuf; // GOOD [FALSE POSITIVE] +} From 6b1cd17009385831b9c6698d9545bb93e835c345 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Sat, 16 Mar 2019 20:40:24 +0100 Subject: [PATCH 2/4] C++: Fix FPs due to data flow Conversion handling Since we cannot track data flow from a fully-converted expression but only the unconverted expression, we should check whether the address initially escapes into the unconverted expression, not the fully-converted one. This fixes most of the false positives observed on lgtm.com. --- .../Memory Management/ReturnStackAllocatedMemory.ql | 2 +- .../ReturnStackAllocatedMemory.expected | 2 -- .../Memory Management/ReturnStackAllocatedMemory/test.cpp | 4 ++-- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql b/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql index 7ea511b11cb..bc3546f36bd 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql @@ -41,7 +41,7 @@ where // the address escapes into some expression `pointerToLocal`, which flows // in a non-trivial way (one or more steps) to a returned expression. exists(Expr pointerToLocal | - variableAddressEscapesTree(va, pointerToLocal.getFullyConverted()) and + variableAddressEscapesTree(va, pointerToLocal) and conservativeDataFlowStep+( DataFlow::exprNode(pointerToLocal), DataFlow::exprNode(r.getExpr()) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected index 90a9789729c..74c71d74d01 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected @@ -6,5 +6,3 @@ | test.cpp:112:2:112:12 | return ... | May return stack-allocated memory from $@. | test.cpp:112:9:112:11 | arr | arr | | test.cpp:119:2:119:19 | return ... | May return stack-allocated memory from $@. | test.cpp:119:11:119:13 | arr | arr | | test.cpp:149:3:149:22 | return ... | May return stack-allocated memory from $@. | test.cpp:149:11:149:21 | threadLocal | threadLocal | -| test.cpp:155:3:155:18 | return ... | May return stack-allocated memory from $@. | test.cpp:154:19:154:26 | localInt | localInt | -| test.cpp:165:3:165:19 | return ... | May return stack-allocated memory from $@. | test.cpp:164:21:164:28 | localBuf | localBuf | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp index 566c1cc860e..daddfd9b71a 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp @@ -152,7 +152,7 @@ int *returnThreadLocal() { int returnDereferenced() { int localInt = 2; int &localRef = localInt; - return localRef; // GOOD [FALSE POSITIVE] + return localRef; // GOOD } typedef unsigned long size_t; @@ -162,5 +162,5 @@ char *returnAfterCopy() { char localBuf[] = "Data"; static char staticBuf[sizeof(localBuf)]; memcpy(staticBuf, localBuf, sizeof(staticBuf)); - return staticBuf; // GOOD [FALSE POSITIVE] + return staticBuf; // GOOD } From d864df5b7f3957f66b738fb2b9ad955fb5c4d34d Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 19 Mar 2019 10:19:13 +0100 Subject: [PATCH 3/4] C++: Tests for new false negatives --- .../ReturnStackAllocatedMemory.expected | 1 + .../ReturnStackAllocatedMemory/test.cpp | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected index 74c71d74d01..9433387203f 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected @@ -6,3 +6,4 @@ | test.cpp:112:2:112:12 | return ... | May return stack-allocated memory from $@. | test.cpp:112:9:112:11 | arr | arr | | test.cpp:119:2:119:19 | return ... | May return stack-allocated memory from $@. | test.cpp:119:11:119:13 | arr | arr | | test.cpp:149:3:149:22 | return ... | May return stack-allocated memory from $@. | test.cpp:149:11:149:21 | threadLocal | threadLocal | +| test.cpp:190:3:190:14 | return ... | May return stack-allocated memory from $@. | test.cpp:188:13:188:19 | myLocal | myLocal | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp index daddfd9b71a..01ef14494f7 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp @@ -164,3 +164,28 @@ char *returnAfterCopy() { memcpy(staticBuf, localBuf, sizeof(staticBuf)); return staticBuf; // GOOD } + +void *conversionBeforeDataFlow() { + int myLocal; + void *pointerToLocal = (void *)&myLocal; // has conversion + return pointerToLocal; // BAD [NOT DETECTED] +} + +void *arrayConversionBeforeDataFlow() { + int localArray[4]; + int *pointerToLocal = localArray; // has conversion + return pointerToLocal; // BAD [NOT DETECTED] +} + +int &dataFlowThroughReference() { + int myLocal; + int &refToLocal = myLocal; // has conversion + return refToLocal; // BAD [NOT DETECTED] +} + +int *&conversionInFlow() { + int myLocal; + int *p = &myLocal; + int *&pRef = p; // has conversion in the middle of data flow + return pRef; // BAD [MISLEADING ALERT MESSAGE] +} From 111a462d16c0c2d228c5d905052e07c5ee57aa27 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 19 Mar 2019 11:09:58 +0100 Subject: [PATCH 4/4] C++: Recover some of the good results we lost My recent changes to suppress FPs in `ReturnStackAllocatedMemory.ql` caused us to lose all results where there was a `Conversion` at the initial address escape. We cannot handle conversions in general, but this commit restores the good results for the trivial types of conversion that we can handle. --- .../ReturnStackAllocatedMemory.ql | 24 ++++++++++++++++--- .../ReturnStackAllocatedMemory.expected | 2 +- .../ReturnStackAllocatedMemory/test.cpp | 4 ++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql b/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql index bc3546f36bd..5a3c0e9fe1f 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql @@ -22,7 +22,24 @@ import semmle.code.cpp.dataflow.DataFlow */ predicate conservativeDataFlowStep(DataFlow::Node n1, DataFlow::Node n2) { DataFlow::localFlowStep(n1, n2) and - not n2.asExpr() instanceof FieldAccess + not n2.asExpr() instanceof FieldAccess and + not hasNontrivialConversion(n2.asExpr()) +} + +/** + * Holds if `e` has a conversion that changes it from lvalue to pointer or + * back. As the data-flow library does not support conversions, we cannot track + * data flow through such expressions. + */ +predicate hasNontrivialConversion(Expr e) { + e instanceof Conversion and not + ( + e instanceof Cast + or + e instanceof ParenthesisExpr + ) + or + hasNontrivialConversion(e.getConversion()) } from LocalScopeVariable var, VariableAccess va, ReturnStmt r @@ -39,9 +56,10 @@ where or // The data flow library doesn't support conversions, so here we check that // the address escapes into some expression `pointerToLocal`, which flows - // in a non-trivial way (one or more steps) to a returned expression. + // in a one or more steps to a returned expression. exists(Expr pointerToLocal | - variableAddressEscapesTree(va, pointerToLocal) and + variableAddressEscapesTree(va, pointerToLocal.getFullyConverted()) and + not hasNontrivialConversion(pointerToLocal) and conservativeDataFlowStep+( DataFlow::exprNode(pointerToLocal), DataFlow::exprNode(r.getExpr()) diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected index 9433387203f..3ed977feaed 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected @@ -6,4 +6,4 @@ | test.cpp:112:2:112:12 | return ... | May return stack-allocated memory from $@. | test.cpp:112:9:112:11 | arr | arr | | test.cpp:119:2:119:19 | return ... | May return stack-allocated memory from $@. | test.cpp:119:11:119:13 | arr | arr | | test.cpp:149:3:149:22 | return ... | May return stack-allocated memory from $@. | test.cpp:149:11:149:21 | threadLocal | threadLocal | -| test.cpp:190:3:190:14 | return ... | May return stack-allocated memory from $@. | test.cpp:188:13:188:19 | myLocal | myLocal | +| test.cpp:171:3:171:24 | return ... | May return stack-allocated memory from $@. | test.cpp:170:35:170:41 | myLocal | myLocal | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp index 01ef14494f7..3aa6cd3e831 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp @@ -168,7 +168,7 @@ char *returnAfterCopy() { void *conversionBeforeDataFlow() { int myLocal; void *pointerToLocal = (void *)&myLocal; // has conversion - return pointerToLocal; // BAD [NOT DETECTED] + return pointerToLocal; // BAD } void *arrayConversionBeforeDataFlow() { @@ -187,5 +187,5 @@ int *&conversionInFlow() { int myLocal; int *p = &myLocal; int *&pRef = p; // has conversion in the middle of data flow - return pRef; // BAD [MISLEADING ALERT MESSAGE] + return pRef; // BAD [NOT DETECTED] }