Merge branch 'master' into ir-flow-fields

Author: MathiasVP

change-notes/1.24/analysis-cpp.md

@@ -25,6 +25,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
 | Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | Cases where the tainted allocation size is range checked are now more reliably excluded. |
+| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | The query now produces fewer, more accurate results. |
 | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |
 | Unsafe array for days of the year (`cpp/leap-year/unsafe-array-for-days-of-the-year`) |  | This query is no longer run on LGTM. |
 | Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | More correct results | This query now also looks for comparisons of the form `0 <= x`. |

cpp/ql/src/Critical/Negativity.qll

@@ -1,10 +1,19 @@
 import cpp
 
+/**
+ * Holds if `val` is an access to the variable `v`, or if `val`
+ * is an assignment with an access to `v` on the left-hand side.
+ */
 predicate valueOfVar(Variable v, Expr val) {
   val = v.getAnAccess() or
   val.(AssignExpr).getLValue() = v.getAnAccess()
 }
 
+/**
+ * Holds if either:
+ * - `cond` is an (in)equality expression that compares the variable `v` to the value `-1`, or
+ * - `cond` is a relational expression that compares the variable `v` to a constant.
+ */
 predicate boundsCheckExpr(Variable v, Expr cond) {
   exists(EQExpr eq |
     cond = eq and
@@ -43,6 +52,18 @@ predicate boundsCheckExpr(Variable v, Expr cond) {
   )
 }
 
+/**
+ * Holds if `node` is an expression in a conditional statement and `succ` is an
+ * immediate successor of `node` that may be reached after evaluating `node`.
+ * For example, given
+ * ```
+ * if (a < 10 && b) func1();
+ * else func2();
+ * ```
+ * this predicate holds when either:
+ * - `node` is `a < 10` and `succ` is `func2()` or `b`, or
+ * - `node` is `b` and `succ` is `func1()` or `func2()`
+ */
 predicate conditionalSuccessor(ControlFlowNode node, ControlFlowNode succ) {
   if node.isCondition()
   then succ = node.getATrueSuccessor() or succ = node.getAFalseSuccessor()
@@ -52,6 +73,12 @@ predicate conditionalSuccessor(ControlFlowNode node, ControlFlowNode succ) {
     )
 }
 
+/**
+ * Holds if the current value of the variable `v` at control-flow
+ * node `n` has been used either in:
+ * - an (in)equality comparison with the value `-1`, or
+ * - a relational comparison that compares `v` to a constant.
+ */
 predicate boundsChecked(Variable v, ControlFlowNode node) {
   exists(Expr test |
     boundsCheckExpr(v, test) and
@@ -63,6 +90,14 @@ predicate boundsChecked(Variable v, ControlFlowNode node) {
   )
 }
 
+/**
+ * Holds if `cond` compares `v` to some common error values. Specifically, this
+ * predicate holds when:
+ * - `cond` checks that `v` is equal to `-1`, or
+ * - `cond` checks that `v` is less than `0`, or
+ * - `cond` checks that `v` is less than or equal to `-1`, or
+ * - `cond` checks that `v` is not some common success value (see `successCondition`).
+ */
 predicate errorCondition(Variable v, Expr cond) {
   exists(EQExpr eq |
     cond = eq and
@@ -88,6 +123,14 @@ predicate errorCondition(Variable v, Expr cond) {
   )
 }
 
+/**
+ * Holds if `cond` compares `v` to some common success values. Specifically, this
+ * predicate holds when:
+ * - `cond` checks that `v` is not equal to `-1`, or
+ * - `cond` checks that `v` is greater than or equal than `0`, or
+ * - `cond` checks that `v` is greater than `-1`, or
+ * - `cond` checks that `v` is not some common error value (see `errorCondition`).
+ */
 predicate successCondition(Variable v, Expr cond) {
   exists(NEExpr ne |
     cond = ne and
@@ -113,6 +156,11 @@ predicate successCondition(Variable v, Expr cond) {
   )
 }
 
+/**
+ * Holds if there exists a comparison operation that checks whether `v`
+ * represents some common *error* values, and `n` may be reached
+ * immediately following the comparison operation.
+ */
 predicate errorSuccessor(Variable v, ControlFlowNode n) {
   exists(Expr cond |
     errorCondition(v, cond) and n = cond.getATrueSuccessor()
@@ -121,6 +169,11 @@ predicate errorSuccessor(Variable v, ControlFlowNode n) {
   )
 }
 
+/**
+ * Holds if there exists a comparison operation that checks whether `v`
+ * represents some common *success* values, and `n` may be reached
+ * immediately following the comparison operation.
+ */
 predicate successSuccessor(Variable v, ControlFlowNode n) {
   exists(Expr cond |
     successCondition(v, cond) and n = cond.getATrueSuccessor()
@@ -129,6 +182,10 @@ predicate successSuccessor(Variable v, ControlFlowNode n) {
   )
 }
 
+/**
+ * Holds if the current value of the variable `v` at control-flow node
+ * `n` may have been checked against a common set of *error* values.
+ */
 predicate checkedError(Variable v, ControlFlowNode n) {
   errorSuccessor(v, n)
   or
@@ -139,6 +196,10 @@ predicate checkedError(Variable v, ControlFlowNode n) {
   )
 }
 
+/**
+ * Holds if the current value of the variable `v` at control-flow node
+ * `n` may have been checked against a common set of *success* values.
+ */
 predicate checkedSuccess(Variable v, ControlFlowNode n) {
   successSuccessor(v, n)
   or

cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql

@@ -15,31 +15,31 @@ import cpp
 import semmle.code.cpp.security.TaintTracking
 import TaintedWithPath
 
-predicate taintedChild(Expr e, Expr tainted) {
-  (
-    isAllocationExpr(e)
-    or
-    any(MulExpr me | me.getAChild() instanceof SizeofOperator) = e
-  ) and
-  tainted = e.getAChild() and
+/**
+ * Holds if `alloc` is an allocation, and `tainted` is a child of it that is a
+ * taint sink.
+ */
+predicate allocSink(Expr alloc, Expr tainted) {
+  isAllocationExpr(alloc) and
+  tainted = alloc.getAChild() and
   tainted.getUnspecifiedType() instanceof IntegralType
 }
 
 class TaintedAllocationSizeConfiguration extends TaintTrackingConfiguration {
-  override predicate isSink(Element tainted) { taintedChild(_, tainted) }
+  override predicate isSink(Element tainted) { allocSink(_, tainted) }
 }
 
 predicate taintedAllocSize(
-  Expr e, Expr source, PathNode sourceNode, PathNode sinkNode, string taintCause
+  Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause
 ) {
   isUserInput(source, taintCause) and
   exists(Expr tainted |
-    taintedChild(e, tainted) and
+    allocSink(alloc, tainted) and
     taintedWithPath(source, tainted, sourceNode, sinkNode)
   )
 }
 
-from Expr e, Expr source, PathNode sourceNode, PathNode sinkNode, string taintCause
-where taintedAllocSize(e, source, sourceNode, sinkNode, taintCause)
-select e, sourceNode, sinkNode, "This allocation size is derived from $@ and might overflow",
+from Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause
+where taintedAllocSize(source, alloc, sourceNode, sinkNode, taintCause)
+select alloc, sourceNode, sinkNode, "This allocation size is derived from $@ and might overflow",
   source, "user input (" + taintCause + ")"

cpp/ql/src/semmle/code/cpp/Class.qll

@@ -458,6 +458,15 @@ class Class extends UserType {
     exists(ClassDerivation d | d.getDerivedClass() = this and d = result)
   }
 
+  /**
+   * Gets class derivation number `index` of this class/struct, for example the
+   * `public B` is derivation 1 in the following code:
+   * ```
+   * class D : public A, public B, public C {
+   *   ...
+   * };
+   * ```
+   */
   ClassDerivation getDerivation(int index) {
     exists(ClassDerivation d | d.getDerivedClass() = this and d.getIndex() = index and d = result)
   }
@@ -900,6 +909,22 @@ class AbstractClass extends Class {
 class TemplateClass extends Class {
   TemplateClass() { usertypes(underlyingElement(this), _, 6) }
 
+  /**
+   * Gets a class instantiated from this template.
+   *
+   * For example for `MyTemplateClass<T>` in the following code, the results are
+   * `MyTemplateClass<int>` and `MyTemplateClass<long>`:
+   * ```
+   * template<class T>
+   * class MyTemplateClass {
+   *   ...
+   * };
+   *
+   * MyTemplateClass<int> instance;
+   *
+   * MyTemplateClass<long> instance;
+   * ```
+   */
   Class getAnInstantiation() {
     result.isConstructedFrom(this) and
     exists(result.getATemplateArgument())

cpp/ql/src/semmle/code/cpp/Comments.qll

@@ -13,8 +13,20 @@ class Comment extends Locatable, @comment {
 
   override Location getLocation() { comments(underlyingElement(this), _, result) }
 
+  /**
+   * Gets the text of this comment, including the opening `//` or `/*`, and the closing `*``/` if
+   * present.
+   */
   string getContents() { comments(underlyingElement(this), result, _) }
 
+  /**
+   * Gets the AST element this comment is associated with. For example, the comment in the
+   * following code is associated with the declaration of `j`.
+   * ```
+   * int i;
+   * int j; // Comment on j
+   * ```
+   */
   Element getCommentedElement() {
     commentbinding(underlyingElement(this), unresolveElement(result))
   }

cpp/ql/src/semmle/code/cpp/Compilation.qll

@@ -40,6 +40,7 @@ class Compilation extends @compilation {
   /** Gets a file compiled during this invocation. */
   File getAFileCompiled() { result = getFileCompiled(_) }
 
+  /** Gets the `i`th file compiled during this invocation */
   File getFileCompiled(int i) { compilation_compiling_files(this, i, unresolveElement(result)) }
 
   /**

cpp/ql/src/semmle/code/cpp/Diagnostics.qll

@@ -11,6 +11,7 @@ class Diagnostic extends Locatable, @diagnostic {
   /** Gets the error code for this compiler message. */
   string getTag() { diagnostics(underlyingElement(this), _, result, _, _, _) }
 
+  /** Holds if `s` is the error code for this compiler message. */
   predicate hasTag(string s) { this.getTag() = s }
 
   /**