Skip to content

Commit 4412415

Browse files
MathiasVPMathiasVP
authored
Merge pull request #15078 from alexet/unique-pointer-temporary
CPP: Add query for detecting invalid uses of temporary unique pointers.
2 parents 32d1f05 + 57e0804 commit 4412415

12 files changed

+425
-76
lines changed

‎cpp/ql/src/Security/CWE/CWE-416/Temporaries.qll

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
import cpp
2+
import semmle.code.cpp.models.implementations.StdContainer
3+
4+
/**
5+
* Holds if `e` will be consumed by its parent as a glvalue and does not have
6+
* an lvalue-to-rvalue conversion. This means that it will be materialized into
7+
* a temporary object.
8+
*/
9+
predicate isTemporary(Expr e) {
10+
e instanceof TemporaryObjectExpr
11+
or
12+
e.isPRValueCategory() and
13+
e.getUnspecifiedType() instanceof Class and
14+
not e.hasLValueToRValueConversion()
15+
}
16+
17+
/** Holds if `e` is written to a container. */
18+
predicate isStoredInContainer(Expr e) {
19+
exists(StdSequenceContainerInsert insert, Call call, int index |
20+
call = insert.getACallToThisFunction() and
21+
index = insert.getAValueTypeParameterIndex() and
22+
call.getArgument(index) = e
23+
)
24+
or
25+
exists(StdSequenceContainerPush push, Call call, int index |
26+
call = push.getACallToThisFunction() and
27+
index = push.getAValueTypeParameterIndex() and
28+
call.getArgument(index) = e
29+
)
30+
or
31+
exists(StdSequenceEmplace emplace, Call call, int index |
32+
call = emplace.getACallToThisFunction() and
33+
index = emplace.getAValueTypeParameterIndex() and
34+
call.getArgument(index) = e
35+
)
36+
or
37+
exists(StdSequenceEmplaceBack emplaceBack, Call call, int index |
38+
call = emplaceBack.getACallToThisFunction() and
39+
index = emplaceBack.getAValueTypeParameterIndex() and
40+
call.getArgument(index) = e
41+
)
42+
}
43+
44+
/**
45+
* Holds if `e` or a conversion of `e` has an lvalue-to-rvalue conversion.
46+
*/
47+
private predicate hasLValueToRValueConversion(Expr e) {
48+
e.getConversion*().hasLValueToRValueConversion() and
49+
not e instanceof ConditionalExpr // ConditionalExpr may be spuriously reported as having an lvalue-to-rvalue conversion
50+
}
51+
52+
/**
53+
* Holds if the value of `e` outlives the enclosing full expression. For
54+
* example, because the value is stored in a local variable.
55+
*/
56+
predicate outlivesFullExpr(Expr e) {
57+
not hasLValueToRValueConversion(e) and
58+
(
59+
any(Assignment assign).getRValue() = e
60+
or
61+
any(Variable v).getInitializer().getExpr() = e
62+
or
63+
any(ReturnStmt ret).getExpr() = e
64+
or
65+
exists(ConditionalExpr cond |
66+
outlivesFullExpr(cond) and
67+
[cond.getThen(), cond.getElse()] = e
68+
)
69+
or
70+
exists(BinaryOperation bin |
71+
outlivesFullExpr(bin) and
72+
bin.getAnOperand() = e and
73+
not bin instanceof ComparisonOperation
74+
)
75+
or
76+
exists(PointerFieldAccess fa |
77+
outlivesFullExpr(fa) and
78+
fa.getQualifier() = e
79+
)
80+
or
81+
exists(AddressOfExpr ao |
82+
outlivesFullExpr(ao) and
83+
ao.getOperand() = e
84+
)
85+
or
86+
exists(ClassAggregateLiteral aggr |
87+
outlivesFullExpr(aggr) and
88+
aggr.getAFieldExpr(_) = e
89+
)
90+
or
91+
exists(ArrayAggregateLiteral aggr |
92+
outlivesFullExpr(aggr) and
93+
aggr.getAnElementExpr(_) = e
94+
)
95+
or
96+
isStoredInContainer(e)
97+
)
98+
}

‎cpp/ql/src/Security/CWE/CWE-416/UseOfStringAfterLifetimeEnds.ql

Lines changed: 1 addition & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -14,81 +14,7 @@
1414

