fix OutOfBounds behavior with new dataflow

Author: mbaluda

c/cert/src/rules/INT31-C/IntegerConversionCausesDataLoss.ql

@@ -91,7 +91,7 @@ where
   ) and
   // A conversion of `-1` to `time_t` is permitted by the standard
   not (
-    c.getType().hasName("time_t") and
+    c.getType().getUnspecifiedType().hasName("time_t") and
     preConversionExpr.getValue() = "-1"
   ) and
   // Conversion to unsigned char is permitted from the range [SCHAR_MIN..UCHAR_MAX], as those can

c/cert/test/rules/ARR30-C/DoNotFormOutOfBoundsPointersOrArraySubscripts.expected

@@ -5,7 +5,13 @@
 | test.c:45:17:45:30 | ... + ... | Buffer may access up to offset 101*1 which is greater than the fixed size 100 of the $@. | test.c:45:17:45:22 | buffer | buffer |
 | test.c:55:5:55:13 | ... - ... | Buffer access may be to a negative index in the buffer. | test.c:55:5:55:9 | ptr16 | buffer |
 | test.c:57:5:57:14 | ... + ... | Buffer accesses offset 22 which is greater than the fixed size 20 of the $@. | test.c:57:5:57:9 | ptr16 | buffer |
+| test.c:58:5:58:14 | ... - ... | Buffer access may be to a negative index in the buffer. | test.c:55:5:55:9 | ptr16 | buffer |
+| test.c:58:5:58:14 | ... - ... | Buffer access may be to a negative index in the buffer. | test.c:56:5:56:9 | ptr16 | buffer |
+| test.c:58:5:58:14 | ... - ... | Buffer access may be to a negative index in the buffer. | test.c:57:5:57:9 | ptr16 | buffer |
 | test.c:58:5:58:14 | ... - ... | Buffer access may be to a negative index in the buffer. | test.c:58:5:58:9 | ptr16 | buffer |
 | test.c:63:3:63:9 | access to array | Buffer access may be to a negative index in the buffer. | test.c:63:3:63:5 | arr | buffer |
 | test.c:65:3:65:9 | access to array | Buffer accesses offset 44 which is greater than the fixed size 40 of the $@. | test.c:65:3:65:5 | arr | buffer |
+| test.c:66:3:66:10 | access to array | Buffer access may be to a negative index in the buffer. | test.c:63:3:63:5 | arr | buffer |
+| test.c:66:3:66:10 | access to array | Buffer access may be to a negative index in the buffer. | test.c:64:3:64:5 | arr | buffer |
+| test.c:66:3:66:10 | access to array | Buffer access may be to a negative index in the buffer. | test.c:65:3:65:5 | arr | buffer |
 | test.c:66:3:66:10 | access to array | Buffer access may be to a negative index in the buffer. | test.c:66:3:66:5 | arr | buffer |

c/common/src/codingstandards/c/OutOfBounds.qll

@@ -379,8 +379,13 @@ module OOB {
     StrncatLibraryFunction() { this.getName() = getNameOrInternalName(["strncat", "wcsncat"]) }
 
     override predicate getALengthParameterIndex(int i) {
-      // `strncat` and `wcsncat` exclude the size of a null terminator
-      i = 2
+      // The source need not contain a null terminator within the first `n` characters.
+      none()
+    }
+
+    override predicate getANullTerminatedParameterIndex(int i) {
+      // The destination must be null-terminated.
+      i = 0
     }
   }
 
@@ -644,42 +649,46 @@ module OOB {
   }
 
   /**
-   * A class for reasoning about the offset of a variable from the original value flowing to it
-   * as a result of arithmetic or pointer arithmetic expressions.
+   * Gets the offset of `expr` from `underlyingBase` due to arithmetic or pointer arithmetic.
+   *
+   * `underlyingBase` may be the arithmetic operand's base expression or `expr` itself, allowing
+   * callers to use whichever dataflow node is available.
    */
   bindingset[expr]
