mirror of
https://github.com/github/codeql.git
synced 2026-04-29 02:35:15 +02:00
C++: Add new query for unsafe use of this.
This commit is contained in:
Mathias Vorreiter Pedersen
@@ -9,6 +9,7 @@
|
||||
+ semmlecode-cpp-queries/Likely Bugs/Conversion/CastArrayPointerArithmetic.ql: /Correctness/Dangerous Conversions
|
||||
+ semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql: /Correctness/Dangerous Conversions
|
||||
+ semmlecode-cpp-queries/Security/CWE/CWE-253/HResultBooleanConversion.ql: /Correctness/Dangerous Conversions
|
||||
+ semmlecode-cpp-queries/Likely Bugs/OO/UnsafeUseOfThis.ql: /Correctness/Dangerous Conversions
|
||||
# Consistent Use
|
||||
+ semmlecode-cpp-queries/Critical/ReturnValueIgnored.ql: /Correctness/Consistent Use
|
||||
+ semmlecode-cpp-queries/Likely Bugs/InconsistentCheckReturnNull.ql: /Correctness/Consistent Use
|
||||
|
||||
20
cpp/ql/src/Likely Bugs/OO/UnsafeUseOfThis.cpp
Normal file
20
cpp/ql/src/Likely Bugs/OO/UnsafeUseOfThis.cpp
Normal file
@@ -0,0 +1,20 @@
|
||||
class Base {
|
||||
private:
|
||||
// pure virtual member function used for initialization of derived classes.
|
||||
virtual void construct() = 0;
|
||||
public:
|
||||
Base() {
|
||||
// wrong: the virtual table of `Derived` has not been initialized yet. So this
|
||||
// call will resolve to `B::construct`, which cannot be called as it is a pure
|
||||
// virtual function.
|
||||
construct();
|
||||
}
|
||||
};
|
||||
|
||||
class Derived : public Base {
|
||||
int field;
|
||||
|
||||
void construct() override {
|
||||
field = 1;
|
||||
}
|
||||
};
|
||||
288
cpp/ql/src/Likely Bugs/OO/UnsafeUseOfThis.ql
Normal file
288
cpp/ql/src/Likely Bugs/OO/UnsafeUseOfThis.ql
Normal file
@@ -0,0 +1,288 @@
|
||||
/**
|
||||
* @name Unsafe use of this in constructor
|
||||
* @description A call to a pure virtual function using a this
|
||||
* pointer of an object that is under construction
|
||||
* may lead to undefined behavior.
|
||||
* @kind path-problem
|
||||
* @id cpp/unsafe-use-of-this
|
||||
* @problem.severity error
|
||||
* @precision high
|
||||
* @tags correctness
|
||||
* language-features
|
||||
* security
|
||||
*/
|
||||
|
||||
import semmle.code.cpp.ir.IR
|
||||
import cpp
|
||||
|
||||
predicate irBbFunctionExit(IRBlock exit) {
|
||||
exit.getLastInstruction() instanceof ExitFunctionInstruction
|
||||
}
|
||||
|
||||
predicate irBbNodePred(IRBlock src, IRBlock pred) { src.getAPredecessor() = pred }
|
||||
|
||||
predicate irBbIPostDominates(IRBlock postDominator, IRBlock node) =
|
||||
idominance(irBbFunctionExit/1, irBbNodePred/2)(_, postDominator, node)
|
||||
|
||||
predicate irBbStrictlyPostDominates(IRBlock postDominator, IRBlock node) {
|
||||
irBbIPostDominates+(postDominator, node)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `postDominator` is a post-dominator of `node` in the control-flow graph. This
|
||||
* is reflexive.
|
||||
*/
|
||||
predicate irBbPostDominates(IRBlock postDominator, IRBlock node) {
|
||||
irBbStrictlyPostDominates(postDominator, node) or postDominator = node
|
||||
}
|
||||
|
||||
bindingset[n, result]
|
||||
int unbind(int n) { result >= n and result <= n }
|
||||
|
||||
/** Holds if `p` is the `n`'th parameter of function `f`. */
|
||||
predicate parameterOf(Parameter p, Function f, int n) { p.getFunction() = f and p.getIndex() = n }
|
||||
|
||||
/**
|
||||
* Holds if `instr` is the `n`'th argument to a call to the function `f`, and
|
||||
* `init` is the corresponding initiazation instruction that receives the value of
|
||||
* `instr` in `f`.
|
||||
*/
|
||||
predicate flowIntoParameter(
|
||||
CallInstruction call, Instruction instr, Function f, int n, InitializeParameterInstruction init
|
||||
) {
|
||||
call.getPositionalArgument(n) = instr and
|
||||
f = call.getStaticCallTarget() and
|
||||
init.getEnclosingFunction() = f
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `instr` is an argument to a call to the function `f`, and
|
||||
* `init` is the corresponding initiazation instruction that receives the value of
|
||||
* `instr` in `f`.
|
||||
*/
|
||||
pragma[noinline]
|
||||
predicate getPositionalArgumentInitParam(
|
||||
CallInstruction call, Instruction instr, InitializeParameterInstruction init, Function f
|
||||
) {
|
||||
exists(int n |
|
||||
parameterOf(_, f, n) and
|
||||
flowIntoParameter(call, instr, f, unbind(n), init)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `instr` is the qualifier to a call to the function `f`, and
|
||||
* `init` is the corresponding initiazation instruction that receives the value of
|
||||
* `instr` in `f`.
|
||||
*/
|
||||
pragma[noinline]
|
||||
predicate getThisArgumentInitParam(
|
||||
CallInstruction call, Instruction instr, InitializeParameterInstruction init, Function f
|
||||
) {
|
||||
call.getStaticCallTarget() = f and
|
||||
init.getEnclosingFunction() = f and
|
||||
call.getThisArgument() = instr and
|
||||
init.getIRVariable() instanceof IRThisVariable
|
||||
}
|
||||
|
||||
/** Holds if `instr` is a `this` pointer of type `c` used by the call instruction `call`. */
|
||||
predicate isSink(Instruction instr, CallInstruction call, Class c) {
|
||||
exists(PureVirtualFunction func |
|
||||
call.getStaticCallTarget() = func and
|
||||
call.getThisArgument() = instr and
|
||||
instr.getResultType().stripType() = c and
|
||||
// Weed out implicit calls to destructors of a base class
|
||||
not func instanceof Destructor
|
||||
)
|
||||
}
|
||||
|
||||
/** Holds if `init` initializes the `this` pointer in class `c`. */
|
||||
predicate isSource(InitializeParameterInstruction init, string msg, Class c) {
|
||||
(
|
||||
exists(Constructor func |
|
||||
not func instanceof CopyConstructor and
|
||||
not func instanceof MoveConstructor and
|
||||
func = init.getEnclosingFunction() and
|
||||
msg = "construction"
|
||||
)
|
||||
or
|
||||
init.getEnclosingFunction() instanceof Destructor and msg = "destruction"
|
||||
) and
|
||||
init.getIRVariable() instanceof IRThisVariable and
|
||||
init.getEnclosingFunction().getDeclaringType() = c
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `instr` flows to the sink instruction `sink` that is a `this`
|
||||
* pointer of type `sinkClass`
|
||||
*/
|
||||
predicate flowsToSink(Instruction instr, Instruction sink, Class sinkClass) {
|
||||
instr = sink and
|
||||
isSink(sink, _, sinkClass)
|
||||
or
|
||||
exists(Instruction mid |
|
||||
successor(instr, mid, sinkClass) and
|
||||
flowsToSink(mid, sink, sinkClass)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `source` is an initialization of a `this` pointer of type `sourceClass`, and
|
||||
* `source` flows to instruction `instr`.
|
||||
*/
|
||||
predicate flowsFromSource(Instruction source, Instruction instr, Class sourceClass) {
|
||||
source = instr and
|
||||
isSource(source, _, sourceClass)
|
||||
or
|
||||
exists(Instruction mid |
|
||||
successorFwd(mid, instr, sourceClass) and
|
||||
flowsFromSource(source, mid, sourceClass)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if
|
||||
* - `instr` is an argument (or argument indirection) to a call, and
|
||||
* - `succ` is the corresponding initialization instruction in the call target, and
|
||||
* - `succ` eventually flows to an instruction that is used as a `this` pointer of type `sinkClass`.
|
||||
*/
|
||||
predicate flowThroughCallable(Instruction instr, Instruction succ, Class sinkClass) {
|
||||
flowsToSink(succ, _, sinkClass) and
|
||||
(
|
||||
// Flow from an argument to a parameter
|
||||
exists(CallInstruction call, InitializeParameterInstruction init | init = succ |
|
||||
getPositionalArgumentInitParam(call, instr, init, call.getStaticCallTarget())
|
||||
or
|
||||
getThisArgumentInitParam(call, instr, init, call.getStaticCallTarget())
|
||||
)
|
||||
or
|
||||
// Flow from argument indirection to parameter indirection
|
||||
exists(
|
||||
CallInstruction call, ReadSideEffectInstruction read, InitializeIndirectionInstruction init
|
||||
|
|
||||
init = succ and
|
||||
read.getPrimaryInstruction() = call and
|
||||
init.getEnclosingFunction() = call.getStaticCallTarget()
|
||||
|
|
||||
exists(int n |
|
||||
read.getSideEffectOperand().getAnyDef() = instr and
|
||||
read.getIndex() = n and
|
||||
init.getParameter().getIndex() = unbind(n)
|
||||
)
|
||||
or
|
||||
call.getThisArgument() = instr and
|
||||
init.getIRVariable() instanceof IRThisVariable
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `instr` flows to `succ` and `succ` eventually flows to an instruction that is used
|
||||
* as a `this` pointer of type `sinkClass`.
|
||||
*/
|
||||
predicate successor(Instruction instr, Instruction succ, Class sinkClass) {
|
||||
flowsToSink(succ, _, sinkClass) and
|
||||
(
|
||||
irBbPostDominates(succ.getBlock(), instr.getBlock()) and
|
||||
(
|
||||
(
|
||||
succ.(CopyInstruction).getSourceValue() = instr or
|
||||
succ.(CheckedConvertOrNullInstruction).getUnary() = instr or
|
||||
succ.(ChiInstruction).getTotal() = instr or
|
||||
succ.(ConvertInstruction).getUnary() = instr
|
||||
)
|
||||
or
|
||||
succ.(InheritanceConversionInstruction).getUnary() = instr
|
||||
)
|
||||
or
|
||||
flowThroughCallable(instr, succ, sinkClass)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if
|
||||
* - `instr` is an argument (or argument indirection) to a call, and
|
||||
* - `succ` is the corresponding initialization instruction in the call target, and
|
||||
* - there exists an initialization of a `this` pointer of type `sourceClass` that flows to `instr`.
|
||||
*/
|
||||
predicate flowThroughCallableFwd(Instruction instr, Instruction succ, Class sourceClass) {
|
||||
flowsFromSource(_, instr, sourceClass) and
|
||||
(
|
||||
// Flow from an argument to a parameter
|
||||
exists(CallInstruction call, InitializeParameterInstruction init | init = succ |
|
||||
getPositionalArgumentInitParam(call, instr, init, call.getStaticCallTarget())
|
||||
or
|
||||
getThisArgumentInitParam(call, instr, init, call.getStaticCallTarget())
|
||||
)
|
||||
or
|
||||
// Flow from argument indirection to parameter indirection
|
||||
exists(
|
||||
CallInstruction call, ReadSideEffectInstruction read, InitializeIndirectionInstruction init
|
||||
|
|
||||
init = succ and
|
||||
init.getEnclosingFunction() = call.getStaticCallTarget() and
|
||||
read.getPrimaryInstruction() = call and
|
||||
read.getSideEffectOperand().getAnyDef() = instr
|
||||
|
|
||||
exists(int n |
|
||||
read.getIndex() = n and
|
||||
init.getParameter().getIndex() = unbind(n)
|
||||
)
|
||||
or
|
||||
call.getThisArgument() = instr and
|
||||
init.getIRVariable() instanceof IRThisVariable
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if there exists an initialization of a `this` pointer of type `sourceClass` that flows
|
||||
* to `instr`, and `instr` flows to `succ` and `succ`.
|
||||
*/
|
||||
predicate successorFwd(Instruction instr, Instruction succ, Class sourceClass) {
|
||||
flowsFromSource(_, instr, sourceClass) and
|
||||
(
|
||||
irBbPostDominates(succ.getBlock(), instr.getBlock()) and
|
||||
(
|
||||
(
|
||||
succ.(CopyInstruction).getSourceValue() = instr or
|
||||
succ.(CheckedConvertOrNullInstruction).getUnary() = instr or
|
||||
succ.(ChiInstruction).getTotal() = instr or
|
||||
succ.(ConvertInstruction).getUnary() = instr
|
||||
)
|
||||
or
|
||||
succ.(InheritanceConversionInstruction).getUnary() = instr
|
||||
)
|
||||
or
|
||||
flowThroughCallableFwd(instr, succ, sourceClass)
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Holds if `instr` is in the path from an initialization of a `this` pointer in a subclass, to a use
|
||||
* of the `this` pointer in a baseclass.
|
||||
*/
|
||||
predicate isInPath(Instruction instr) {
|
||||
exists(Class sourceClass, Class sinkClass |
|
||||
flowsFromSource(_, instr, sourceClass) and
|
||||
flowsToSink(instr, _, sinkClass) and
|
||||
sourceClass.getABaseClass+() = sinkClass
|
||||
)
|
||||
}
|
||||
|
||||
query predicate edges(Instruction a, Instruction b) { successor(a, b, _) }
|
||||
|
||||
query predicate nodes(Instruction n, string key, string val) {
|
||||
isInPath(n) and key = "semmle.label" and val = n.toString()
|
||||
}
|
||||
|
||||
from
|
||||
Instruction source, Instruction sink, CallInstruction call, string msg, Class sourceClass,
|
||||
Class sinkClass
|
||||
where
|
||||
isSource(source, msg, sourceClass) and
|
||||
flowsToSink(source, sink, sinkClass) and
|
||||
isSink(sink, call, sinkClass) and
|
||||
sourceClass.getABaseClass+() = sinkClass
|
||||
select call.getUnconvertedResultExpression(), source, sink,
|
||||
"Call to pure virtual function during " + msg
|
||||
@@ -0,0 +1,47 @@
|
||||
edges
|
||||
| test.cpp:7:2:7:2 | InitializeParameter: B | test.cpp:8:10:8:13 | Load: this |
|
||||
| test.cpp:8:10:8:13 | Load: this | test.cpp:30:16:30:16 | InitializeParameter: x |
|
||||
| test.cpp:11:10:11:10 | InitializeParameter: b | test.cpp:12:9:12:9 | Load: b |
|
||||
| test.cpp:12:9:12:9 | CopyValue: (reference dereference) | test.cpp:12:9:12:9 | ConvertToNonVirtualBase: (A)... |
|
||||
| test.cpp:12:9:12:9 | Load: b | test.cpp:12:9:12:9 | CopyValue: (reference dereference) |
|
||||
| test.cpp:15:2:15:3 | InitializeParameter: ~B | test.cpp:16:3:16:3 | Load: this |
|
||||
| test.cpp:16:3:16:3 | Load: this | file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... |
|
||||
| test.cpp:21:2:21:2 | InitializeParameter: C | test.cpp:21:6:21:6 | ConvertToNonVirtualBase: call to B |
|
||||
| test.cpp:21:2:21:2 | InitializeParameter: C | test.cpp:22:10:22:13 | Load: this |
|
||||
| test.cpp:21:6:21:6 | ConvertToNonVirtualBase: call to B | test.cpp:7:2:7:2 | InitializeParameter: B |
|
||||
| test.cpp:22:10:22:13 | ConvertToNonVirtualBase: (B *)... | test.cpp:30:16:30:16 | InitializeParameter: x |
|
||||
| test.cpp:22:10:22:13 | Load: this | test.cpp:22:10:22:13 | ConvertToNonVirtualBase: (B *)... |
|
||||
| test.cpp:27:5:27:5 | InitializeParameter: D | test.cpp:27:14:27:17 | Load: this |
|
||||
| test.cpp:27:13:27:17 | ConvertToNonVirtualBase: (B)... | test.cpp:27:13:27:17 | CopyValue: (reference to) |
|
||||
| test.cpp:27:13:27:17 | CopyValue: (reference to) | test.cpp:11:10:11:10 | InitializeParameter: b |
|
||||
| test.cpp:27:13:27:17 | CopyValue: * ... | test.cpp:27:13:27:17 | ConvertToNonVirtualBase: (B)... |
|
||||
| test.cpp:27:14:27:17 | Load: this | test.cpp:27:13:27:17 | CopyValue: * ... |
|
||||
| test.cpp:30:16:30:16 | InitializeParameter: x | test.cpp:31:2:31:2 | Load: x |
|
||||
| test.cpp:31:2:31:2 | Load: x | test.cpp:31:2:31:2 | ConvertToNonVirtualBase: (A *)... |
|
||||
nodes
|
||||
| file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
|
||||
| test.cpp:7:2:7:2 | InitializeParameter: B | semmle.label | InitializeParameter: B |
|
||||
| test.cpp:8:10:8:13 | Load: this | semmle.label | Load: this |
|
||||
| test.cpp:11:10:11:10 | InitializeParameter: b | semmle.label | InitializeParameter: b |
|
||||
| test.cpp:12:9:12:9 | ConvertToNonVirtualBase: (A)... | semmle.label | ConvertToNonVirtualBase: (A)... |
|
||||
| test.cpp:12:9:12:9 | CopyValue: (reference dereference) | semmle.label | CopyValue: (reference dereference) |
|
||||
| test.cpp:12:9:12:9 | Load: b | semmle.label | Load: b |
|
||||
| test.cpp:15:2:15:3 | InitializeParameter: ~B | semmle.label | InitializeParameter: ~B |
|
||||
| test.cpp:16:3:16:3 | Load: this | semmle.label | Load: this |
|
||||
| test.cpp:21:2:21:2 | InitializeParameter: C | semmle.label | InitializeParameter: C |
|
||||
| test.cpp:21:6:21:6 | ConvertToNonVirtualBase: call to B | semmle.label | ConvertToNonVirtualBase: call to B |
|
||||
| test.cpp:22:10:22:13 | ConvertToNonVirtualBase: (B *)... | semmle.label | ConvertToNonVirtualBase: (B *)... |
|
||||
| test.cpp:22:10:22:13 | Load: this | semmle.label | Load: this |
|
||||
| test.cpp:27:5:27:5 | InitializeParameter: D | semmle.label | InitializeParameter: D |
|
||||
| test.cpp:27:13:27:17 | ConvertToNonVirtualBase: (B)... | semmle.label | ConvertToNonVirtualBase: (B)... |
|
||||
| test.cpp:27:13:27:17 | CopyValue: (reference to) | semmle.label | CopyValue: (reference to) |
|
||||
| test.cpp:27:13:27:17 | CopyValue: * ... | semmle.label | CopyValue: * ... |
|
||||
| test.cpp:27:14:27:17 | Load: this | semmle.label | Load: this |
|
||||
| test.cpp:30:16:30:16 | InitializeParameter: x | semmle.label | InitializeParameter: x |
|
||||
| test.cpp:31:2:31:2 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
|
||||
| test.cpp:31:2:31:2 | Load: x | semmle.label | Load: x |
|
||||
#select
|
||||
| test.cpp:12:11:12:11 | call to f | test.cpp:27:5:27:5 | InitializeParameter: D | test.cpp:12:9:12:9 | ConvertToNonVirtualBase: (A)... | Call to pure virtual function during construction |
|
||||
| test.cpp:16:3:16:3 | call to f | test.cpp:15:2:15:3 | InitializeParameter: ~B | file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during destruction |
|
||||
| test.cpp:31:5:31:5 | call to f | test.cpp:7:2:7:2 | InitializeParameter: B | test.cpp:31:2:31:2 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
|
||||
| test.cpp:31:5:31:5 | call to f | test.cpp:21:2:21:2 | InitializeParameter: C | test.cpp:31:2:31:2 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
|
||||
@@ -0,0 +1 @@
|
||||
Likely Bugs/OO/UnsafeUseOfThis.ql
|
||||
40
cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/test.cpp
Normal file
40
cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/test.cpp
Normal file
@@ -0,0 +1,40 @@
|
||||
struct A { virtual void f() = 0; };
|
||||
|
||||
struct B;
|
||||
void call_f(B*);
|
||||
|
||||
struct B : public A {
|
||||
B() {
|
||||
call_f(this);
|
||||
}
|
||||
|
||||
B(B& b) {
|
||||
b.f(); // BAD: undefined behavior
|
||||
}
|
||||
|
||||
~B() {
|
||||
f(); // BAD: undefined behavior
|
||||
}
|
||||
};
|
||||
|
||||
struct C : public B {
|
||||
C() {
|
||||
call_f(this);
|
||||
}
|
||||
};
|
||||
|
||||
struct D : public B {
|
||||
D() : B(*this) {}
|
||||
};
|
||||
|
||||
void call_f(B* x) {
|
||||
x->f(); // 2 x BAD: Undefined behavior
|
||||
}
|
||||
|
||||
struct E : public A {
|
||||
E() {
|
||||
f(); // GOOD: Will call `E::f`
|
||||
}
|
||||
|
||||
void f() override {}
|
||||
};
|
||||
Reference in New Issue
Block a user