Merge pull request #986 from github/michaelrfairhurst/preconditions-rule-22-3-1-no-asserts-on-constants

MichaelRFairhurst · web-flow · commit 8071ddbeb9b6 · 2026-03-17T09:33:57.000Z
First implementation of Rule 22-3-1
diff --git a/c/cert/src/codeql-pack.lock.yml b/c/cert/src/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/c/cert/test/codeql-pack.lock.yml b/c/cert/test/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/c/common/src/codeql-pack.lock.yml b/c/common/src/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/c/common/test/codeql-pack.lock.yml b/c/common/test/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/c/misra/src/codeql-pack.lock.yml b/c/misra/src/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/c/misra/test/codeql-pack.lock.yml b/c/misra/test/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/cpp/autosar/src/codeql-pack.lock.yml b/cpp/autosar/src/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/cpp/autosar/test/codeql-pack.lock.yml b/cpp/autosar/test/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/cpp/cert/src/codeql-pack.lock.yml b/cpp/cert/src/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/cpp/cert/test/codeql-pack.lock.yml b/cpp/cert/test/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/cpp/common/src/codeql-pack.lock.yml b/cpp/common/src/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/cpp/common/src/codingstandards/cpp/ast/Conditions.qll b/cpp/common/src/codingstandards/cpp/ast/Conditions.qll
@@ -0,0 +1,29 @@
+import cpp
+import codeql.util.Boolean
+
+/**
+ * Any type of conditional evaluation, such as if statements, and conditional expressions.
+ *
+ * A condition may be:
+ * - An if statement condition
+ * - A conditional expression (ternary) condition
+ * - A short-circuiting logical expression (&& or ||)
+ */
+class Conditional extends Element {
+  Expr condition;
+
+  Conditional() {
+    condition = this.(ConditionalExpr).getCondition()
+    or
+    condition = this.(IfStmt).getCondition()
+    or
+    condition = this.(LogicalOrExpr).getLeftOperand()
+    or
+    condition = this.(LogicalAndExpr).getLeftOperand()
+  }
+
+  /**
+   * Get the expression that controls the flow of this conditional.
+   */
+  Expr getCondition() { result = condition }
+}
diff --git a/cpp/common/src/codingstandards/cpp/ast/Search.qll b/cpp/common/src/codingstandards/cpp/ast/Search.qll
@@ -0,0 +1,21 @@
+import cpp
+import qtil.Qtil
+
+module OutermostSearch<Qtil::Signature<Element>::Type Find> {
+  Find find(Element e) {
+    // Do not return a result if there are multiple siblings to different `Find`s.
+    result = unique(Find found | found = outermostSearchImpl(e))
+  }
+
+  private Find outermostSearchImpl(Element e) {
+    if e instanceof Find
+    then result = e
+    else (
+      result = outermostSearchImpl(e.(ExprStmt).getExpr())
+      or
+      result = outermostSearchImpl(e.(Stmt).getAChild())
+      or
+      result = outermostSearchImpl(e.(Expr).getAChild())
+    )
+  }
+}
diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/Preconditions3.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/Preconditions3.qll
@@ -0,0 +1,26 @@
+//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY.  **/
+import cpp
+import RuleMetadata
+import codingstandards.cpp.exclusions.RuleMetadata
+
+newtype Preconditions3Query = TAssertMacroUsedWithAConstantExpressionQuery()
+
+predicate isPreconditions3QueryMetadata(Query query, string queryId, string ruleId, string category) {
+  query =
+    // `Query` instance for the `assertMacroUsedWithAConstantExpression` query
+    Preconditions3Package::assertMacroUsedWithAConstantExpressionQuery() and
+  queryId =
+    // `@id` for the `assertMacroUsedWithAConstantExpression` query
+    "cpp/misra/assert-macro-used-with-a-constant-expression" and
+  ruleId = "RULE-22-3-1" and
+  category = "required"
+}
+
+module Preconditions3Package {
+  Query assertMacroUsedWithAConstantExpressionQuery() {
+    //autogenerate `Query` type
+    result =
+      // `Query` type for `assertMacroUsedWithAConstantExpression` query
+      TQueryCPP(TPreconditions3PackageQuery(TAssertMacroUsedWithAConstantExpressionQuery()))
+  }
+}
diff --git a/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll b/cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll
@@ -65,6 +65,7 @@ import OrderOfEvaluation
 import OutOfBounds
 import Pointers
 import Preconditions1
