Merge branch 'main' into cpp/comma-before-misleading-indentation

cpp/ql/src/Best Practices/Hiding/LocalVariableHidesGlobalVariable.ql

@@ -35,4 +35,4 @@ from LocalVariableOrParameter lv, GlobalVariable gv
 where
   lv.getName() = gv.getName() and
   lv.getFile() = gv.getFile()
-select lv, lv.type() + gv.getName() + " hides $@ with the same name.", gv, "a global variable"
+select lv, lv.type() + gv.getName() + " hides a $@ with the same name.", gv, "global variable"

cpp/ql/src/Best Practices/Unused Entities/UnusedStaticVariables.qhelp

@@ -11,7 +11,7 @@ caused by an unhandled case.</p>
 
 </overview>
 <recommendation>
-<p>Check that the unused static variable does not indicate a defect, for example, an unhandled case. If the static variable is genuinuely not needed, 
+<p>Check that the unused static variable does not indicate a defect, for example, an unhandled case. If the static variable is genuinely not needed, 
 then removing it will make code more readable. If the static variable is needed then you should update the code to fix the defect.</p>
 
 </recommendation>

cpp/ql/src/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 0.4.1
+
+### Minor Analysis Improvements
+
+* The alert message of many queries have been changed to better follow the style guide and make the message consistent with other languages.
+
 ## 0.4.0
 
 ### New Queries

cpp/ql/src/Critical/DescriptorMayNotBeClosed.qhelp

@@ -19,7 +19,7 @@ This can occur when an operation performed on the open descriptor fails, and the
 
 <example>
 <p>In the example below, the <code>sockfd</code> socket may remain open if an error is triggered. 
-The code should be updated to ensure that the socket is always closed when when the function ends.
+The code should be updated to ensure that the socket is always closed when the function ends.
 </p>
 <sample src="DescriptorMayNotBeClosed.cpp" />
 </example>

cpp/ql/src/Diagnostics/SuccessfullyExtractedFiles.ql

@@ -3,6 +3,7 @@
  * @description Lists all files in the source code directory that were extracted without encountering a problem in the file.
  * @kind diagnostic
  * @id cpp/diagnostics/successfully-extracted-files
+ * @tags successfully-extracted-files
  */
 
 import cpp

cpp/ql/src/Documentation/DocumentApi.qhelp

@@ -15,7 +15,7 @@ As an exception, because their purpose is usually obvious, it is not necessary t
 </overview>
 <recommendation>
 <p>
-Add comments to document the purpose of the function. In particular, ensure that the public API of the function is carefully documented. This reduces the chance that a future change to the function will introduce a defect by changing the API and breaking the expections of the calling functions.
+Add comments to document the purpose of the function. In particular, ensure that the public API of the function is carefully documented. This reduces the chance that a future change to the function will introduce a defect by changing the API and breaking the expectations of the calling functions.
 </p>
 
 </recommendation>

cpp/ql/src/Likely Bugs/Arithmetic/ComparisonPrecedence.qhelp

@@ -6,7 +6,7 @@
 
 <overview>
 <p>
-This rule finds comparison expressions that use 2 or more comparison operators and are not completely paranthesized. 
+This rule finds comparison expressions that use 2 or more comparison operators and are not completely parenthesized. 
 It is best to fully parenthesize complex comparison expressions to explicitly define the order of the comparison operators.
 </p> 
 

cpp/ql/src/Likely Bugs/Conversion/CastArrayPointerArithmetic.ql