-  private int getArithmeticOffsetValue(Expr expr, Expr base) {
-    result = getMinStatedValue(expr.(PointerArithmeticExpr).getOperand()) and
-    base = expr.(PointerArithmeticExpr).getPointer()
-    or
-    // &(array[index]) expressions
-    result =
-      getMinStatedValue(expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getOperand()) and
-    base = expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getPointer()
-    or
-    result = getMinStatedValue(expr.(AddExpr).getRightOperand()) and
-    base = expr.(AddExpr).getLeftOperand()
-    or
-    result = -getMinStatedValue(expr.(SubExpr).getRightOperand()) and
-    base = expr.(SubExpr).getLeftOperand()
-    or
-    expr instanceof IncrementOperation and
-    result = 1 and
-    base = expr.(IncrementOperation).getOperand()
-    or
-    expr instanceof DecrementOperation and
-    result = -1 and
-    base = expr.(DecrementOperation).getOperand()
-    or
-    // fall-back if `expr` is not an arithmetic or pointer arithmetic expression
-    not expr instanceof PointerArithmeticExpr and
-    not expr.(AddressOfExpr).getOperand() instanceof PointerArithmeticExpr and
-    not expr instanceof AddExpr and
-    not expr instanceof SubExpr and
-    not expr instanceof IncrementOperation and
-    not expr instanceof DecrementOperation and
-    base = expr and
-    result = 0
+  private int getArithmeticOffsetValue(Expr expr, Expr underlyingBase) {
+    exists(Expr base | underlyingBase = [base, expr] |
+      result = getMinStatedValue(expr.(PointerArithmeticExpr).getOperand()) and
+      base = expr.(PointerArithmeticExpr).getPointer()
+      or
+      // &(array[index]) expressions
+      result =
+        getMinStatedValue(expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getOperand()) and
+      base = expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getPointer()
+      or
+      result = getMinStatedValue(expr.(AddExpr).getRightOperand()) and
+      base = expr.(AddExpr).getLeftOperand()
+      or
+      result = -getMinStatedValue(expr.(SubExpr).getRightOperand()) and
+      base = expr.(SubExpr).getLeftOperand()
+      or
+      expr instanceof IncrementOperation and
+      result = 1 and
+      base = expr.(IncrementOperation).getOperand()
+      or
+      expr instanceof DecrementOperation and
+      result = -1 and
+      base = expr.(DecrementOperation).getOperand()
+      or
+      // fall-back if `expr` is not an arithmetic or pointer arithmetic expression
+      not expr instanceof PointerArithmeticExpr and
+      not expr.(AddressOfExpr).getOperand() instanceof PointerArithmeticExpr and
+      not expr instanceof AddExpr and
+      not expr instanceof SubExpr and
+      not expr instanceof IncrementOperation and
+      not expr instanceof DecrementOperation and
+      base = expr and
+      result = 0
+    )
   }
 
   private int constOrZero(Expr e) {

c/common/test/rules/constlikereturnvalue/ConstLikeReturnValue.expected

@@ -1,5 +1,4 @@
 problems
-| test.c:11:7:11:12 | * ... | test.c:18:16:18:21 | call to getenv | test.c:11:7:11:12 | * ... | The object returned by the function getenv should not be modified. |
 | test.c:11:8:11:12 | c_str | test.c:18:16:18:21 | call to getenv | test.c:11:7:11:12 | * ... | The object returned by the function getenv should not be modified. |
 | test.c:67:5:67:9 | conv4 | test.c:64:11:64:20 | call to localeconv | test.c:67:5:67:9 | conv4 | The object returned by the function localeconv should not be modified. |
 | test.c:76:5:76:8 | conv | test.c:72:25:72:34 | call to localeconv | test.c:76:5:76:8 | conv | The object returned by the function localeconv should not be modified. |

c/misra/test/rules/RULE-21-18/test.c

@@ -103,4 +103,10 @@ void test(void) {
     strxfrm(buf + 1, buf2,
             sizeof(buf) - 1); // NON_COMPLIANT - not null-terminated
   }
-}
+}
+
+void test_strncat_bounded_source(void) {
+  char destination[2] = {0};
+  char source[1] = {'x'};
+  strncat(destination, source, 1); // COMPLIANT
+}