+import Preconditions3
 import Preconditions4
 import Preprocessor
 import Preprocessor2
@@ -153,6 +154,7 @@ newtype TCPPQuery =
   TOutOfBoundsPackageQuery(OutOfBoundsQuery q) or
   TPointersPackageQuery(PointersQuery q) or
   TPreconditions1PackageQuery(Preconditions1Query q) or
+  TPreconditions3PackageQuery(Preconditions3Query q) or
   TPreconditions4PackageQuery(Preconditions4Query q) or
   TPreprocessorPackageQuery(PreprocessorQuery q) or
   TPreprocessor2PackageQuery(Preprocessor2Query q) or
@@ -241,6 +243,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat
   isOutOfBoundsQueryMetadata(query, queryId, ruleId, category) or
   isPointersQueryMetadata(query, queryId, ruleId, category) or
   isPreconditions1QueryMetadata(query, queryId, ruleId, category) or
+  isPreconditions3QueryMetadata(query, queryId, ruleId, category) or
   isPreconditions4QueryMetadata(query, queryId, ruleId, category) or
   isPreprocessorQueryMetadata(query, queryId, ruleId, category) or
   isPreprocessor2QueryMetadata(query, queryId, ruleId, category) or
diff --git a/cpp/common/src/codingstandards/cpp/standardlibrary/Assert.qll b/cpp/common/src/codingstandards/cpp/standardlibrary/Assert.qll
@@ -0,0 +1,163 @@
+import cpp
+import codingstandards.cpp.ast.Conditions
+import codingstandards.cpp.ast.Search
+
+/**
+ * The assert macro.
+ */
+class AssertMacro extends Macro {
+  AssertMacro() { this.getName() = "assert" }
+}
+
+/**
+ * An assertion via the `assert` macro.
+ */
+class AssertInvocation extends MacroInvocation {
+  AssertInvocation() { this.getMacro() instanceof AssertMacro }
+
+  /**
+   * Get the expression that is being asserted, for example, `x > 0` in `assert(x > 0);`.
+   *
+   * Since the assert macro may expand to various forms of conditional statements, this method
+   * uses a combination of AST inspection and control flow analysis to determine the actual
+   * condition being asserted.
+   *
+   * For example, `assert(x)` may expand to `{ if (!__unlikely(x)) abort(); }`. In this case:
+   * - We first identify the "outermost conditional" generated by the macro, which is the `if`
+   *   statement, with the raw condition `!__unlikely(x)`.
+   * - We then analyze the control flow to see if the abort occurs when this raw condition is true
+   *   or false. In this case, it aborts when the raw condition `!__unlikely(x)` is true.
+   * - To unwrap the user provided condition `x` from the raw condition, we remove the compiler
+   *   intrinsics such as `__unlikely`.
+   * - Lastly, we also account for the negation. Since the program aborts on `!x`, we know we are
+   *   asserting `!x` is false, which is equivalent to asserting `x` is true.
+   *
+   * Note that the last two bullets are handled in either order, to support `!__unlikely(x)` or
+   * `__unlikely(!x)`.
+   */
+  Expr getAssertCondition() {
+    exists(Conditional conditional, Expr condition, Boolean isTrue |
+      // Get the outermost conditional in the assert body (if, &&, ||, ?:, etc).
+      conditional = OutermostSearch<Conditional>::find(this.getGeneratedBody()) and
+      condition = conditional.getCondition() and
+      // An assertion of form `assert(x)` may expand to a negated form, e.g. `if (!x) abort()`, or
+      // it may expand to a non-negated form e.g. `x || abort()`. We check whether the condition
+      // appears to abort when `condition` is true or false to distinguish these cases.
+      Asserts::appearsToAssert(conditional, isTrue) and
+      // If we seem to be asserting the condition is both true and false, give no result.
+      not Asserts::appearsToAssert(conditional, isTrue.booleanNot()) and
+      // Unwrap compiler inserted calls around the actual asserted condition such as `__unlikely`,
+      // and unwrap conditions such as `!x` if we found `appearsToAssert(conditional, false)`.
+      result = Asserts::unwrapAssertRawCondition(condition, isTrue.booleanNot())
+    )
+  }
+
+  /**
+   * Helper method to get the body whether it is a statement or an expression.
+   */
+  Element getGeneratedBody() { result = this.getStmt() or result = this.getExpr() }
+
+  /**
+   * Holds if this is an assertion of the form `assert(false)` or `assert(false && "message")`.
+   */
+  predicate isAssertFalse() {
+    exists(Expr assertCondition |
+      assertCondition = this.getAssertCondition() and
+      (
+        // plain `assert(false)`
+        assertCondition.(Literal).getValue() = "0"
+        or
+        // with literal, e.g. `assert(false && "message")`
+        exists(LogicalAndExpr lAnd |
+          lAnd = assertCondition and
+          lAnd.getLeftOperand().(Literal).getValue() = "0" and
+          lAnd.getRightOperand() instanceof StringLiteral
+        )
+      )
+    )
+  }
+}
+
+private module Asserts {
+  /**
+   * The outermost condition of an assert macro may not be the actual condition passed to assert, as
+   * the compiler may insert special calls like `__unlikely` or `__builtin_expect` around it. This
+   * function unwraps those calls to get to the actual condition.
+   *
+   * `negated` indicates whether negations were unwrapped. For example, `assert(x)` may expand
+   * to `if (!x) abort();`, so this predicate would hold for `(x, true)` and `(!x, false)`.
+   */
+  Expr unwrapAssertRawCondition(Expr e, Boolean negated) {
+    exists(Boolean inner_negated |
+      result = unwrapAssertRawCondition(e.(NotExpr).getOperand(), inner_negated) and
+      negated = inner_negated.booleanNot()
+    )
+    or
+    if e.(FunctionCall).getTarget().getName().matches("__%")
+    then result = unwrapAssertRawCondition(e.(FunctionCall).getArgument(0), negated)
+    else (
+      result = e and negated = false
+    )
+  }
+
+  /**
+   * Holds if the given conditional appears to assert its condition to be `isTrue`.
+   *
+   * For example, both `x || abort();` and `if (!x) abort();` effectively assert that `x`
+   * is true (in the second case, by asserting that `!x` is false).
+   */
+  predicate appearsToAssert(Conditional conditional, Boolean isTrue) {
+    // Check if an aborting side is reachable via the given boolean value of the condition.
+    sideAborts(conditional, _, isTrue.booleanNot())
+    or
+    // If the condition is a constant value, then we may not be able to reach the side that aborts
+    // via control flow. Detect such cases here.
+    not sideAborts(conditional, _, _) and
+    (
+      // If the false side is unreachable, we presume the false side aborts when reachable
+      not exists(conditional.getCondition().getAFalseSuccessor()) and
+      // Therefore this asserts the condition is true
+      isTrue = true
+      or
+      // If the true side is unreachable, we presume the true side aborts when reachable
+      not exists(conditional.getCondition().getATrueSuccessor()) and
+      // Therefore this asserts the condition is false
+      isTrue = false
+    )
+  }
+
+  /**
+   * Holds if a control flow node contained by the `conditional` ast node appears to abort the
+   * program when the condition evaluates to `isTrue`.
+   */
+  predicate sideAborts(Conditional conditional, Expr abort, Boolean isTrue) {
+    abort.getEnclosingElement*() = conditional and
+    appearsToAbort(abort) and
+    exists(ControlFlowNode branch |
+      branch.getASuccessor*() = abort and
+      (
+        isTrue = true and
+        branch = conditional.getCondition().(ControlFlowNode).getATrueSuccessor()
+        or
+        isTrue = false and
+        branch = conditional.getCondition().(ControlFlowNode).getAFalseSuccessor()
+      )
+    )
+  }
+
+  /**
+   * Holds if a control flow node appears to abort the program.
+   *
+   * We currently detect:
+   * - Calls to functions with "fail" or "abort" in their name.
+   * - Calls to functions beginning with "__" and containing "assert" in their name.
+   * - Nodes with no successors (e.g. a call to `std::terminate` or a throw statement).
+   */
+  predicate appearsToAbort(ControlFlowNode node) {
+    node.(FunctionCall).getTarget().getName().matches(["%fail%", "%abort%", "__%assert"])
+    or
+    not exists(node.getASuccessor()) and not node instanceof Conversion
+  }
+}
+
+import Asserts
diff --git a/cpp/common/src/qlpack.yml b/cpp/common/src/qlpack.yml
@@ -3,6 +3,7 @@ version: 2.56.0-dev
 license: MIT
 dependencies:
   codeql/cpp-all: 5.0.0