1515
import cpp
1616
import semmle.code.cpp.models.implementations.StdString
17-
import semmle.code.cpp.models.implementations.StdContainer
18-
19-
/**
20-
* Holds if `e` will be consumed by its parent as a glvalue and does not have
21-
* an lvalue-to-rvalue conversion. This means that it will be materialized into
22-
* a temporary object.
23-
*/
24-
predicate isTemporary(Expr e) {
25-
e instanceof TemporaryObjectExpr
26-
or
27-
e.isPRValueCategory() and
28-
e.getUnspecifiedType() instanceof Class and
29-
not e.hasLValueToRValueConversion()
30-
}
31-
32-
/** Holds if `e` is written to a container. */
33-
predicate isStoredInContainer(Expr e) {
34-
exists(StdSequenceContainerInsert insert, Call call, int index |
35-
call = insert.getACallToThisFunction() and
36-
index = insert.getAValueTypeParameterIndex() and
37-
call.getArgument(index) = e
38-
)
39-
or
40-
exists(StdSequenceContainerPush push, Call call, int index |
41-
call = push.getACallToThisFunction() and
42-
index = push.getAValueTypeParameterIndex() and
43-
call.getArgument(index) = e
44-
)
45-
or
46-
exists(StdSequenceEmplace emplace, Call call, int index |
47-
call = emplace.getACallToThisFunction() and
48-
index = emplace.getAValueTypeParameterIndex() and
49-
call.getArgument(index) = e
50-
)
51-
or
52-
exists(StdSequenceEmplaceBack emplaceBack, Call call, int index |
53-
call = emplaceBack.getACallToThisFunction() and
54-
index = emplaceBack.getAValueTypeParameterIndex() and
55-
call.getArgument(index) = e
56-
)
57-
}
58-
59-
/**
60-
* Holds if the value of `e` outlives the enclosing full expression. For
61-
* example, because the value is stored in a local variable.
62-
*/
63-
predicate outlivesFullExpr(Expr e) {
64-
any(Assignment assign).getRValue() = e
65-
or
66-
any(Variable v).getInitializer().getExpr() = e
67-
or
68-
any(ReturnStmt ret).getExpr() = e
69-
or
70-
exists(ConditionalExpr cond |
71-
outlivesFullExpr(cond) and
72-
[cond.getThen(), cond.getElse()] = e
73-
)
74-
or
75-
exists(BinaryOperation bin |
76-
outlivesFullExpr(bin) and
77-
bin.getAnOperand() = e
78-
)
79-
or
80-
exists(ClassAggregateLiteral aggr |
81-
outlivesFullExpr(aggr) and
82-
aggr.getAFieldExpr(_) = e
83-
)
84-
or
85-
exists(ArrayAggregateLiteral aggr |
86-
outlivesFullExpr(aggr) and
87-
aggr.getAnElementExpr(_) = e
88-
)
89-
or
90-
isStoredInContainer(e)
91-
}
17+
import Temporaries
9218

9319
from Call c
9420
where

‎cpp/ql/src/Security/CWE/CWE-416/UseOfUniquePointerAfterLifetimeEnds.qhelp

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>Calling <code>get</code> on a <code>std::unique_ptr</code> object returns a pointer to the underlying allocations.
8+
When the <code>std::unique_ptr</code> object is destroyed, the pointer returned by <code>get</code> is no
9+
longer valid. If the pointer is used after the <code>std::unique_ptr</code> object is destroyed, then the behavior is undefined.
10+
</p>
11+
</overview>
12+
13+
<recommendation>
14+
<p>
15+
Ensure that the pointer returned by <code>get</code> does not outlive the underlying <code>std::unique_ptr</code> object.
16+
</p>
17+
</recommendation>
18+
19+
<example>
20+
<p>
21+
The following example gets a <code>std::unique_ptr</code> object, and then converts the resulting unique pointer to a
22+
pointer using <code>get</code> so that it can be passed to the <code>work</code> function.
23+
24+
However, the <code>std::unique_ptr</code> object is destroyed as soon as the call
25+
to <code>get</code> returns. This means that <code>work</code> is given a pointer to invalid memory.
26+
</p>
27+
28+
<sample src="UseOfUniquePointerAfterLifetimeEndsBad.cpp" />
29+
30+
<p>
31+
The following example fixes the above code by ensuring that the pointer returned by the call to <code>get</code> does
32+
not outlive the underlying <code>std::unique_ptr</code> objects. This ensures that the pointer passed to <code>work</code>
33+
points to valid memory.
34+
</p>
35+
36+
<sample src="UseOfUniquePointerAfterLifetimeEndsGood.cpp" />
37+
38+
</example>
39+
<references>
40+
41+
<li><a href="https://wiki.sei.cmu.edu/confluence/display/cplusplus/MEM50-CPP.+Do+not+access+freed+memory">MEM50-CPP. Do not access freed memory</a>.</li>
42+
43+
</references>
44+
</qhelp>

‎cpp/ql/src/Security/CWE/CWE-416/UseOfUniquePointerAfterLifetimeEnds.ql

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/**
2+
* @name Use of unique pointer after lifetime ends
3+
* @description Referencing the contents of a unique pointer after the underlying object has expired may lead to unexpected behavior.
4+
* @kind problem
5+
* @precision high
6+
* @id cpp/use-of-unique-pointer-after-lifetime-ends
7+
* @problem.severity warning
8+
* @security-severity 8.8
9+
* @tags reliability
10+
* security
11+
* external/cwe/cwe-416
12+
* external/cwe/cwe-664
13+
*/
14+
15+
import cpp
16+
import semmle.code.cpp.models.interfaces.PointerWrapper
17+
import Temporaries
18+
19+
predicate isUniquePointerDerefFunction(Function f) {
20+
exists(PointerWrapper wrapper |
21+
f = wrapper.getAnUnwrapperFunction() and
22+
// We only want unique pointers as the memory behind share pointers may still be
23+
// alive after the shared pointer is destroyed.
24+
wrapper.(Class).hasQualifiedName(["std", "bsl"], "unique_ptr")
25+
)
26+
}
27+
28+
from Call c
29+
where
30+
outlivesFullExpr(c) and
31+
not c.isFromUninstantiatedTemplate(_) and
32+
isUniquePointerDerefFunction(c.getTarget()) and
33+
isTemporary(c.getQualifier().getFullyConverted())
34+
select c,
35+
"The underlying unique pointer object is destroyed after the call to '" + c.getTarget() +
36+
"' returns."

