diff --git a/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql b/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql index 7ea511b11cb..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.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 3e7026704ce..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 @@ -5,3 +5,5 @@ | 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: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 af9d5c44dd6..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 @@ -143,3 +143,49 @@ 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 +} + +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 +} + +void *conversionBeforeDataFlow() { + int myLocal; + void *pointerToLocal = (void *)&myLocal; // has conversion + return pointerToLocal; // BAD +} + +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 [NOT DETECTED] +}