cpp/cert/test/rules/MEM52-CPP/DetectAndHandleMemoryAllocationErrors.expected

@@ -1,8 +1,2 @@
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (DetectAndHandleMemoryAllocationErrors.ql:64,5-13)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (DetectAndHandleMemoryAllocationErrors.ql:87,46-54)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (DetectAndHandleMemoryAllocationErrors.ql:88,22-30)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (DetectAndHandleMemoryAllocationErrors.ql:92,20-28)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (DetectAndHandleMemoryAllocationErrors.ql:97,35-43)
-WARNING: module 'DataFlow' has been deprecated and may be removed in future (DetectAndHandleMemoryAllocationErrors.ql:102,38-46)
 | test.cpp:24:7:24:34 | new | nothrow new allocation of $@ returns here without a subsequent check to see whether the pointer is valid. | test.cpp:24:7:24:34 | new | StructA * |
 | test.cpp:40:17:40:38 | call to allocate_without_check | nothrow new allocation of $@ returns here without a subsequent check to see whether the pointer is valid. | test.cpp:35:17:35:44 | new | StructA * |

cpp/common/src/codingstandards/cpp/OutOfBounds.qll

@@ -384,16 +384,13 @@ module OOB {
     StrncatLibraryFunction() { this.getName() = getNameOrInternalName(["strncat", "wcsncat"]) }
 
     override predicate getALengthParameterIndex(int i) {
-      // `strncat` and `wcsncat` exclude the size of a null terminator, but
-      // both stops copying right after the null terminator is encountered.
-      // In fact, they don't care if the source buffer is null-terminated
-      // or not.
+      // The source need not contain a null terminator within the first `n` characters.
       none()
     }
 
     override predicate getANullTerminatedParameterIndex(int i) {
-      // `strncat` does not require null-terminated parameters
-      none()
+      // The destination must be null-terminated.
+      i = 0
     }
   }
 
@@ -657,42 +654,46 @@ module OOB {
   }
 
   /**
-   * A class for reasoning about the offset of a variable from the original value flowing to it
-   * as a result of arithmetic or pointer arithmetic expressions.
+   * Gets the offset of `expr` from `underlyingBase` due to arithmetic or pointer arithmetic.
+   *
+   * `underlyingBase` may be the arithmetic operand's base expression or `expr` itself, allowing
+   * callers to use whichever dataflow node is available.
    */
   bindingset[expr]
