Merge branch 'main' into atorralba/android-implicit-pending-intents

Author: atorralba

CODEOWNERS

@@ -27,4 +27,4 @@
 /docs/query-*-style-guide.md @github/codeql-analysis-reviewers
 
 # QL for QL reviewers
-/ql/ @erik-krogh @tausbn
+/ql/ @github/codeql-ql-for-ql-reviewers

CONTRIBUTING.md

@@ -4,6 +4,9 @@ We welcome contributions to our CodeQL libraries and queries. Got an idea for a
 
 There is lots of useful documentation to help you write queries, ranging from information about query file structure to tutorials for specific target languages. For more information on the documentation available, see [CodeQL queries](https://help.semmle.com/QL/learn-ql/writing-queries/writing-queries.html) on [help.semmle.com](https://help.semmle.com).
 
+## Change notes
+
+Any nontrivial user-visible change to a query pack or library pack should have a change note. For details on how to add a change note for your change, see [this guide](docs/change-notes.md).
 
 ## Submitting a new experimental query
 

cpp/ql/lib/CHANGELOG.md

@@ -1,3 +1,5 @@
+## 0.0.6
+
 ## 0.0.5
 
 ## 0.0.4

cpp/ql/lib/change-notes/2022-01-14-hex-format-range-analysis.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* `FormatLiteral::getMaxConvertedLength` now uses range analysis to provide a
+  more accurate length for integers formatted with `%x`

cpp/ql/lib/change-notes/released/0.0.6.md

@@ -0,0 +1 @@
+## 0.0.6

cpp/ql/lib/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.0.5
+lastReleaseVersion: 0.0.6

cpp/ql/lib/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/cpp-all
-version: 0.0.6-dev
+version: 0.0.7-dev
 groups: cpp
 dbscheme: semmlecode.cpp.dbscheme
 extractor: cpp

cpp/ql/lib/semmle/code/cpp/commons/Printf.qll

@@ -382,6 +382,9 @@ private int lengthInBase10(float f) {
   result = f.log10().floor() + 1
 }
 
+pragma[nomagic]
+private predicate isPointerTypeWithBase(Type base, PointerType pt) { base = pt.getBaseType() }
+
 bindingset[expr]
 private BufferWriteEstimationReason getEstimationReasonForIntegralExpression(Expr expr) {
   // we consider the range analysis non trivial if it
@@ -399,6 +402,18 @@ private BufferWriteEstimationReason getEstimationReasonForIntegralExpression(Exp
   else result = TTypeBoundsAnalysis()
 }
 
+/**
+ * Gets the number of hex digits required to represent the integer represented by `f`.
+ *
+ * `f` is assumed to be nonnegative.
+ */
+bindingset[f]
+private int lengthInBase16(float f) {
+  f = 0 and result = 1
+  or
+  result = (f.log2() / 4.0).floor() + 1
+}
+
 /**
  * A class to represent format strings that occur as arguments to invocations of formatting functions.
  */
@@ -950,19 +965,19 @@ class FormatLiteral extends Literal {
       (
         conv = ["s", "S"] and
         len = "h" and
-        result.(PointerType).getBaseType() instanceof PlainCharType
+        isPointerTypeWithBase(any(PlainCharType plainCharType), result)
         or
         conv = ["s", "S"] and
         len = ["l", "w"] and
-        result.(PointerType).getBaseType() = this.getWideCharType()
+        isPointerTypeWithBase(this.getWideCharType(), result)
         or
         conv = "s" and
         (len != "l" and len != "w" and len != "h") and
-        result.(PointerType).getBaseType() = this.getDefaultCharType()
+        isPointerTypeWithBase(this.getDefaultCharType(), result)
         or
         conv = "S" and
         (len != "l" and len != "w" and len != "h") and
-        result.(PointerType).getBaseType() = this.getNonDefaultCharType()
+        isPointerTypeWithBase(this.getNonDefaultCharType(), result)
       )
     )
   }
@@ -1253,23 +1268,42 @@ class FormatLiteral extends Literal {
         or
         this.getConversionChar(n).toLowerCase() = "x" and
         // e.g. "12345678"
-        exists(int sizeBytes, int baseLen |
-          sizeBytes =
-            min(int bytes |
-              bytes = this.getIntegralDisplayType(n).getSize()
+        exists(int baseLen, int typeBasedBound, int valueBasedBound |
+          typeBasedBound =
+            min(int digits |
+              digits = 2 * this.getIntegralDisplayType(n).getSize()
               or
               exists(IntegralType t |
                 t = this.getUse().getConversionArgument(n).getType().getUnderlyingType()
               |
-                t.isUnsigned() and bytes = t.getSize()
+                t.isUnsigned() and
+                digits = 2 * t.getSize()
               )
             ) and
-          baseLen = sizeBytes * 2 and
-          (
-            if this.hasAlternateFlag(n) then len = 2 + baseLen else len = baseLen // "0x"
-          )
-        ) and
-        reason = TTypeBoundsAnalysis()
+          exists(Expr arg, float lower, float upper, float typeLower, float typeUpper |
+            arg = this.getUse().getConversionArgument(n) and
+            lower = lowerBound(arg.getFullyConverted()) and
+            upper = upperBound(arg.getFullyConverted()) and
+            typeLower = exprMinVal(arg.getFullyConverted()) and
+            typeUpper = exprMaxVal(arg.getFullyConverted())
+          |
+            valueBasedBound =
+              lengthInBase16(max(float cand |
+                  // If lower can be negative we use `(unsigned)-1` as the candidate value.
+                  lower < 0 and
+                  cand = 2.pow(any(IntType t | t.isUnsigned()).getSize() * 8)
+                  or
+                  cand = upper
+                )) and
+            (
+              if lower > typeLower or upper < typeUpper
+              then reason = TValueFlowAnalysis()
+              else reason = TTypeBoundsAnalysis()
+            )
+          ) and
+          baseLen = valueBasedBound.minimum(typeBasedBound) and
+          if this.hasAlternateFlag(n) then len = 2 + baseLen else len = baseLen // "0x"
+        )
         or
         this.getConversionChar(n).toLowerCase() = "p" and
         exists(PointerType ptrType, int baseLen |

cpp/ql/src/CHANGELOG.md

@@ -1,3 +1,5 @@
+## 0.0.6
+
 ## 0.0.5
 
 ### New Queries

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql

@@ -22,11 +22,13 @@ import semmle.code.cpp.dataflow.DataFlow
  *  Holds if `sub` is guarded by a condition which ensures that
  * `left >= right`.
  */
-pragma[noinline]
+pragma[nomagic]
 predicate isGuarded(SubExpr sub, Expr left, Expr right) {
-  exists(GuardCondition guard, int k |
-    guard.controls(sub.getBasicBlock(), _) and
-    guard.ensuresLt(left, right, k, sub.getBasicBlock(), false) and
+  exprIsSubLeftOrLess(pragma[only_bind_into](sub), _) and // Manual magic
+  exists(GuardCondition guard, int k, BasicBlock bb |
+    pragma[only_bind_into](bb) = sub.getBasicBlock() and
+    guard.controls(pragma[only_bind_into](bb), _) and
+    guard.ensuresLt(left, right, k, bb, false) and
     k >= 0
   )
 }
@@ -36,47 +38,56 @@ predicate isGuarded(SubExpr sub, Expr left, Expr right) {
  * `sub.getLeftOperand()`.
  */
 predicate exprIsSubLeftOrLess(SubExpr sub, DataFlow::Node n) {
-  n.asExpr() = sub.getLeftOperand()
-  or
-  exists(DataFlow::Node other |
-    // dataflow
-    exprIsSubLeftOrLess(sub, other) and
-    (
-      DataFlow::localFlowStep(n, other) or
-      DataFlow::localFlowStep(other, n)
+  interestingSubExpr(sub, _) and // Manual magic
+  (
+    n.asExpr() = sub.getLeftOperand()
+    or
+    exists(DataFlow::Node other |
+      // dataflow
+      exprIsSubLeftOrLess(sub, other) and
+      (
+        DataFlow::localFlowStep(n, other) or
+        DataFlow::localFlowStep(other, n)
+      )
+    )
+    or
+    exists(DataFlow::Node other |
+      // guard constraining `sub`
+      exprIsSubLeftOrLess(sub, other) and
+      isGuarded(sub, other.asExpr(), n.asExpr()) // other >= n
+    )
+    or
+    exists(DataFlow::Node other, float p, float q |
+      // linear access of `other`
+      exprIsSubLeftOrLess(sub, other) and
+      linearAccess(n.asExpr(), other.asExpr(), p, q) and // n = p * other + q
+      p <= 1 and
+      q <= 0
+    )
+    or
+    exists(DataFlow::Node other, float p, float q |
+      // linear access of `n`
+      exprIsSubLeftOrLess(sub, other) and
+      linearAccess(other.asExpr(), n.asExpr(), p, q) and // other = p * n + q
+      p >= 1 and
+      q >= 0
     )
-  )
-  or
-  exists(DataFlow::Node other |
-    // guard constraining `sub`
-    exprIsSubLeftOrLess(sub, other) and
-    isGuarded(sub, other.asExpr(), n.asExpr()) // other >= n
-  )
-  or
-  exists(DataFlow::Node other, float p, float q |
-    // linear access of `other`
-    exprIsSubLeftOrLess(sub, other) and
-    linearAccess(n.asExpr(), other.asExpr(), p, q) and // n = p * other + q
-    p <= 1 and
-    q <= 0
-  )
-  or
-  exists(DataFlow::Node other, float p, float q |
-    // linear access of `n`
-    exprIsSubLeftOrLess(sub, other) and
-    linearAccess(other.asExpr(), n.asExpr(), p, q) and // other = p * n + q
-    p >= 1 and
-    q >= 0
   )
 }
 
-from RelationalOperation ro, SubExpr sub
-where
-  not isFromMacroDefinition(ro) and
+predicate interestingSubExpr(SubExpr sub, RelationalOperation ro) {
   not isFromMacroDefinition(sub) and
   ro.getLesserOperand().getValue().toInt() = 0 and
   ro.getGreaterOperand() = sub and
   sub.getFullyConverted().getUnspecifiedType().(IntegralType).isUnsigned() and
-  exprMightOverflowNegatively(sub.getFullyConverted()) and // generally catches false positives involving constants
-  not exprIsSubLeftOrLess(sub, DataFlow::exprNode(sub.getRightOperand())) // generally catches false positives where there's a relation between the left and right operands
+  // generally catches false positives involving constants
+  exprMightOverflowNegatively(sub.getFullyConverted())
+}
+
+from RelationalOperation ro, SubExpr sub
+where
+  interestingSubExpr(sub, ro) and
+  not isFromMacroDefinition(ro) and
+  // generally catches false positives where there's a relation between the left and right operands
+  not exprIsSubLeftOrLess(sub, DataFlow::exprNode(sub.getRightOperand()))
 select ro, "Unsigned subtraction can never be negative."