hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-02-23 18:33:42 +01:00
Code Issues Packages Projects Releases Wiki Activity

C++: Move 'cpp/iterator-to-expired-container' out of experimental.

Browse Source
This commit is contained in:
Mathias Vorreiter Pedersen
2024-04-29 11:07:55 +01:00
parent 8ccedd658a
commit 179270ffc1
7 changed files with 1 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.qhelp
View File

@@ -1,53 +0,0 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
Using an iterator owned by a container after the lifetime of the container has expired can lead to undefined behavior.
This is because the iterator may be invalidated when the container is destroyed, and dereferencing an invalidated iterator is undefined behavior.
These problems can be hard to spot due to C++'s complex rules for temporary object lifetimes and their extensions.
</p>
</overview>
<recommendation>
<p>
Never create an iterator to a temporary container when the iterator is expected to be used after the container's lifetime has expired.
</p>
</recommendation>
<example>
<p>
</p>
<p>
The rules for lifetime extension ensures that the code in <code>lifetime_of_temp_extended</code> is well-defined. This is because the
lifetime of the temporary container returned by <code>get_vector</code> is extended to the end of the loop. However, prior to C++23,
the lifetime extension rules do not ensure that the container returned by <code>get_vector</code> is extended in <code>lifetime_of_temp_not_extended</code>.
This is because the temporary container is not bound to a rvalue reference.
</p>
<sample src="IteratorToExpiredContainerExtendedLifetime.cpp" />
</example>
<references>
<li>CERT C Coding Standard:
<a href="https://wiki.sei.cmu.edu/confluence/display/c/MEM30-C.+Do+not+access+freed+memory">MEM30-C. Do not access freed memory</a>.</li>
<li>
OWASP:
<a href="https://owasp.org/www-community/vulnerabilities/Using_freed_memory">Using freed memory</a>.
</li>
<li>
<a href="https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetime.pdf">Lifetime safety: Preventing common dangling</a>
</li>
<li>
<a href="https://en.cppreference.com/w/cpp/container">Containers library</a>
</li>
<li>
<a href="https://en.cppreference.com/w/cpp/language/range-for">Range-based for loop (since C++11)</a>
</li>
</references>
</qhelp>

129
cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql
View File