-  private int getArithmeticOffsetValue(Expr expr, Expr base) {
-    result = getMinStatedValue(expr.(PointerArithmeticExpr).getOperand()) and
-    base = expr.(PointerArithmeticExpr).getPointer()
-    or
-    // &(array[index]) expressions
-    result =
-      getMinStatedValue(expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getOperand()) and
-    base = expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getPointer()
-    or
-    result = getMinStatedValue(expr.(AddExpr).getRightOperand()) and
-    base = expr.(AddExpr).getLeftOperand()
-    or
-    result = -getMinStatedValue(expr.(SubExpr).getRightOperand()) and
-    base = expr.(SubExpr).getLeftOperand()
-    or
-    expr instanceof IncrementOperation and
-    result = 1 and
-    base = expr.(IncrementOperation).getOperand()
-    or
-    expr instanceof DecrementOperation and
-    result = -1 and
-    base = expr.(DecrementOperation).getOperand()
-    or
-    // fall-back if `expr` is not an arithmetic or pointer arithmetic expression
-    not expr instanceof PointerArithmeticExpr and
-    not expr.(AddressOfExpr).getOperand() instanceof PointerArithmeticExpr and
-    not expr instanceof AddExpr and
-    not expr instanceof SubExpr and
-    not expr instanceof IncrementOperation and
-    not expr instanceof DecrementOperation and
-    base = expr and
-    result = 0
+  private int getArithmeticOffsetValue(Expr expr, Expr underlyingBase) {
+    exists(Expr base | underlyingBase = [base, expr] |
+      result = getMinStatedValue(expr.(PointerArithmeticExpr).getOperand()) and
+      base = expr.(PointerArithmeticExpr).getPointer()
+      or
+      // &(array[index]) expressions
+      result =
+        getMinStatedValue(expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getOperand()) and
+      base = expr.(AddressOfExpr).getOperand().(PointerArithmeticExpr).getPointer()
+      or
+      result = getMinStatedValue(expr.(AddExpr).getRightOperand()) and
+      base = expr.(AddExpr).getLeftOperand()
+      or
+      result = -getMinStatedValue(expr.(SubExpr).getRightOperand()) and
+      base = expr.(SubExpr).getLeftOperand()
+      or
+      expr instanceof IncrementOperation and
+      result = 1 and
+      base = expr.(IncrementOperation).getOperand()
+      or
+      expr instanceof DecrementOperation and
+      result = -1 and
+      base = expr.(DecrementOperation).getOperand()
+      or
+      // fall-back if `expr` is not an arithmetic or pointer arithmetic expression
+      not expr instanceof PointerArithmeticExpr and
+      not expr.(AddressOfExpr).getOperand() instanceof PointerArithmeticExpr and
+      not expr instanceof AddExpr and
+      not expr instanceof SubExpr and
+      not expr instanceof IncrementOperation and
+      not expr instanceof DecrementOperation and
+      base = expr and
+      result = 0
+    )
   }
 
   private int constOrZero(Expr e) {

cpp/common/src/codingstandards/cpp/Overflow.qll

@@ -6,7 +6,7 @@ import cpp
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import SimpleRangeAnalysisCustomizations
 import semmle.code.cpp.controlflow.Guards
-import semmle.code.cpp.dataflow.TaintTracking
+import semmle.code.cpp.dataflow.new.TaintTracking
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import codingstandards.cpp.Expr
 import codingstandards.cpp.UndefinedBehavior

cpp/common/test/rules/constlikereturnvalue/ConstLikeReturnValue.expected

@@ -1,5 +1,4 @@
 problems
-| test.cpp:11:7:11:12 | * ... | test.cpp:18:16:18:21 | call to getenv | test.cpp:11:7:11:12 | * ... | The object returned by the function getenv should not be modified. |
 | test.cpp:11:8:11:12 | c_str | test.cpp:18:16:18:21 | call to getenv | test.cpp:11:7:11:12 | * ... | The object returned by the function getenv should not be modified. |
 | test.cpp:67:5:67:9 | conv4 | test.cpp:64:11:64:20 | call to localeconv | test.cpp:67:5:67:9 | conv4 | The object returned by the function localeconv should not be modified. |
 | test.cpp:76:5:76:8 | conv | test.cpp:72:25:72:34 | call to localeconv | test.cpp:76:5:76:8 | conv | The object returned by the function localeconv should not be modified. |

cpp/misra/test/rules/RULE-8-7-1/PointerArgumentToCstringFunctionIsInvalid.expected

@@ -37,3 +37,4 @@
 | test.cpp:477:5:477:10 | call to memcpy | The size of the $@ passed to memcpy is 63 bytes, but the $@ is 128 bytes. | test.cpp:477:17:477:24 | ... + ... | read buffer | test.cpp:477:27:477:41 | ... * ... | size argument |
 | test.cpp:477:5:477:10 | call to memcpy | The size of the $@ passed to memcpy is 64 bytes, but the $@ is 128 bytes. | test.cpp:477:12:477:14 | buf | write buffer | test.cpp:477:27:477:41 | ... * ... | size argument |
 | test.cpp:484:3:484:8 | call to memcpy | The $@ passed to memcpy is accessed at an excessive offset of 1 element(s) from the $@. | test.cpp:484:10:484:10 | p | write buffer | test.cpp:482:30:482:50 | ... * ... | allocation size base |
+| test.cpp:552:3:552:9 | call to strncat | The $@ passed to strncat might not be null-terminated. | test.cpp:552:11:552:21 | destination | argument | test.cpp:552:11:552:21 | destination |  |