@@ -25,8 +25,11 @@ class CastToPointerArithFlow extends DataFlow::Configuration {
 
   override predicate isSource(DataFlow::Node node) {
     not node.asExpr() instanceof Conversion and
-    introducesNewField(node.asExpr().getType().(DerivedType).getBaseType(),
-      node.asExpr().getConversion*().getType().(DerivedType).getBaseType())
+    exists(Type baseType1, Type baseType2 |
+      hasBaseType(node.asExpr(), baseType1) and
+      hasBaseType(node.asExpr().getConversion*(), baseType2) and
+      introducesNewField(baseType1, baseType2)
+    )
   }
 
   override predicate isSink(DataFlow::Node node) {
@@ -35,6 +38,17 @@ class CastToPointerArithFlow extends DataFlow::Configuration {
   }
 }
 
+/**
+ * Holds if the type of `e` is a `DerivedType` with `base` as its base type.
+ *
+ * This predicate ensures that joins go from `e` to `base` instead
+ * of the other way around.
+ */
+pragma[inline]
+predicate hasBaseType(Expr e, Type base) {
+  pragma[only_bind_into](base) = e.getType().(DerivedType).getBaseType()
+}
+
 /**
  * `derived` has a (possibly indirect) base class of `base`, and at least one new
  * field has been introduced in the inheritance chain after `base`.

cpp/ql/src/Likely Bugs/Likely Typos/IncorrectNotOperatorUsage.qhelp

@@ -6,9 +6,9 @@
 <overview>
 <p>This rule finds logical-not operator usage as an operator for in a bit-wise operation.</p>
 
-<p>Due to the nature of logical operation result value, only the lowest bit could possibly be set, and it is unlikely to be intent in bitwise opeartions. Violations are often indicative of a typo, using a logical-not (<code>!</code>) opeartor instead of the bit-wise not (<code>~</code>) operator. </p>
+<p>Due to the nature of logical operation result value, only the lowest bit could possibly be set, and it is unlikely to be intent in bitwise operations. Violations are often indicative of a typo, using a logical-not (<code>!</code>) operator instead of the bit-wise not (<code>~</code>) operator. </p>
 <p>This rule is restricted to analyze bit-wise and (<code>&amp;</code>) and bit-wise or (<code>|</code>) operation in order to provide better precision.</p>
-<p>This rule ignores instances where a double negation (<code>!!</code>) is explicitly used as the opeartor of the bitwise operation, as this is a commonly used as a mechanism to normalize an integer value to either 1 or 0.</p>
+<p>This rule ignores instances where a double negation (<code>!!</code>) is explicitly used as the operator of the bitwise operation, as this is a commonly used as a mechanism to normalize an integer value to either 1 or 0.</p>
 <p>NOTE: It is not recommended to use this rule in kernel code or older C code as it will likely find several false positive instances.</p>
 
 </overview>

cpp/ql/src/Likely Bugs/Memory Management/PointerOverflow.qhelp

@@ -49,7 +49,7 @@ pointer overflow.
 
 <p>
 While it's not the subject of this query, the expression <code>ptr + i &lt;
-ptr_end</code> is also an invalid range check. It's undefined behavor in
+ptr_end</code> is also an invalid range check. It's undefined behavior in
 C/C++ to create a pointer that points more than one past the end of an
 allocation.
 </p>

cpp/ql/src/Likely Bugs/Memory Management/SuspiciousCallToStrncat.qhelp

@@ -12,7 +12,7 @@ the third argument to the entire size of the destination buffer.
 Executing a call of this type may cause a buffer overflow unless the buffer is known to be empty.</p>
 
 <p>Similarly, calls of the form <code>strncat(dest, src, sizeof (dest) - strlen (dest))</code> allow one
-byte to be written ouside the <code>dest</code> buffer.</p>
+byte to be written outside the <code>dest</code> buffer.</p>
 
 <p>Buffer overflows can lead to anything from a segmentation fault to a security vulnerability.</p>
 

cpp/ql/src/Likely Bugs/ShortLoopVarName.ql

@@ -48,5 +48,5 @@ where
   not coordinatePair(iterationVar, innerVar)
 select iterationVar,
   "Iteration variable " + iterationVar.getName() +
-    " for $@ should have a descriptive name, since there is $@.", outer, "this loop", inner,
-  "a nested loop"
+    " for $@ should have a descriptive name, since there is a $@.", outer, "this loop", inner,
+  "nested loop"

cpp/ql/src/Metrics/Classes/CLackOfCohesionCK.qhelp

@@ -6,7 +6,7 @@
 <p>
 This metric provides an indication of the lack of cohesion of a class, 
 using a method proposed by Chidamber and Kemerer in 1994. The idea
-behind measuring a class's cohesion is that most funcions in well-designed
+behind measuring a class's cohesion is that most functions in well-designed
 classes will access the same fields. Types that exhibit a lack of cohesion
 are often trying to take on multiple responsibilities, and should be split
 into several smaller classes.

cpp/ql/src/Metrics/Namespaces/StableNamespaces.qhelp

@@ -11,7 +11,7 @@
       by changes to other packages. If this metric value is high, a package is easily 
       influenced. If the values is low, the impact of changes to other packages is likely to be minimal. Instability 
       is estimated as the number of outgoing dependencies relative to the total
-      number of depencies.</p>
+      number of dependencies.</p>
 </overview>
 
 <references>

cpp/ql/src/Metrics/Namespaces/UnstableNamespaces.qhelp

@@ -11,7 +11,7 @@
       by changes to other packages. If this metric value is high, a package is easily 
       influenced. If the values is low, the impact of changes to other packages is likely to be minimal. Instability 
       is estimated as the number of outgoing dependencies relative to the total
-      number of depencies.</p>
+      number of dependencies.</p>
 </overview>
 
 <references>

cpp/ql/src/Security/CWE/CWE-121/UnterminatedVarargsCall.ql

@@ -56,29 +56,26 @@ class VarargsFunction extends Function {
     result = strictcount(FunctionCall fc | fc = this.getACallToThisFunction())
   }
 
-  string normalTerminator(int cnt) {
+  string normalTerminator(int cnt, int totalCount) {
+    // the terminator is 0 or -1
     result = ["0", "-1"] and
+    // at least 80% of calls have the terminator
     cnt = this.trailingArgValueCount(result) and
-    2 * cnt > this.totalCount() and
-    not exists(FunctionCall fc, int index |
-      // terminator value is used in a non-terminating position
-      this.nonTrailingVarArgValue(fc, index) = result
-    )
+    totalCount = this.totalCount() and
+    100 * cnt / totalCount >= 80 and
+    // terminator value is not used in a non-terminating position
+    not exists(FunctionCall fc, int index | this.nonTrailingVarArgValue(fc, index) = result)
   }
 
-  predicate isWhitelisted() {
-    this.hasGlobalName("open") or
-    this.hasGlobalName("fcntl") or
-    this.hasGlobalName("ptrace")
-  }
+  predicate isWhitelisted() { this.hasGlobalName(["open", "fcntl", "ptrace", "mremap"]) }
 }
 
-from VarargsFunction f, FunctionCall fc, string terminator, int cnt
+from VarargsFunction f, FunctionCall fc, string terminator, int cnt, int totalCount
 where
-  terminator = f.normalTerminator(cnt) and
+  terminator = f.normalTerminator(cnt, totalCount) and
   fc = f.getACallToThisFunction() and
   not normalisedExprValue(f.trailingArgumentIn(fc)) = terminator and
   not f.isWhitelisted()
 select fc,
-  "Calls to $@ should use the value " + terminator + " as a terminator (" + cnt + " calls do).", f,
-  f.getQualifiedName()
+  "Calls to $@ should use the value " + terminator + " as a terminator (" + cnt + " of " +
+    totalCount + " calls do).", f, f.getQualifiedName()

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

@@ -135,5 +135,5 @@ where
   sink.getNode().asExpr() = va and
   missingGuard(va, effect)
 select sink.getNode(), source, sink,
-  "Arithmetic expression depends on an $@, potentially causing an " + effect + ".",
+  "This arithmetic expression depends on an $@, potentially causing an " + effect + ".",
   getExpr(source.getNode()), "uncontrolled value"

cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.qhelp

@@ -5,7 +5,7 @@
 
 <overview>
   <p>This query indicates that a call is setting the DACL field in a <code>SECURITY_DESCRIPTOR</code> to null.</p>
-  <p>When using <code>SetSecurityDescriptorDacl</code> to set a discretionary access control (DACL), setting the <code>bDaclPresent</code> argument to <code>TRUE</code> indicates the prescence of a DACL in the security description in the argument <code>pDacl</code>.</p>
+  <p>When using <code>SetSecurityDescriptorDacl</code> to set a discretionary access control (DACL), setting the <code>bDaclPresent</code> argument to <code>TRUE</code> indicates the presence of a DACL in the security description in the argument <code>pDacl</code>.</p>
   <p>When the <code>pDacl</code> parameter does not point to a DACL (i.e. it is <code>NULL</code>) and the <code>bDaclPresent</code> flag is <code>TRUE</code>, a <code>NULL DACL</code> is specified.</p>
   <p>A <code>NULL DACL</code> grants full access to any user who requests it; normal security checking is not performed with respect to the object.</p>
 </overview>

cpp/ql/src/change-notes/2022-10-06-unterminated-variadic-call.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Unterminated variadic call" (`cpp/unterminated-variadic-call`) query has been tuned to produce fewer false positive results.

cpp/ql/src/change-notes/released/0.4.1.md

@@ -1,4 +1,5 @@
----
-category: minorAnalysis
----
-* The alert message of many queries have been changed to better follow the style guide and make the message consistent with other languages.
+## 0.4.1
+
+### Minor Analysis Improvements
+
+* The alert message of many queries have been changed to better follow the style guide and make the message consistent with other languages.

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

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.4.0
+lastReleaseVersion: 0.4.1

cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.cpp

@@ -0,0 +1,17 @@
+#define MAX_SIZE 1024
+
+struct FixedArray {
+  int buf[MAX_SIZE];
+};
+
+int main(){
+  FixedArray arr;
+
+  for(int i = 0; i <= MAX_SIZE; i++) {
+    arr.buf[i] = 0; // BAD
+  }
+
+  for(int i = 0; i < MAX_SIZE; i++) {
+    arr.buf[i] = 0; // GOOD
+  }
+}

cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The program performs an out-of-bounds read or write operation. In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code.</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that pointer dereferences are properly guarded to ensure that they cannot be used to read or write past the end of the allocation.</p>
+
+</recommendation>
+<example>
+<p>The first example uses a for loop which is improperly bounded by a non-strict less-than operation and will write one position past the end of the array. The second example bounds the for loop properly with a strict less-than operation.</p>
+<sample src="ConstantSizeArrayOffByOne.cpp" />
+
+</example>
+<references>
+
+<li>CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts">ARR30-C. Do not form or use out-of-bounds pointers or array subscripts</a>.</li>
+<li>
+OWASP:
+<a href="https://owasp.org/www-community/vulnerabilities/Buffer_Overflow">Buffer Overflow</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-193/ConstantSizeArrayOffByOne.ql

@@ -0,0 +1,107 @@
+/**
+ * @name Constant array overflow
+ * @description Dereferencing a pointer that points past a statically-sized array is undefined behavior
+ *              and may lead to security vulnerabilities
+ * @kind path-problem
+ * @problem.severity error
+ * @id cpp/constant-array-overflow
+ * @tags reliability
+ *       security
+ */
+
+import experimental.semmle.code.cpp.semantic.analysis.RangeAnalysis
+import experimental.semmle.code.cpp.semantic.SemanticBound
+import experimental.semmle.code.cpp.semantic.SemanticExprSpecific
+import semmle.code.cpp.ir.IR
+import experimental.semmle.code.cpp.ir.dataflow.DataFlow
+import experimental.semmle.code.cpp.ir.dataflow.DataFlow2
+import DataFlow2::PathGraph
+
+pragma[nomagic]
+Instruction getABoundIn(SemBound b, IRFunction func) {
+  result = b.getExpr(0) and
+  result.getEnclosingIRFunction() = func
+}
+
+/**
+ * Holds if `i <= b + delta`.
+ */
+pragma[nomagic]
+predicate bounded(Instruction i, Instruction b, int delta) {
+  exists(SemBound bound, IRFunction func |
+    semBounded(getSemanticExpr(i), bound, delta, true, _) and
+    b = getABoundIn(bound, func) and
+    i.getEnclosingIRFunction() = func
+  )
+}
+
+class FieldAddressToPointerArithmeticConf extends DataFlow::Configuration {
+  FieldAddressToPointerArithmeticConf() { this = "FieldAddressToPointerArithmeticConf" }
+
+  override predicate isSource(DataFlow::Node source) { isFieldAddressSource(_, source) }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(PointerAddInstruction pai | pai.getLeft() = sink.asInstruction())
+  }
+}
+
+predicate isFieldAddressSource(Field f, DataFlow::Node source) {
+  source.asInstruction().(FieldAddressInstruction).getField() = f
+}
+
+/**
+ * Holds if `sink` is a sink for `InvalidPointerToDerefConf` and `i` is a `StoreInstruction` that
+ * writes to an address that non-strictly upper-bounds `sink`, or `i` is a `LoadInstruction` that
+ * reads from an address that non-strictly upper-bounds `sink`.
+ */
+predicate isInvalidPointerDerefSink(DataFlow::Node sink, Instruction i, string operation) {
+  exists(AddressOperand addr, int delta |
+    bounded(addr.getDef(), sink.asInstruction(), delta) and
+    delta >= 0 and
+    i.getAnOperand() = addr
+  |
+    i instanceof StoreInstruction and
+    operation = "write"
+    or
+    i instanceof LoadInstruction and
+    operation = "read"
+  )
+}
+
+predicate isConstantSizeOverflowSource(Field f, PointerAddInstruction pai, int delta) {
+  exists(
+    int size, int bound, FieldAddressToPointerArithmeticConf conf, DataFlow::Node source,
+    DataFlow::InstructionNode sink
+  |
+    conf.hasFlow(source, sink) and
+    isFieldAddressSource(f, source) and
+    pai.getLeft() = sink.asInstruction() and
+    f.getUnspecifiedType().(ArrayType).getArraySize() = size and
+    semBounded(getSemanticExpr(pai.getRight()), any(SemZeroBound b), bound, true, _) and
+    delta = bound - size and
+    delta >= 0 and
+    size != 0 and
+    size != 1
+  )
+}
+
+class PointerArithmeticToDerefConf extends DataFlow2::Configuration {
+  PointerArithmeticToDerefConf() { this = "PointerArithmeticToDerefConf" }
+
+  override predicate isSource(DataFlow::Node source) {
+    isConstantSizeOverflowSource(_, source.asInstruction(), _)
+  }
+
+  override predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink(sink, _, _) }
+}
+
+from
+  Field f, DataFlow2::PathNode source, DataFlow2::PathNode sink, Instruction deref,
+  PointerArithmeticToDerefConf conf, string operation, int delta
+where
+  conf.hasFlowPath(source, sink) and
+  isInvalidPointerDerefSink(sink.getNode(), deref, operation) and
+  isConstantSizeOverflowSource(f, source.getNode().asInstruction(), delta)
+select source, source, sink,
+  "This pointer arithmetic may have an off-by-" + (delta + 1) +
+    " error allowing it to overrun $@ at this $@.", f, f.getName(), deref, operation