@@ -1,129 +0,0 @@
/**
* @name Iterator to expired container
* @description Using an iterator owned by a container whose lifetime has expired may lead to unexpected behavior.
* @kind problem
* @precision high
* @id cpp/iterator-to-expired-container
* @problem.severity warning
* @tags reliability
* security
* external/cwe/cwe-416
* external/cwe/cwe-664
*/
import cpp
import semmle.code.cpp.ir.IR
import semmle.code.cpp.dataflow.new.DataFlow
import semmle.code.cpp.models.implementations.StdContainer
import semmle.code.cpp.models.implementations.StdMap
import semmle.code.cpp.models.implementations.Iterator
private predicate tempToDestructorSink(DataFlow::Node sink, CallInstruction call) {
call = sink.asOperand().(ThisArgumentOperand).getCall() and
call.getStaticCallTarget() instanceof Destructor
}
/** Holds if `pun` is the post-update node of the qualifier of `Call`. */
private predicate isPostUpdateOfQualifier(CallInstruction call, DataFlow::PostUpdateNode pun) {
call.getThisArgumentOperand() = pun.getPreUpdateNode().asOperand()
}
/**
* Gets a `DataFlow::Node` that represents a temporary that will be destroyed
* by a call to a destructor when `n` is destroyed, or a `DataFlow::Node` that
* will transitively be destroyed by a call to a destructor.
*
* For the latter case, consider something like:
* ```
* std::vector<std::vector<int>> get_2d_vector();
* auto& v = get_2d_vector()[0];
* ```
* Given the above, this predicate returns the node corresponding
* to `get_2d_vector()[0]` since the temporary `get_2d_vector()` gets
* destroyed by a call to `std::vector<std::vector<int>>::~vector`,
* and thus the result of `get_2d_vector()[0]` is also an invalid reference.
*/
DataFlow::Node getADestroyedNode(DataFlow::Node n) {
// Case 1: The pointer that goes into the destructor call is destroyed
exists(CallInstruction destructorCall |
tempToDestructorSink(n, destructorCall) and
isPostUpdateOfQualifier(destructorCall, result)
)
or
// Case 2: Anything that was derived from the temporary that is now destroyed
// is also destroyed.
exists(CallInstruction call |
result.asInstruction() = call and
DataFlow::localFlow(DataFlow::operandNode(call.getThisArgumentOperand()), n)
|
call.getStaticCallTarget() instanceof StdSequenceContainerAt or
call.getStaticCallTarget() instanceof StdMapAt
)
}
predicate destroyedToBeginSink(DataFlow::Node sink, FunctionCall fc) {
exists(CallInstruction call |
call = sink.asOperand().(ThisArgumentOperand).getCall() and
fc = call.getUnconvertedResultExpression() and
call.getStaticCallTarget() instanceof BeginOrEndFunction
)
}
/**
* A configuration to track flow from a temporary variable to the qualifier of
* a destructor call, and subsequently to a qualifier of a call to `begin` or
* `end`.
*/
module Config implements DataFlow::StateConfigSig {
newtype FlowState =
additional TempToDestructor() or
additional DestroyedToBegin(DataFlow::Node n) {
exists(DataFlow::Node thisOperand |
tempToDestructorSink(thisOperand, _) and
n = getADestroyedNode(thisOperand)
)
}
predicate isSource(DataFlow::Node source, FlowState state) {
source.asInstruction().(VariableAddressInstruction).getIRVariable() instanceof IRTempVariable and
state = TempToDestructor()
}
predicate isAdditionalFlowStep(
DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
) {
tempToDestructorSink(node1, _) and
state1 = TempToDestructor() and
state2 = DestroyedToBegin(node2) and
node2 = getADestroyedNode(node1)
}
predicate isSink(DataFlow::Node sink, FlowState state) {
// Note: This is a non-trivial cartesian product!
// Hopefully, both of these sets are quite small in practice
destroyedToBeginSink(sink, _) and state instanceof DestroyedToBegin
}
DataFlow::FlowFeature getAFeature() {
// By blocking argument-to-parameter flow we ensure that we don't enter a
// function body where the temporary outlives anything inside the function.
// This prevents false positives in cases like:
// ```cpp
// void foo(const std::vector<int>& v) {
// for(auto x : v) { ... } // this is fine since v outlives the loop
// }
// ...
// foo(create_temporary())
// ```
result instanceof DataFlow::FeatureHasSinkCallContext
}
}
module Flow = DataFlow::GlobalWithState<Config>;
from Flow::PathNode source, Flow::PathNode sink, FunctionCall beginOrEnd, DataFlow::Node mid
where
Flow::flowPath(source, sink) and
destroyedToBeginSink(sink.getNode(), beginOrEnd) and
sink.getState() = Config::DestroyedToBegin(mid)
select mid, "This object is destroyed before $@ is called.", beginOrEnd, beginOrEnd.toString()

20
cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainerExtendedLifetime.cpp
View File

@@ -1,20 +0,0 @@
#include <vector>
std::vector<int> get_vector();
void use(int);
void lifetime_of_temp_extended() {
for(auto x : get_vector()) {
use(x); // GOOD: The lifetime of the vector returned by `get_vector()` is extended until the end of the loop.
}
}
// Writes the the values of `v` to an external log and returns it unchanged.
const std::vector<int>& log_and_return_argument(const std::vector<int>& v);
void lifetime_of_temp_not_extended() {
for(auto x : log_and_return_argument(get_vector())) {
use(x); // BAD: The lifetime of the vector returned by `get_vector()` is not extended, and the behavior is undefined.
}
}