‎cpp/ql/src/Security/CWE/CWE-416/UseOfUniquePointerAfterLifetimeEndsBad.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
#include <memory>
2+
std::unique_ptr<T> getUniquePointer();
3+
void work(const T*);
4+
5+
// BAD: the unique pointer is deallocated when `get` returns. So `work`
6+
// is given a pointer to invalid memory.
7+
void work_with_unique_ptr_bad() {
8+
const T* combined_string = getUniquePointer().get();
9+
work(combined_string);
10+
}

‎cpp/ql/src/Security/CWE/CWE-416/UseOfUniquePointerAfterLifetimeEndsGood.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
#include <memory>
2+
std::unique_ptr<T> getUniquePointer();
3+
void work(const T*);
4+
5+
// GOOD: the unique pointer outlives the call to `work`. So the pointer
6+
// obtainted from `get` is valid.
7+
void work_with_unique_ptr_good() {
8+
auto combined_string = getUniquePointer();
9+
work(combined_string.get());
10+
}

‎cpp/ql/src/change-notes/2023-12-12-use-of-unique-pointer-after-lifetime-ends.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `cpp/use-of-unique-pointer-after-lifetime-ends`, to detect uses of the contents unique pointers that will be destroyed immediately.

‎cpp/ql/test/query-tests/Security/CWE/CWE-416/semmle/tests/UseOfStringAfterLifetimeEnds/UseOfStringAfterLifetimeEnds.expected

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,4 +9,5 @@
99
| test.cpp:188:39:188:42 | call to data | The underlying string object is destroyed after the call to 'data' returns. |
1010
| test.cpp:189:44:189:47 | call to data | The underlying string object is destroyed after the call to 'data' returns. |
1111
| test.cpp:191:29:191:32 | call to data | The underlying string object is destroyed after the call to 'data' returns. |
12-
| test.cpp:193:31:193:35 | call to c_str | The underlying string object is destroyed after the call to 'c_str' returns. |
12+
| test.cpp:193:47:193:51 | call to c_str | The underlying string object is destroyed after the call to 'c_str' returns. |
13+
| test.cpp:195:31:195:35 | call to c_str | The underlying string object is destroyed after the call to 'c_str' returns. |

‎cpp/ql/test/query-tests/Security/CWE/CWE-416/semmle/tests/UseOfStringAfterLifetimeEnds/test.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,8 @@ const char* test1(bool b1, bool b2) {
190190
char* s9;
191191
s9 = std::string("hello").data(); // BAD
192192

193+
const char* s13 = b1 ? std::string("hello").c_str() : s1; // BAD
194+
193195
return std::string("hello").c_str(); // BAD
194196
}
195197

‎cpp/ql/test/query-tests/Security/CWE/CWE-416/semmle/tests/UseOfUniquePtrAfterLifetimeEnds/UseOfUniquePointerAfterLifetimeEnds.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
| test.cpp:156:15:156:15 | call to operator* | The underlying unique pointer object is destroyed after the call to 'operator*' returns. |
2+
| test.cpp:157:31:157:33 | call to get | The underlying unique pointer object is destroyed after the call to 'get' returns. |
3+
| test.cpp:159:32:159:32 | call to operator-> | The underlying unique pointer object is destroyed after the call to 'operator->' returns. |
4+
| test.cpp:160:35:160:37 | call to get | The underlying unique pointer object is destroyed after the call to 'get' returns. |
5+
| test.cpp:161:44:161:46 | call to get | The underlying unique pointer object is destroyed after the call to 'get' returns. |
6+
| test.cpp:163:25:163:27 | call to get | The underlying unique pointer object is destroyed after the call to 'get' returns. |
7+
| test.cpp:172:33:172:35 | call to get | The underlying unique pointer object is destroyed after the call to 'get' returns. |
8+
| test.cpp:174:32:174:34 | call to get | The underlying unique pointer object is destroyed after the call to 'get' returns. |
9+
| test.cpp:177:16:177:16 | call to operator* | The underlying unique pointer object is destroyed after the call to 'operator*' returns. |
10+
| test.cpp:177:36:177:36 | call to operator* | The underlying unique pointer object is destroyed after the call to 'operator*' returns. |
11+
| test.cpp:179:11:179:11 | call to operator* | The underlying unique pointer object is destroyed after the call to 'operator*' returns. |

0 commit comments

Comments
 (0)