cpp/ql/src/experimental/Security/CWE/CWE-754/ImproperCheckReturnValueScanf.qhelp

@@ -12,7 +12,7 @@ The user should check the return value of `scanf` and related functions and chec
 </p>
 </recommendation>
 <example>
-<p>The first first example below is correct, as value of `i` is only read once it is checked that `scanf` has read one item. The second example is incorrect, as the return value of `scanf` is not checked, and as `scanf` might have failed to read any item before returning.</p>
+<p>The first example below is correct, as value of `i` is only read once it is checked that `scanf` has read one item. The second example is incorrect, as the return value of `scanf` is not checked, and as `scanf` might have failed to read any item before returning.</p>
 <sample src="ImproperCheckReturnValueScanf.cpp" />
 
 </example>

cpp/ql/src/jsf/4.07 Header Files/AV Rule 35.qhelp

@@ -5,12 +5,12 @@
 <overview>
 <p>
 Some header files, such as those which define structures or classes, cannot be included more than once within a translation unit, as doing so would
-cause a redefinition error. Such headers must be guarded to prevent ill-effects from multiple inclusion. Simlarly, if header files include other
+cause a redefinition error. Such headers must be guarded to prevent ill-effects from multiple inclusion. Similarly, if header files include other
 header files, and this inclusion graph contains a cycle, then at least one file within the cycle must contain header guards in order to break the
 cycle. Because of cases like these, all headers should be guarded as a matter of good practice, even if they do not strictly need to be.
 </p>
 <p>
-Furthermore, most modern compilers contain optimisations which are triggered by header guards. If the header guard strictly conforms to the pattern
+Furthermore, most modern compilers contain optimizations which are triggered by header guards. If the header guard strictly conforms to the pattern
 that compilers expect, then inclusions of that header other than the first have absolutely no effect: the file isn't re-read from disk, nor is it
 re-tokenised or re-preprocessed. This can result in a noticeable, albeit minor, improvement to compilation time.
 </p>

cpp/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/cpp-queries
-version: 0.4.1-dev
+version: 0.4.2-dev
 groups: 
   - cpp
   - queries