Merge pull request #1497 from geoffw0/dates-5

CPP: General clean up for the new dates queries

Author: jbj

cpp/ql/src/Likely Bugs/Leap Year/Adding365DaysPerYear.ql

@@ -5,8 +5,7 @@
  * @problem.severity warning
  * @id cpp/leap-year/adding-365-days-per-year
  * @precision medium
- * @tags security
- *       leap-year
+ * @tags leap-year
  */
 
 import cpp

cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll

@@ -1,5 +1,5 @@
 /**
- * Provides a library for helping create leap year related queries
+ * Provides a library for helping create leap year related queries.
  */
 
 import cpp
@@ -8,18 +8,18 @@ import semmle.code.cpp.controlflow.Guards
 import semmle.code.cpp.commons.DateTime
 
 /**
- * Get the top-level BinaryOperation enclosing the expression e
+ * Get the top-level `BinaryOperation` enclosing the expression e.
  */
-BinaryOperation getATopLevelBinaryOperationExpression(Expr e) {
+private BinaryOperation getATopLevelBinaryOperationExpression(Expr e) {
   result = e.getEnclosingElement().(BinaryOperation)
   or
   result = getATopLevelBinaryOperationExpression(e.getEnclosingElement())
 }
 
 /**
- * Holds if the top-level binary operation for expression `e` includes the operator specified in `operator` with an operand specified by `valueToCheck`
+ * Holds if the top-level binary operation for expression `e` includes the operator specified in `operator` with an operand specified by `valueToCheck`.
  */
-predicate additionalLogicalCheck(Expr e, string operation, int valueToCheck) {
+private predicate additionalLogicalCheck(Expr e, string operation, int valueToCheck) {
   exists(BinaryLogicalOperation bo | bo = getATopLevelBinaryOperationExpression(e) |
     exists(BinaryArithmeticOperation bao | bao = bo.getAChild*() |
       bao.getAnOperand().getValue().toInt() = valueToCheck and
@@ -29,13 +29,13 @@ predicate additionalLogicalCheck(Expr e, string operation, int valueToCheck) {
 }
 
 /**
- * Operation that seems to be checking for leap year
+ * An `Operation` that seems to be checking for leap year.
  */
 class CheckForLeapYearOperation extends Operation {
   CheckForLeapYearOperation() {
     exists(BinaryArithmeticOperation bo | bo = this |
       bo.getAnOperand().getValue().toInt() = 4 and
-      bo.getOperator().toString() = "%" and
+      bo.getOperator() = "%" and
       additionalLogicalCheck(this.getEnclosingElement(), "%", 100) and
       additionalLogicalCheck(this.getEnclosingElement(), "%", 400)
     )
@@ -45,12 +45,12 @@ class CheckForLeapYearOperation extends Operation {
 }
 
 /**
- * abstract class of type YearFieldAccess that would represent an access to a year field on a struct and is used for arguing about leap year calculations
+ * A `YearFieldAccess` that would represent an access to a year field on a struct and is used for arguing about leap year calculations.
  */
 abstract class LeapYearFieldAccess extends YearFieldAccess {
   /**
    * Holds if the field access is a modification,
-   * and it involves an arithmetic operation
+   * and it involves an arithmetic operation.
    */
   predicate isModifiedByArithmeticOperation() {
     this.isModified() and
@@ -59,8 +59,7 @@ abstract class LeapYearFieldAccess extends YearFieldAccess {
       (
         op instanceof AssignArithmeticOperation or
         exists(BinaryArithmeticOperation bao | bao = op.getAnOperand()) or
-        op instanceof PostfixCrementOperation or
-        op instanceof PrefixCrementOperation
+        op instanceof CrementOperation
       )
     )
   }
@@ -70,8 +69,8 @@ abstract class LeapYearFieldAccess extends YearFieldAccess {
    * and it involves an arithmetic operation.
    * In order to avoid false positives, the operation does not includes values that are normal for year normalization.
    *
-   * 1900 - struct tm counts years since 1900
-   * 1980/80 -  FAT32 epoch
+   * 1900 - `struct tm` counts years since 1900
+   * 1980/80 - FAT32 epoch
    */
   predicate isModifiedByArithmeticOperationNotForNormalization() {
     this.isModified() and
@@ -117,42 +116,40 @@ abstract class LeapYearFieldAccess extends YearFieldAccess {
           )
         )
         or
-        op instanceof PostfixCrementOperation
-        or
-        op instanceof PrefixCrementOperation
+        op instanceof CrementOperation
       )
     )
   }
 
   /**
-   * Holds if the top-level binary operation includes a modulus operator with an operand specified by `valueToCheck`
+   * Holds if the top-level binary operation includes a modulus operator with an operand specified by `valueToCheck`.
    */
   predicate additionalModulusCheckForLeapYear(int valueToCheck) {
     additionalLogicalCheck(this, "%", valueToCheck)
   }
 
   /**
-   * Holds if the top-level binary operation includes an addition or subtraction operator with an operand specified by `valueToCheck`
+   * Holds if the top-level binary operation includes an addition or subtraction operator with an operand specified by `valueToCheck`.
    */
   predicate additionalAdditionOrSubstractionCheckForLeapYear(int valueToCheck) {
     additionalLogicalCheck(this, "+", valueToCheck) or
     additionalLogicalCheck(this, "-", valueToCheck)
   }
 
   /**
-   * Holds true if this object is used on a modulus 4 operation, which would likely indicate the start of a leap year check
+   * Holds if this object is used on a modulus 4 operation, which would likely indicate the start of a leap year check.
    */
   predicate isUsedInMod4Operation() {
     not this.isModified() and
     exists(BinaryArithmeticOperation bo |
       bo.getAnOperand() = this and
       bo.getAnOperand().getValue().toInt() = 4 and
-      bo.getOperator().toString() = "%"
+      bo.getOperator() = "%"
     )
   }
 
   /**
-   * Holds true if this object seems to be used in a valid gregorian calendar leap year check
+   * Holds if this object seems to be used in a valid gregorian calendar leap year check.
    */
   predicate isUsedInCorrectLeapYearCheck() {
     // The Gregorian leap year rule is:
@@ -168,17 +165,17 @@ abstract class LeapYearFieldAccess extends YearFieldAccess {
 }
 
 /**
- * YearFieldAccess for SYSTEMTIME struct
+ * `YearFieldAccess` for the `SYSTEMTIME` struct.
  */
 class StructSystemTimeLeapYearFieldAccess extends LeapYearFieldAccess {
-  StructSystemTimeLeapYearFieldAccess() { this.toString().matches("wYear") }
+  StructSystemTimeLeapYearFieldAccess() { this.getTarget().getName() = "wYear" }
 }
 
 /**
- * YearFieldAccess for struct tm
+ * `YearFieldAccess` for `struct tm`.
  */
 class StructTmLeapYearFieldAccess extends LeapYearFieldAccess {
-  StructTmLeapYearFieldAccess() { this.toString().matches("tm_year") }
+  StructTmLeapYearFieldAccess() { this.getTarget().getName() = "tm_year" }
 
   override predicate isUsedInCorrectLeapYearCheck() {
     this.isUsedInMod4Operation() and
@@ -198,7 +195,7 @@ class StructTmLeapYearFieldAccess extends LeapYearFieldAccess {
 }
 
 /**
- * FunctionCall that includes an operation that is checking for leap year
+ * `FunctionCall` that includes an operation that is checking for leap year.
  */
 class ChecksForLeapYearFunctionCall extends FunctionCall {
   ChecksForLeapYearFunctionCall() {
@@ -209,8 +206,8 @@ class ChecksForLeapYearFunctionCall extends FunctionCall {
 }
 
 /**
- * DataFlow::Configuration for finding a variable access that would flow into
- * a function call that includes an operation to check for leap year
+ * `DataFlow::Configuration` for finding a variable access that would flow into
+ * a function call that includes an operation to check for leap year.
  */
 class LeapYearCheckConfiguration extends DataFlow::Configuration {
   LeapYearCheckConfiguration() { this = "LeapYearCheckConfiguration" }
@@ -225,7 +222,7 @@ class LeapYearCheckConfiguration extends DataFlow::Configuration {
 }
 
 /**
- * DataFlow::Configuration for finding an operation w/hardcoded 365 that will flow into a FILEINFO field
+ * `DataFlow::Configuration` for finding an operation with hardcoded 365 that will flow into a `FILEINFO` field.
  */
 class FiletimeYearArithmeticOperationCheckConfiguration extends DataFlow::Configuration {
   FiletimeYearArithmeticOperationCheckConfiguration() {
@@ -241,7 +238,7 @@ class FiletimeYearArithmeticOperationCheckConfiguration extends DataFlow::Config
 
   override predicate isSink(DataFlow::Node sink) {
     exists(StructLikeClass dds, FieldAccess fa, AssignExpr aexpr, Expr e | e = sink.asExpr() |
-      dds instanceof FileTimeStruct and
+      dds instanceof PackedTimeType and
       fa.getQualifier().getUnderlyingType() = dds and
       fa.isModified() and
       aexpr.getAChild() = fa and
@@ -251,7 +248,7 @@ class FiletimeYearArithmeticOperationCheckConfiguration extends DataFlow::Config
 }
 
 /**
- * DataFlow::Configuration for finding an operation w/hardcoded 365 that will flow into any known date/time field
+ * `DataFlow::Configuration` for finding an operation with hardcoded 365 that will flow into any known date/time field.
  */
 class PossibleYearArithmeticOperationCheckConfiguration extends DataFlow::Configuration {
   PossibleYearArithmeticOperationCheckConfiguration() {
@@ -272,7 +269,7 @@ class PossibleYearArithmeticOperationCheckConfiguration extends DataFlow::Config
       e = node1.asExpr() and
       aexpr = node2.asExpr()
     |
-      (dds instanceof FileTimeStruct or dds instanceof DateDataStruct) and
+      (dds instanceof PackedTimeType or dds instanceof UnpackedTimeType) and
       fa.getQualifier().getUnderlyingType() = dds and
       aexpr.getLValue() = fa and
       aexpr.getRValue().getAChild*() = e
@@ -281,7 +278,7 @@ class PossibleYearArithmeticOperationCheckConfiguration extends DataFlow::Config
 
   override predicate isSink(DataFlow::Node sink) {
     exists(StructLikeClass dds, FieldAccess fa, AssignExpr aexpr | aexpr = sink.asExpr() |
-      (dds instanceof FileTimeStruct or dds instanceof DateDataStruct) and
+      (dds instanceof PackedTimeType or dds instanceof UnpackedTimeType) and
       fa.getQualifier().getUnderlyingType() = dds and
       fa.isModified() and
       aexpr.getLValue() = fa

cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql

@@ -5,8 +5,7 @@
  * @problem.severity warning
  * @id cpp/leap-year/unchecked-after-arithmetic-year-modification
  * @precision medium
- * @tags security
- *       leap-year
+ * @tags leap-year
  */
 
 import cpp

cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationGood.c

@@ -2,7 +2,7 @@ SYSTEMTIME st;
 FILETIME ft;
 GetSystemTime(&st);
 
-// Flawed logic will result in invalid date
+// Flawed logic may result in invalid date
 st.wYear++;
 
 // Check for leap year, and adjust the date accordingly
@@ -12,4 +12,4 @@ st.wDay = st.wMonth == 2 && st.wDay == 29 && !isLeapYear ? 28 : st.wDay;
 if (!SystemTimeToFileTime(&st, &ft))
 {
 	// handle error
-}
+}

cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql

@@ -5,67 +5,66 @@
  * @problem.severity warning
  * @id cpp/leap-year/unchecked-return-value-for-time-conversion-function
  * @precision medium
- * @tags security
- *       leap-year
+ * @tags leap-year
  */
 
 import cpp
 import LeapYear
 
 /**
- * A YearFieldAccess that is modifying the year by any arithmetic operation
+ * A `YearFieldAccess` that is modifying the year by any arithmetic operation.
  *
  * NOTE:
  * To change this class to work for general purpose date transformations that do not check the return value,
  * make the following changes:
- *  -> extends FieldAccess (line 27)
- *  -> this.isModified (line 33)
+ *  - change `extends LeapYearFieldAccess` to `extends FieldAccess`.
+ *  - change `this.isModifiedByArithmeticOperation()` to `this.isModified()`.
  * Expect a lower precision for a general purpose version.
  */
 class DateStructModifiedFieldAccess extends LeapYearFieldAccess {
   DateStructModifiedFieldAccess() {
     exists(Field f, StructLikeClass struct |
       f.getAnAccess() = this and
       struct.getAField() = f and
-      struct.getUnderlyingType() instanceof DateDataStruct and
+      struct.getUnderlyingType() instanceof UnpackedTimeType and
       this.isModifiedByArithmeticOperation()
     )
   }
 }
 
 /**
- * This is a list of APIs that will get the system time, and therefore guarantee that the value is valid
+ * This is a list of APIs that will get the system time, and therefore guarantee that the value is valid.
  */
 class SafeTimeGatheringFunction extends Function {
   SafeTimeGatheringFunction() {
-    this.getQualifiedName().matches("GetFileTime") or
-    this.getQualifiedName().matches("GetSystemTime") or
-    this.getQualifiedName().matches("NtQuerySystemTime")
+    this.getQualifiedName() = "GetFileTime" or
+    this.getQualifiedName() = "GetSystemTime" or
+    this.getQualifiedName() = "NtQuerySystemTime"
   }
 }
 
 /**
- * This list of APIs should check for the return value to detect problems during the conversion
+ * This list of APIs should check for the return value to detect problems during the conversion.
  */
 class TimeConversionFunction extends Function {
   TimeConversionFunction() {
-    this.getQualifiedName().matches("FileTimeToSystemTime") or
-    this.getQualifiedName().matches("SystemTimeToFileTime") or
-    this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTime") or
-    this.getQualifiedName().matches("SystemTimeToTzSpecificLocalTimeEx") or
-    this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTime") or
-    this.getQualifiedName().matches("TzSpecificLocalTimeToSystemTimeEx") or
-    this.getQualifiedName().matches("RtlLocalTimeToSystemTime") or
-    this.getQualifiedName().matches("RtlTimeToSecondsSince1970") or
-    this.getQualifiedName().matches("_mkgmtime")
+    this.getQualifiedName() = "FileTimeToSystemTime" or
+    this.getQualifiedName() = "SystemTimeToFileTime" or
+    this.getQualifiedName() = "SystemTimeToTzSpecificLocalTime" or
+    this.getQualifiedName() = "SystemTimeToTzSpecificLocalTimeEx" or
+    this.getQualifiedName() = "TzSpecificLocalTimeToSystemTime" or
+    this.getQualifiedName() = "TzSpecificLocalTimeToSystemTimeEx" or
+    this.getQualifiedName() = "RtlLocalTimeToSystemTime" or
+    this.getQualifiedName() = "RtlTimeToSecondsSince1970" or
+    this.getQualifiedName() = "_mkgmtime"
   }
 }
 
 from FunctionCall fcall, TimeConversionFunction trf, Variable var
 where
   fcall = trf.getACallToThisFunction() and
   fcall instanceof ExprInVoidContext and
-  var.getUnderlyingType() instanceof DateDataStruct and
+  var.getUnderlyingType() instanceof UnpackedTimeType and
   (
     exists(AddressOfExpr aoe |
       aoe = fcall.getAnArgument() and

cpp/ql/src/Likely Bugs/Leap Year/UnsafeArrayForDaysOfYear.ql

@@ -16,22 +16,26 @@ class LeapYearUnsafeDaysOfTheYearArrayType extends ArrayType {
   LeapYearUnsafeDaysOfTheYearArrayType() { this.getArraySize() = 365 }
 }
 
-from Element element
+from Element element, string allocType
 where
   exists(NewArrayExpr nae |
     element = nae and
-    nae.getAllocatedType() instanceof LeapYearUnsafeDaysOfTheYearArrayType
+    nae.getAllocatedType() instanceof LeapYearUnsafeDaysOfTheYearArrayType and
+    allocType = "an array allocation"
   )
   or
   exists(Variable var |
     var = element and
-    var.getType() instanceof LeapYearUnsafeDaysOfTheYearArrayType
+    var.getType() instanceof LeapYearUnsafeDaysOfTheYearArrayType and
+    allocType = "an array allocation"
   )
   or
   exists(ConstructorCall cc |
     element = cc and
     cc.getTarget().hasName("vector") and
-    cc.getArgument(0).getValue().toInt() = 365
+    cc.getArgument(0).getValue().toInt() = 365 and
+    allocType = "a std::vector allocation"
   )
 select element,
-  "There is an array or std::vector allocation with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios."
+  "There is " + allocType +
+    " with a hard-coded set of 365 elements, which may indicate the number of days in a year without considering leap year scenarios."