Merge pull request #4644 from MathiasVP/unsafe-use-of-this-query

C++: Add 'unsafe use of this' query
2026-05-05 05:35:13 +02:00 · 2020-11-25 08:40:12 +01:00
parent 48460e3e45 648acc3bfc
commit 5f0141953d
8 changed files with 377 additions and 0 deletions
--- a/Bugs/OO/UnsafeUseOfThis.cpp
+++ b/Bugs/OO/UnsafeUseOfThis.cpp
@@ -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 `Base::construct`, which cannot be called as it is a pure
+        // virtual function.
+        construct();
+    }
+};
+
+class Derived : public Base {
+    int field;
+
+    void construct() override {
+        field = 1;
+    }
+};
--- a/Bugs/OO/UnsafeUseOfThis.qhelp
+++ b/Bugs/OO/UnsafeUseOfThis.qhelp
@@ -0,0 +1,30 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>This rule finds calls to pure virtual member functions in constructors and destructors. When executing the body of a constructor of class <code>T</code>, the virtual table of <code>T</code> refers to the virtual table of one of <code>T</code>'s base classes. This can produce unexpected behavior, including program abort that can lead to denial of service attacks. The same problem exists during destruction of an object.</p>
+
+</overview>
+<recommendation>
+<p>Do not rely on virtual dispatch in constructors and destructors. Instead, each class should be responsible for acquiring and releasing its resources. If a base class needs to refer to a derived class during initialization, use the Dynamic Binding During Initialization idiom.</p>
+
+</recommendation>
+<example><sample src="UnsafeUseOfThis.cpp" />
+
+
+
+</example>
+<references>
+
+<li>ISO C++ FAQ: <a href="https://isocpp.org/wiki/faq/strange-inheritance#calling-virtuals-from-ctors">When my base class's constructor calls a virtual function on its this object, why doesn't my derived class's override of that virtual function get invoked?</a>
+</li>
+<li>SEI CERT C++ Coding Standard <a href="https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP50-CPP.+Do+not+invoke+virtual+functions+from+constructors+or+destructors">OOP50-CPP. Do not invoke virtual functions from constructors or destructors</a>
+</li>
+<li>ISO C++ FAQ: <a href="https://isocpp.org/wiki/faq/strange-inheritance#calling-virtuals-from-ctor-idiom">Okay, but is there a way to simulate that behavior as if dynamic binding worked on the this object within my base class's constructor?</a>
+</li>
+
+
+</references></qhelp>
--- a/Bugs/OO/UnsafeUseOfThis.ql
+++ b/Bugs/OO/UnsafeUseOfThis.ql
@@ -0,0 +1,212 @@
+/**
+ * @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 very-high
+ * @tags correctness
+ *       language-features
+ *       security
+ */
+
+import cpp
+// We don't actually use the global value numbering library in this query, but without it we end up
+// recomputing the IR.
+private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+private import semmle.code.cpp.ir.IR
+
+bindingset[n, result]
+int unbind(int n) { result >= n and result <= n }
+
+/** Holds if `p` is the `n`'th parameter of the non-virtual function `f`. */
+predicate parameterOf(Parameter p, Function f, int n) {
+  not f.isVirtual() and f.getParameter(n) = p
+}
+
+/**
+ * Holds if `instr` is the `n`'th argument to a call to the non-virtual 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
+) {
+  not f.isVirtual() and
+  call.getPositionalArgument(n) = instr and
+  f = call.getStaticCallTarget() and
+  getEnclosingNonVirtualFunctionInitializeParameter(init, f) and
+  init.getParameter().getIndex() = unbind(n)
+}
+
+/**
+ * Holds if `instr` is an argument to a call to the function `f`, and `init` is the
+ * corresponding initialization 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 non-virtual 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
+) {
+  not f.isVirtual() and
+  call.getStaticCallTarget() = f and
+  getEnclosingNonVirtualFunctionInitializeParameter(init, f) and
+  call.getThisArgument() = instr and
+  init.getIRVariable() instanceof IRThisVariable
+}
+
+/** Holds if `instr` is a `this` pointer used by the call instruction `call`. */
+predicate isSink(Instruction instr, CallInstruction call) {
+  exists(PureVirtualFunction func |
+    call.getStaticCallTarget() = func and
+    call.getThisArgument() = instr 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 a sink (which is a use of the value of `instr` as a `this` pointer).
+ */
+predicate flowsToSink(Instruction instr, Instruction sink) {
+  flowsFromSource(instr) and
+  (
+    isSink(instr, _) and instr = sink
+    or
+    exists(Instruction mid |
+      successor(instr, mid) and
+      flowsToSink(mid, sink)
+    )
+  )
+}
+
+/** Holds if `instr` flows from a source. */
+predicate flowsFromSource(Instruction instr) {
+  isSource(instr, _, _)
+  or
+  exists(Instruction mid |
+    successor(mid, instr) and
+    flowsFromSource(mid)
+  )
+}
+
+/** Holds if `f` is the enclosing non-virtual function of `init`. */
+predicate getEnclosingNonVirtualFunctionInitializeParameter(
+  InitializeParameterInstruction init, Function f
+) {
+  not f.isVirtual() and
+  init.getEnclosingFunction() = f
+}
+
+/** Holds if `f` is the enclosing non-virtual function of `init`. */
+predicate getEnclosingNonVirtualFunctionInitializeIndirection(
+  InitializeIndirectionInstruction init, Function f
+) {
+  not f.isVirtual() and
+  init.getEnclosingFunction() = f
+}
+
+/**
+ * Holds if `instr` is an argument (or argument indirection) to a call, and
+ * `succ` is the corresponding initialization instruction in the call target.
+ */
+predicate flowThroughCallable(Instruction instr, Instruction succ) {
+  // 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
+    getEnclosingNonVirtualFunctionInitializeIndirection(init, 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`. */
+predicate successor(Instruction instr, Instruction succ) {
+  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)
+}
+
+/**
+ * Holds if:
+ * - `source` is an initialization of a `this` pointer of type `sourceClass`, and
+ * - `sink` is a use of the `this` pointer, and
+ * - `call` invokes a pure virtual function using `sink` as the `this` pointer, and
+ * - `msg` is a string describing whether `source` is from a constructor or destructor.
+ */
+predicate flows(
+  Instruction source, string msg, Class sourceClass, Instruction sink, CallInstruction call
+) {
+  isSource(source, msg, sourceClass) and
+  flowsToSink(source, sink) and
+  isSink(sink, call)
+}
+
+query predicate edges(Instruction a, Instruction b) { successor(a, b) and flowsToSink(b, _) }
+
+query predicate nodes(Instruction n, string key, string val) {
+  flowsToSink(n, _) and
+  key = "semmle.label" and
+  val = n.toString()
+}
+
+from Instruction source, Instruction sink, CallInstruction call, string msg, Class sourceClass
+where
+  flows(source, msg, sourceClass, sink, call) and
+  // Only raise an alert if there is no override of the pure virtual function in any base class.
+  not exists(Class c | c = sourceClass.getABaseClass*() |
+    c.getAMemberFunction().getAnOverriddenFunction() = call.getStaticCallTarget()
+  )
+select call.getUnconvertedResultExpression(), source, sink,
+  "Call to pure virtual function during " + msg
--- a/cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/UnsafeUseOfThis.expected
+++ b/cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/UnsafeUseOfThis.expected
@@ -0,0 +1,61 @@
+edges
+| test.cpp:7:3:7:3 | InitializeParameter: B | test.cpp:8:12:8:15 | Load: this |
+| test.cpp:8:12:8:15 | Load: this | test.cpp:34:16:34:16 | InitializeParameter: x |
+| test.cpp:11:8:11:8 | InitializeParameter: b | test.cpp:12:5:12:5 | Load: b |
+| test.cpp:12:5:12:5 | CopyValue: (reference dereference) | test.cpp:12:5:12:5 | ConvertToNonVirtualBase: (A)... |
+| test.cpp:12:5:12:5 | Load: b | test.cpp:12:5:12:5 | CopyValue: (reference dereference) |
+| test.cpp:15:3:15:4 | InitializeParameter: ~B | test.cpp:16:5:16:5 | Load: this |
+| test.cpp:16:5:16:5 | Load: this | file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... |
+| test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:21:13:21:13 | ConvertToNonVirtualBase: call to B |
+| test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:22:12:22:15 | Load: this |
+| test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:25:7:25:10 | Load: this |
+| test.cpp:21:13:21:13 | ConvertToNonVirtualBase: call to B | test.cpp:7:3:7:3 | InitializeParameter: B |
+| test.cpp:22:12:22:15 | ConvertToNonVirtualBase: (B *)... | test.cpp:34:16:34:16 | InitializeParameter: x |
+| test.cpp:22:12:22:15 | Load: this | test.cpp:22:12:22:15 | ConvertToNonVirtualBase: (B *)... |
+| test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (B *)... | test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (A *)... |
+| test.cpp:25:7:25:10 | Load: this | test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (B *)... |
+| test.cpp:31:3:31:3 | InitializeParameter: D | test.cpp:31:12:31:15 | Load: this |
+| test.cpp:31:11:31:15 | ConvertToNonVirtualBase: (B)... | test.cpp:31:11:31:15 | CopyValue: (reference to) |
+| test.cpp:31:11:31:15 | CopyValue: (reference to) | test.cpp:11:8:11:8 | InitializeParameter: b |
+| test.cpp:31:11:31:15 | CopyValue: * ... | test.cpp:31:11:31:15 | ConvertToNonVirtualBase: (B)... |
+| test.cpp:31:12:31:15 | Load: this | test.cpp:31:11:31:15 | CopyValue: * ... |
+| test.cpp:34:16:34:16 | InitializeParameter: x | test.cpp:35:3:35:3 | Load: x |
+| test.cpp:35:3:35:3 | Load: x | test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... |
+| test.cpp:47:3:47:3 | InitializeParameter: F | test.cpp:48:10:48:13 | Load: this |
+| test.cpp:48:10:48:13 | ConvertToNonVirtualBase: (E *)... | test.cpp:48:6:48:13 | ConvertToNonVirtualBase: (A *)... |
+| test.cpp:48:10:48:13 | Load: this | test.cpp:48:10:48:13 | ConvertToNonVirtualBase: (E *)... |
+nodes
+| file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
+| test.cpp:7:3:7:3 | InitializeParameter: B | semmle.label | InitializeParameter: B |
+| test.cpp:8:12:8:15 | Load: this | semmle.label | Load: this |
+| test.cpp:11:8:11:8 | InitializeParameter: b | semmle.label | InitializeParameter: b |
+| test.cpp:12:5:12:5 | ConvertToNonVirtualBase: (A)... | semmle.label | ConvertToNonVirtualBase: (A)... |
+| test.cpp:12:5:12:5 | CopyValue: (reference dereference) | semmle.label | CopyValue: (reference dereference) |
+| test.cpp:12:5:12:5 | Load: b | semmle.label | Load: b |
+| test.cpp:15:3:15:4 | InitializeParameter: ~B | semmle.label | InitializeParameter: ~B |
+| test.cpp:16:5:16:5 | Load: this | semmle.label | Load: this |
+| test.cpp:21:3:21:3 | InitializeParameter: C | semmle.label | InitializeParameter: C |
+| test.cpp:21:13:21:13 | ConvertToNonVirtualBase: call to B | semmle.label | ConvertToNonVirtualBase: call to B |
+| test.cpp:22:12:22:15 | ConvertToNonVirtualBase: (B *)... | semmle.label | ConvertToNonVirtualBase: (B *)... |
+| test.cpp:22:12:22:15 | Load: this | semmle.label | Load: this |
+| test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
+| test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (B *)... | semmle.label | ConvertToNonVirtualBase: (B *)... |
+| test.cpp:25:7:25:10 | Load: this | semmle.label | Load: this |
+| test.cpp:31:3:31:3 | InitializeParameter: D | semmle.label | InitializeParameter: D |
+| test.cpp:31:11:31:15 | ConvertToNonVirtualBase: (B)... | semmle.label | ConvertToNonVirtualBase: (B)... |
+| test.cpp:31:11:31:15 | CopyValue: (reference to) | semmle.label | CopyValue: (reference to) |
+| test.cpp:31:11:31:15 | CopyValue: * ... | semmle.label | CopyValue: * ... |
+| test.cpp:31:12:31:15 | Load: this | semmle.label | Load: this |
+| test.cpp:34:16:34:16 | InitializeParameter: x | semmle.label | InitializeParameter: x |
+| test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
+| test.cpp:35:3:35:3 | Load: x | semmle.label | Load: x |
+| test.cpp:47:3:47:3 | InitializeParameter: F | semmle.label | InitializeParameter: F |
+| test.cpp:48:6:48:13 | ConvertToNonVirtualBase: (A *)... | semmle.label | ConvertToNonVirtualBase: (A *)... |
+| test.cpp:48:10:48:13 | ConvertToNonVirtualBase: (E *)... | semmle.label | ConvertToNonVirtualBase: (E *)... |
+| test.cpp:48:10:48:13 | Load: this | semmle.label | Load: this |
+#select
+| test.cpp:12:7:12:7 | call to f | test.cpp:31:3:31:3 | InitializeParameter: D | test.cpp:12:5:12:5 | ConvertToNonVirtualBase: (A)... | Call to pure virtual function during construction |
+| test.cpp:16:5:16:5 | call to f | test.cpp:15:3:15:4 | InitializeParameter: ~B | file://:0:0:0:0 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during destruction |
+| test.cpp:25:13:25:13 | call to f | test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:25:7:25:10 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
+| test.cpp:35:6:35:6 | call to f | test.cpp:7:3:7:3 | InitializeParameter: B | test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
+| test.cpp:35:6:35:6 | call to f | test.cpp:21:3:21:3 | InitializeParameter: C | test.cpp:35:3:35:3 | ConvertToNonVirtualBase: (A *)... | Call to pure virtual function during construction |
--- a/cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/UnsafeUseOfThis.qlref
+++ b/cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/UnsafeUseOfThis.qlref
@@ -0,0 +1 @@
+Likely Bugs/OO/UnsafeUseOfThis.ql
--- a/cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/test.cpp
+++ b/cpp/ql/test/query-tests/Critical/UnsafeUseOfThis/test.cpp
@@ -0,0 +1,50 @@
+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(bool b) {
+    call_f(this);
+
+    if(b) {
+      this->f(); // BAD: undefined behavior
+    }
+  }
+};
+
+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 {}
+};
+
+struct F : public E {
+  F() {
+    ((A*)this)->f(); // GOOD: Will call `E::f`
+  }
+};