+  advanced-security/qtil: "0.0.3"
 dataExtensions:
 - ext/*.model.yml
 - ext/unicode.yml
diff --git a/cpp/common/test/codeql-pack.lock.yml b/cpp/common/test/codeql-pack.lock.yml
@@ -1,6 +1,8 @@
 ---
 lockVersion: 1.0.0
 dependencies:
+  advanced-security/qtil:
+    version: 0.0.3
   codeql/cpp-all:
     version: 5.0.0
   codeql/dataflow:
diff --git a/cpp/common/test/includes/standard-library/assert.h b/cpp/common/test/includes/standard-library/assert.h
@@ -1,6 +1,13 @@
 #ifndef _GHLIBCPP_ASSERT
 #define _GHLIBCPP_ASSERT
 
-#define assert(x) (void)0
+#include <stdlib.h>
+#include <stdio.h>
+
+#define __assert(e, file, line) \
+	((void)printf ("%s:%d: failed assertion `%s'\n", file, line, e), abort())
+
+#define assert(e)  \
+   { (__builtin_expect(!(e), 0) ? __assert (#e, __FILE__, __LINE__) : (void)0); }
 
 #endif // _GHLIBCPP_ASSERT
diff --git a/cpp/common/test/library/codingstandards/cpp/standardlibrary/Assert/AssertTest.expected b/cpp/common/test/library/codingstandards/cpp/standardlibrary/Assert/AssertTest.expected
diff --git a/cpp/common/test/library/codingstandards/cpp/standardlibrary/Assert/AssertTest.ql b/cpp/common/test/library/codingstandards/cpp/standardlibrary/Assert/AssertTest.ql
diff --git a/cpp/common/test/library/codingstandards/cpp/standardlibrary/Assert/test.cpp b/cpp/common/test/library/codingstandards/cpp/standardlibrary/Assert/test.cpp
diff --git a/cpp/misra/src/rules/RULE-22-3-1/AssertMacroUsedWithAConstantExpression.ql b/cpp/misra/src/rules/RULE-22-3-1/AssertMacroUsedWithAConstantExpression.ql
diff --git a/cpp/misra/test/rules/RULE-22-3-1/AssertMacroUsedWithAConstantExpression.expected b/cpp/misra/test/rules/RULE-22-3-1/AssertMacroUsedWithAConstantExpression.expected
diff --git a/cpp/misra/test/rules/RULE-22-3-1/AssertMacroUsedWithAConstantExpression.qlref b/cpp/misra/test/rules/RULE-22-3-1/AssertMacroUsedWithAConstantExpression.qlref
diff --git a/cpp/misra/test/rules/RULE-22-3-1/test.cpp b/cpp/misra/test/rules/RULE-22-3-1/test.cpp
diff --git a/rule_packages/cpp/Preconditions3.json b/rule_packages/cpp/Preconditions3.json
diff --git a/rules.csv b/rules.csv
