diff --git a/cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll b/cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll index c67bf7cf96e3..b01c775e5535 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll +++ b/cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll @@ -14,7 +14,7 @@ class PackedTimeType extends Type { } } -private predicate timeType(string typeName) { typeName = ["_SYSTEMTIME", "SYSTEMTIME", "tm"] } +private predicate timeType(string typeName) { typeName = ["_SYSTEMTIME", "SYSTEMTIME", "tm", "TIME_FIELDS", "PTIME_FIELDS"] } /** * A type that is used to represent times and dates in an 'unpacked' form, that is, @@ -95,3 +95,24 @@ class StructTmMonthFieldAccess extends MonthFieldAccess { class StructTmYearFieldAccess extends YearFieldAccess { StructTmYearFieldAccess() { this.getTarget().getName() = "tm_year" } } + +/** + * A `DayFieldAccess` for the `TIME_FIELDS` struct. + */ +class TimeFieldsDayFieldAccess extends DayFieldAccess { + TimeFieldsDayFieldAccess() { this.getTarget().getName() = "Day" } +} + +/** + * A `MonthFieldAccess` for the `TIME_FIELDS` struct. + */ +class TimeFieldsMonthFieldAccess extends MonthFieldAccess { + TimeFieldsMonthFieldAccess() { this.getTarget().getName() = "Month" } +} + +/** + * A `YearFieldAccess` for the `TIME_FIELDS` struct. + */ +class TimeFieldsYearFieldAccess extends YearFieldAccess { + TimeFieldsYearFieldAccess() { this.getTarget().getName() = "Year" } +} \ No newline at end of file diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll index 06b6aff66abd..39e56627db2a 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll +++ b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll @@ -41,7 +41,6 @@ class CheckForLeapYearOperation extends Expr { } } -bindingset[modVal] Expr moduloCheckEQ_0(EQExpr eq, int modVal) { exists(RemExpr rem | rem = eq.getLeftOperand() | result = rem.getLeftOperand() and @@ -50,7 +49,6 @@ Expr moduloCheckEQ_0(EQExpr eq, int modVal) { eq.getRightOperand().getValue().toInt() = 0 } -bindingset[modVal] Expr moduloCheckNEQ_0(NEExpr neq, int modVal) { exists(RemExpr rem | rem = neq.getLeftOperand() | result = rem.getLeftOperand() and @@ -59,48 +57,6 @@ Expr moduloCheckNEQ_0(NEExpr neq, int modVal) { neq.getRightOperand().getValue().toInt() = 0 } -/** - * Returns if the two expressions resolve to the same value, albeit it is a fuzzy attempt. - * SSA is not fit for purpose here as calls break SSA equivalence. - */ -predicate exprEq_propertyPermissive(Expr e1, Expr e2) { - not e1 = e2 and - ( - DataFlow::localFlow(DataFlow::exprNode(e1), DataFlow::exprNode(e2)) - or - if e1 instanceof ThisExpr and e2 instanceof ThisExpr - then any() - else - /* If it's a direct Access, check that the target is the same. */ - if e1 instanceof Access - then e1.(Access).getTarget() = e2.(Access).getTarget() - else - /* If it's a Call, compare qualifiers and only permit no-argument Calls. */ - if e1 instanceof Call - then - e1.(Call).getTarget() = e2.(Call).getTarget() and - e1.(Call).getNumberOfArguments() = 0 and - e2.(Call).getNumberOfArguments() = 0 and - if e1.(Call).hasQualifier() - then exprEq_propertyPermissive(e1.(Call).getQualifier(), e2.(Call).getQualifier()) - else any() - else - /* If it's a binaryOperation, compare op and recruse */ - if e1 instanceof BinaryOperation - then - e1.(BinaryOperation).getOperator() = e2.(BinaryOperation).getOperator() and - exprEq_propertyPermissive(e1.(BinaryOperation).getLeftOperand(), - e2.(BinaryOperation).getLeftOperand()) and - exprEq_propertyPermissive(e1.(BinaryOperation).getRightOperand(), - e2.(BinaryOperation).getRightOperand()) - else - // Otherwise fail (and permit the raising of a finding) - if e1 instanceof Literal - then e1.(Literal).getValue() = e2.(Literal).getValue() - else none() - ) -} - /** * An expression that is the subject of a mod-4 check. * ie `expr % 4 == 0` @@ -226,8 +182,7 @@ class ExprCheckCenturyComponent extends LogicalOrExpr { ExprCheckCenturyComponent() { exists(ExprCheckCenturyComponentDiv400 exprDiv400, ExprCheckCenturyComponentDiv100 exprDiv100 | this.getAnOperand() = exprDiv100 and - this.getAnOperand() = exprDiv400 and - exprEq_propertyPermissive(exprDiv100.getYearExpr(), exprDiv400.getYearExpr()) + this.getAnOperand() = exprDiv400 ) } @@ -252,8 +207,7 @@ final class ExprCheckLeapYearFormA extends ExprCheckLeapYear, LogicalAndExpr { ExprCheckLeapYearFormA() { exists(Expr e, ExprCheckCenturyComponent centuryCheck | e = moduloCheckEQ_0(this.getLeftOperand(), 4) and - centuryCheck = this.getAnOperand().getAChild*() and - exprEq_propertyPermissive(e, centuryCheck.getYearExpr()) + centuryCheck = this.getAnOperand().getAChild*() ) } } @@ -268,12 +222,7 @@ final class ExprCheckLeapYearFormB extends ExprCheckLeapYear, LogicalOrExpr { exists(VariableAccess va1, VariableAccess va2, VariableAccess va3 | va1 = moduloCheckEQ_0(this.getAnOperand(), 400) and va2 = moduloCheckNEQ_0(this.getAnOperand().(LogicalAndExpr).getAnOperand(), 100) and - va3 = moduloCheckEQ_0(this.getAnOperand().(LogicalAndExpr).getAnOperand(), 4) and - // The 400-leap year check may be offset by [1900,1970,2000]. - exists(Expr va1_subExpr | va1_subExpr = va1.getAChild*() | - exprEq_propertyPermissive(va1_subExpr, va2) and - exprEq_propertyPermissive(va2, va3) - ) + va3 = moduloCheckEQ_0(this.getAnOperand().(LogicalAndExpr).getAnOperand(), 4) ) } } @@ -410,6 +359,50 @@ class StructTmLeapYearFieldAccess extends LeapYearFieldAccess { } } +/** + * `stDate.wMonth == 2` + */ +class DateCheckMonthFebruary extends Operation { + DateCheckMonthFebruary() { + this.getOperator() = "==" and + this.getAnOperand() instanceof MonthFieldAccess and + this.getAnOperand().(Literal).getValue() = "2" + } + + Expr getDateQualifier() { result = this.getAnOperand().(MonthFieldAccess).getQualifier() } +} + +/** + * `stDate.wDay == 29` + */ +class DateCheckDay29 extends Operation { + DateCheckDay29() { + this.getOperator() = "==" and + this.getAnOperand() instanceof DayFieldAccess and + this.getAnOperand().(Literal).getValue() = "29" + } + + Expr getDateQualifier() { result = this.getAnOperand().(DayFieldAccess).getQualifier() } +} + +/** + * The combination of a February and Day 29 verification + * `stDate.wMonth == 2 && stDate.wDay == 29` + */ +class DateFebruary29Check extends Operation { + DateFebruary29Check() { + this.getOperator() = "&&" and + exists(DateCheckMonthFebruary checkFeb, DateCheckDay29 check29 | + checkFeb = this.getAnOperand() and + check29 = this.getAnOperand() + ) + } + + Expr getDateQualifier() { + result = this.getAnOperand().(DateCheckMonthFebruary).getDateQualifier() + } +} + /** * `Function` that includes an operation that is checking for leap year. */ @@ -425,7 +418,7 @@ class ChecksForLeapYearFunctionCall extends FunctionCall { } /** - * A `DataFlow` configuraiton for finding a variable access that would flow into + * A `DataFlow` configuration for finding a variable access that would flow into * a function call that includes an operation to check for leap year. */ private module LeapYearCheckFlowConfig implements DataFlow::ConfigSig { @@ -541,7 +534,7 @@ class TimeConversionFunction extends Function { "FileTimeToSystemTime", "SystemTimeToFileTime", "SystemTimeToTzSpecificLocalTime", "SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime", "TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime", - "RtlTimeToSecondsSince1970", "_mkgmtime" + "RtlTimeToSecondsSince1970", "_mkgmtime", "SetSystemTime" ] } } diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql index 18ad003eb71f..21b6403f2c13 100644 --- a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +++ b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql @@ -1,7 +1,7 @@ /** * @name Year field changed using an arithmetic operation without checking for leap year (AntiPattern 1) * @description A field that represents a year is being modified by an arithmetic operation, but no proper check for leap years can be detected afterwards. - * @kind problem + * @kind path-problem * @problem.severity warning * @id cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification * @precision medium @@ -11,98 +11,332 @@ import cpp import LeapYear +import semmle.code.cpp.controlflow.IRGuards /** - * Holds if there is no known leap-year verification for the given `YearWriteOp`. - * Binds the `var` argument to the qualifier of the `ywo` argument. - */ -bindingset[ywo] -predicate isYearModifedWithoutExplicitLeapYearCheck(Variable var, YearWriteOp ywo) { - exists(VariableAccess va, YearFieldAccess yfa | - yfa = ywo.getYearAccess() and - yfa.getQualifier() = va and - var.getAnAccess() = va and - // Avoid false positives - not ( - // If there is a local check for leap year after the modification - exists(LeapYearFieldAccess yfacheck | - yfacheck.getQualifier() = var.getAnAccess() and - yfacheck.isUsedInCorrectLeapYearCheck() and - yfacheck.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() - ) + * Functions whose operations should never be considered a + * source or sink of a dangerous leap year operation. + */ +class IgnorableFunction extends Function { + IgnorableFunction() { + // Helper utility in postgres with string time conversions + this.getName() = "DecodeISO8601Interval" + or + // helper utility for date conversions in qtbase + this.getName() = "adjacentDay" + or + // Windows API function that does timezone conversions + this.getName().matches("%SystemTimeToTzSpecificLocalTime%") + or + // Windows APIs that do time conversions + this.getName().matches("%localtime%\\_s%") + or + // Windows APIs that do time conversions + this.getName().matches("%SpecificLocalTimeToSystemTime%") + or + // postgres function for diffing timestamps, date for leap year + // is not applicable. + this.getName().toLowerCase().matches("%timestamp%age%") + or + // Reading byte streams often involves operations of some base, but that's + // not a real source of leap year issues. + this.getName().toLowerCase().matches("%read%bytes%") + or + // A postgres function for local time conversions + // conversion operations (from one time structure to another) are generally ignorable + this.getName() = "localsub" + } +} + +/** + * The set of expressions which are ignorable; either because they seem to not be part of a year mutation, + * or because they seem to be a conversion pattern of mapping date scalars. + */ +abstract class IgnorableOperation extends Expr { } + +class IgnorableExprRem extends IgnorableOperation instanceof RemExpr { } + +/** + * Anything involving an operation with 10, 100, 1000, 10000 is often a sign of conversion + * or atoi. + */ +class IgnorableExpr10MulipleComponent extends IgnorableOperation { + IgnorableExpr10MulipleComponent() { + this.(Operation).getAnOperand().getValue().toInt() in [10, 100, 1000, 10000] + or + exists(AssignOperation a | a.getRValue() = this | + a.getRValue().getValue().toInt() in [10, 100, 1000, 10000] + ) + } +} + +/** + * Anything involving a sub expression with char literal 48, ignore as a likely string conversion + * e.g., X - '0' + */ +class IgnorableExpr48Mapping extends IgnorableOperation { + IgnorableExpr48Mapping() { + this.(SubExpr).getRightOperand().getValue().toInt() = 48 + or + exists(AssignSubExpr e | e.getRValue() = this | e.getRValue().getValue().toInt() = 48) + } +} + +class IgnorableCharLiteralArithmetic extends IgnorableOperation { + IgnorableCharLiteralArithmetic() { + exists(this.(BinaryArithmeticOperation).getAnOperand().(TextLiteral).getValue()) + or + exists(AssignArithmeticOperation e | e.getRValue() = this | + exists(this.(TextLiteral).getValue()) + ) + } +} + +/** + * Constants often used in date conversions (from one date data type to another) + * Numerous examples exist, like 1900 or 2000 that convert years from one + * representation to another. + * Also '0' is sometimes observed as an atoi style conversion. + */ +bindingset[c] +predicate isLikelyConversionConstant(int c) { + exists(int i | i = c.abs() | + //| i >= 100) + i = 146097 or // days in 400-year Gregorian cycle + i = 36524 or // days in 100-year Gregorian subcycle + i = 1461 or // days in 4-year cycle (incl. 1 leap) + i = 32044 or // Fliegel–van Flandern JDN epoch shift + i = 1721425 or // JDN of 0001‑01‑01 (Gregorian) + i = 1721119 or // alt epoch offset + i = 2400000 or // MJD → JDN conversion + i = 2400001 or // alt MJD → JDN conversion + i = 2141 or // fixed‑point month/day extraction + i = 2000 or // observed in some conversions + i = 65536 or // observed in some conversions + i = 7834 or // observed in some conversions + i = 256 or // observed in some conversions + i = 1900 or // struct tm base‑year offset; harmless + i = 1899 or // Observed in uses with 1900 to address off by one scenarios + i = 292275056 or // qdatetime.h Qt Core year range first year constant + i = 292278994 or // qdatetime.h Qt Core year range last year constant + i = 0 + ) +} + +/** + * Some constants indicate conversion that are ignorable, e.g., + * julian to gregorian conversion or conversions from linux time structs + * that start at 1900, etc. + */ +class IgnorableConstantArithmetic extends IgnorableOperation { + IgnorableConstantArithmetic() { + exists(int i | isLikelyConversionConstant(i) | + this.(Operation).getAnOperand().getValue().toInt() = i or - // If there is a data flow from the variable that was modified to a function that seems to check for leap year - exists(VariableAccess source, ChecksForLeapYearFunctionCall fc | - source = var.getAnAccess() and - LeapYearCheckFlow::flow(DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument())) + exists(AssignArithmeticOperation a | this = a.getRValue() | + a.getRValue().getValue().toInt() = i ) + ) + } +} + +// If a unary minus assume it is some sort of conversion +class IgnorableUnaryMinus extends IgnorableOperation { + IgnorableUnaryMinus() { + this instanceof UnaryMinusExpr + or + this.(Operation).getAnOperand() instanceof UnaryMinusExpr + } +} + +class OperationAsArgToIgnorableFunction extends IgnorableOperation { + OperationAsArgToIgnorableFunction() { + exists(Call c | + c.getAnArgument().getAChild*() = this and + c.getTarget() instanceof IgnorableFunction + ) + } +} + +/** + * Literal OP literal means the result is constant/known + * and the operation is basically ignorable (it's not a real operation but + * probably one visual simplicity what it means). + */ +class ConstantBinaryArithmeticOperation extends IgnorableOperation { + ConstantBinaryArithmeticOperation() { + this instanceof BinaryArithmeticOperation and + this.(BinaryArithmeticOperation).getLeftOperand() instanceof Literal and + this.(BinaryArithmeticOperation).getRightOperand() instanceof Literal + } +} + +class IgnorableBinaryBitwiseOperation extends IgnorableOperation instanceof BinaryBitwiseOperation { +} + +class IgnorableUnaryBitwiseOperation extends IgnorableOperation instanceof UnaryBitwiseOperation { } + +class IgnorableAssignmentBitwiseOperation extends IgnorableOperation instanceof AssignBitwiseOperation +{ } + +/** + * Any arithmetic operation where one of the operands is a pointer or char type, ignore it + */ +class IgnorablePointerOrCharArithmetic extends IgnorableOperation { + IgnorablePointerOrCharArithmetic() { + this instanceof BinaryArithmeticOperation and + ( + this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof PointerType or - // If there is a data flow from the field that was modified to a function that seems to check for leap year - exists(VariableAccess vacheck, YearFieldAccess yfacheck, ChecksForLeapYearFunctionCall fc | - vacheck = var.getAnAccess() and - yfacheck.getQualifier() = vacheck and - LeapYearCheckFlow::flow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument())) - ) + this.(BinaryArithmeticOperation).getAnOperand().getUnspecifiedType() instanceof CharType or - // If there is a successor or predecessor that sets the month or day to a fixed value - exists(FieldAccess mfa, AssignExpr ae, int val | - mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess - | - mfa.getQualifier() = var.getAnAccess() and - mfa.isModified() and - ( - mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or - yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() - ) and - ae = mfa.getEnclosingElement() and - ae.getAnOperand().getValue().toInt() = val - ) + // Operations on calls to functions that accept char or char* + this.(BinaryArithmeticOperation) + .getAnOperand() + .(Call) + .getAnArgument() + .getUnspecifiedType() + .stripType() instanceof CharType + or + // Operations on calls to functions named like "strlen", "wcslen", etc + // NOTE: workaround for cases where the wchar_t type is not a char, but an unsigned short + // unclear if there is a best way to filter cases like these out based on type info. + this.(BinaryArithmeticOperation).getAnOperand().(Call).getTarget().getName().matches("%len%") ) + or + exists(AssignArithmeticOperation a | a.getRValue() = this | + a.getAnOperand().getUnspecifiedType() instanceof PointerType + or + a.getAnOperand().getUnspecifiedType() instanceof CharType + or + // Operations on calls to functions that accept char or char* + a.getAnOperand().(Call).getAnArgument().getUnspecifiedType().stripType() instanceof CharType + or + // Operations on calls to functions named like "strlen", "wcslen", etc + this.(BinaryArithmeticOperation).getAnOperand().(Call).getTarget().getName().matches("%len%") + ) + } +} + +/** + * An expression that is a candidate source for an dataflow configuration for an Operation that could flow to a Year field. + */ +predicate isOperationSourceCandidate(Expr e) { + not e instanceof IgnorableOperation and + not e.getEnclosingFunction() instanceof IgnorableFunction and + ( + e instanceof SubExpr + or + e instanceof AddExpr + or + e instanceof CrementOperation + or + exists(AssignSubExpr a | a.getRValue() = e) + or + exists(AssignAddExpr a | a.getRValue() = e) ) } /** - * The set of all write operations to the Year field of a date struct. + * A dataflow that tracks an ignorable operation (eg. bitwise op) to a operation source, so we may disqualify it. */ -abstract class YearWriteOp extends Operation { - /** Extracts the access to the Year field */ - abstract YearFieldAccess getYearAccess(); +module IgnorableOperationToOperationSourceCandidateConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { n.asExpr() instanceof IgnorableOperation } - /** Get the expression which represents the new value. */ - abstract Expr getMutationExpr(); + predicate isSink(DataFlow::Node n) { isOperationSourceCandidate(n.asExpr()) } + + // looking for sources and sinks in the same function + DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureEqualSourceSinkCallContext + } } +module IgnorableOperationToOperationSourceCandidateFlow = + TaintTracking::Global; + /** - * A unary operation (Crement) performed on a Year field. + * The set of all expressions which is a candidate expression and also does not flow from to to some ignorable expression (eg. bitwise op) + * ``` + * a = something <<< 2; + * myDate.year = a + 1; // invalid + * ... + * a = someDate.year + 1; + * myDate.year = a; // valid + * ``` */ -class YearWriteOpUnary extends YearWriteOp, UnaryOperation { - YearWriteOpUnary() { this.getOperand() instanceof YearFieldAccess } - - override YearFieldAccess getYearAccess() { result = this.getOperand() } +class OperationSource extends Expr { + OperationSource() { + isOperationSourceCandidate(this) and + // If the candidate came from an ignorable operation, ignore the candidate + // NOTE: we cannot easily flow the candidate to an ignorable operation as that can + // be tricky in practice, e.g., a mod operation on a year would be part of a leap year check + // but a mod operation ending in a year is more indicative of something to ignore (a conversion) + not exists(IgnorableOperationToOperationSourceCandidateFlow::PathNode sink | + sink.getNode().asExpr() = this and + sink.isSink() + ) + } +} - override Expr getMutationExpr() { result = this } +class YearFieldAssignmentNode extends DataFlow::Node { + YearFieldAssignmentNode() { + not this.getEnclosingCallable().getUnderlyingCallable() instanceof IgnorableFunction and + ( + this.asExpr() instanceof YearFieldAssignment or + this.asDefiningArgument() instanceof YearFieldOutArgAssignment + ) + } } /** - * An assignment operation or mutation on the Year field of a date object. + * An assignment to a year field, which will be a sink */ -class YearWriteOpAssignment extends YearWriteOp, Assignment { - YearWriteOpAssignment() { this.getLValue() instanceof YearFieldAccess } +abstract class YearFieldAssignment extends Expr { + abstract YearFieldAccess getYearFieldAccess(); +} - override YearFieldAccess getYearAccess() { result = this.getLValue() } +class YearFieldOutArgAssignment extends YearFieldAssignment { + YearFieldAccess access; - override Expr getMutationExpr() { - // Note: may need to use DF analysis to pull out the original value, - // if there is excessive false positives. - if this.getOperator() = "=" - then - exists(DataFlow::Node source, DataFlow::Node sink | - sink.asExpr() = this.getRValue() and - OperationToYearAssignmentFlow::flow(source, sink) and - result = source.asExpr() - ) - else result = this + YearFieldOutArgAssignment() { + exists(Call c | c.getAnArgument() = access and this = access) + or + exists(Call c, AddressOfExpr aoe | + c.getAnArgument() = aoe and + aoe.getOperand() = access and + this = aoe + ) + } + + override YearFieldAccess getYearFieldAccess() { result = access } +} + +/** + * An assignment to x ie `x = a`. + */ +class YearFieldAssignmentAccess extends YearFieldAssignment { + YearFieldAccess access; + + YearFieldAssignmentAccess() { + // TODO: why can't I make this just the L value? Doesn't seem to work + exists(Assignment a | + a.getLValue() = access and + a.getRValue() = this + ) } + + override YearFieldAccess getYearFieldAccess() { result = access } +} + +/** + * A year field assignment, by a unary operator ie `x++`. + */ +class YearFieldAssignmentUnary extends YearFieldAssignment instanceof CrementOperation { + YearFieldAccess access; + + YearFieldAssignmentUnary() { this.getOperand() = access } + + override YearFieldAccess getYearFieldAccess() { result = access } } /** @@ -110,32 +344,188 @@ class YearWriteOpAssignment extends YearWriteOp, Assignment { * to the Year field of a date object. */ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node n) { - not n.asExpr() instanceof Access and - not n.asExpr() instanceof Literal + predicate isSource(DataFlow::Node n) { n.asExpr() instanceof OperationSource } + + predicate isSink(DataFlow::Node n) { n instanceof YearFieldAssignmentNode } + + predicate isBarrier(DataFlow::Node n) { + exists(ArrayExpr arr | arr.getArrayOffset() = n.asExpr()) + or + n.asExpr().getUnspecifiedType() instanceof PointerType + or + n.asExpr().getUnspecifiedType() instanceof CharType + or + n.asExpr() instanceof IgnorableOperation + or + isLeapYearCheckSink(n) } - predicate isSink(DataFlow::Node n) { - exists(Assignment a | - a.getLValue() instanceof YearFieldAccess and - a.getRValue() = n.asExpr() + /** Block flow out of an operation source to get the "closest" operation to the sink */ + predicate isBarrierIn(DataFlow::Node n) { isSource(n) } + + predicate isBarrierOut(DataFlow::Node n) { isSink(n) } +} + +module OperationToYearAssignmentFlow = TaintTracking::Global; + +predicate isLeapYearCheckSink(DataFlow::Node sink) { + exists(ExprCheckLeapYear e | sink.asExpr() = e.getAChild*()) + or + // Local leap year check + sink.asExpr().(LeapYearFieldAccess).isUsedInCorrectLeapYearCheck() + or + // passed to function that seems to check for leap year + exists(ChecksForLeapYearFunctionCall fc | + sink.asExpr() = fc.getAnArgument() + or + sink.asExpr() = fc.getAnArgument().(FieldAccess).getQualifier() + or + exists(AddressOfExpr aoe | + fc.getAnArgument() = aoe and + aoe.getOperand() = sink.asExpr() + ) + ) +} + +/** + * A flow configuration from a Year field access to some Leap year check or guard + */ +module YearFieldAccessToLeapYearCheckConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { source instanceof YearFieldAssignmentNode } + + predicate isSink(DataFlow::Node sink) { isLeapYearCheckSink(sink) } + + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + // flow from a YearFieldAccess to the qualifier + node2.asExpr() = node1.asExpr().(YearFieldAccess).getQualifier() + } + + /** + * Enforcing the check must occur in the same call context as the source, + * i.e., do not return from the source function and check in a caller. + */ + DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } +} + +module YearFieldAccessToLeapYearCheckFlow = + TaintTracking::Global; + +/** Does there exist a flow from the given YearFieldAccess to a Leap Year check or guard? */ +predicate isYearModifiedWithCheck(YearFieldAccess fa) { + exists(YearFieldAccessToLeapYearCheckFlow::PathNode src | + src.isSource() and + src.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess() = fa + ) + or + // If the time flows to a time conversion whose value/result is checked, + // assume the leap year is being handled. + exists(TimeStructToCheckedTimeConversionFlow::PathNode timeQualSrc | + timeQualSrc.isSource() and + timeQualSrc.getNode().asExpr() = fa.getQualifier() + ) + // or + // isUsedInFeb29Check(fa) +} + +// /** +// * If there is a flow from a date struct year field access/assignment to a Feb 29 check +// */ +// predicate isUsedInFeb29Check(YearFieldAccess fa) { +// exists(DateFebruary29Check check | +// DataFlow::localExprFlow(fa.getQualifier(), check.getDateQualifier()) +// or +// DataFlow::localExprFlow(check.getDateQualifier(), fa.getQualifier()) +// ) +// } +/** + * An expression which checks the value of a Month field `a->month == 1`. + */ +class MonthEqualityCheck extends EqualityOperation { + MonthEqualityCheck() { this.getAnOperand() instanceof MonthFieldAccess } + + Expr getExprCompared() { + exists(Expr e | + e = this.getAnOperand() and + not e instanceof MonthFieldAccess and + result = e ) } } -module OperationToYearAssignmentFlow = DataFlow::Global; +class MonthEqualityCheckGuard extends GuardCondition instanceof MonthEqualityCheck { } + +/** + * Verifies if the expression is guarded by a check on the Month property of a date struct, that is NOT February. + */ +bindingset[e] +pragma[inline_late] +predicate isControlledByMonthEqualityCheckNonFebruary(Expr e) { + exists(MonthEqualityCheckGuard monthGuard | + monthGuard.controls(e.getBasicBlock(), true) and + not monthGuard.(MonthEqualityCheck).getExprCompared().getValueText() = "2" + ) +} + +module TimeStructToCheckedTimeConversionConfig implements DataFlow::StateConfigSig { + class FlowState = boolean; + + predicate isSource(DataFlow::Node source, FlowState state) { + exists(YearFieldAccess yfa | source.asExpr() = yfa.getQualifier()) and + state = false + } + + predicate isSink(DataFlow::Node sink, FlowState state) { + state = true and + exists(IfStmt ifs | ifs.getCondition().getAChild*() = sink.asExpr()) + } + + predicate isAdditionalFlowStep( + DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 + ) { + state1 in [true, false] and + state2 = true and + exists(Call c | + c.getTarget() instanceof TimeConversionFunction and + c.getAnArgument().getAChild*() = node1.asExpr() and + node2.asExpr() = c + ) + } + + DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } +} + +module TimeStructToCheckedTimeConversionFlow = + DataFlow::GlobalWithState; + +import OperationToYearAssignmentFlow::PathGraph -from Variable var, YearWriteOp ywo, Expr mutationExpr +from OperationToYearAssignmentFlow::PathNode src, OperationToYearAssignmentFlow::PathNode sink where - mutationExpr = ywo.getMutationExpr() and - isYearModifedWithoutExplicitLeapYearCheck(var, ywo) and - not isNormalizationOperation(mutationExpr) and - not ywo instanceof AddressOfExpr and - not exists(Call c, TimeConversionFunction f | f.getACallToThisFunction() = c | - c.getAnArgument().getAChild*() = var.getAnAccess() and - ywo.getASuccessor*() = c + OperationToYearAssignmentFlow::flowPath(src, sink) and + not isYearModifiedWithCheck(sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess()) and + not isControlledByMonthEqualityCheckNonFebruary(sink.getNode().asExpr()) and + // TODO: this is an older heuristic that should be reassessed. + // The heuristic stipulates that if a month or day is set to a constant in the local flow up to or after + // a modified year's sink, then assume the user is knowingly handling the conversion correctly. + // There is no interpretation of the assignment of the month/day or consideration for what branch the assignment is on. + // Merely if the assignment of a constant day/month occurs anywhere, the year modification can likely + // be ignored. + not exists(FieldAccess mfa, AssignExpr ae, YearFieldAccess yfa, int val, Variable var | + mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess + | + yfa = sink.getNode().asExpr().(YearFieldAssignment).getYearFieldAccess() and + var.getAnAccess() = yfa.getQualifier() and + mfa.getQualifier() = var.getAnAccess() and + mfa.isModified() and + ( + mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or + yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+() + ) and + ae = mfa.getEnclosingElement() and + ae.getAnOperand().getValue().toInt() = val and + // If the constant looks like it relates to february, then there still might be an error + // so only look for any other constant. + not val in [2, 28, 29] ) -select ywo, - "$@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found.", - ywo.getEnclosingFunction(), ywo.getEnclosingFunction().toString(), - ywo.getYearAccess().getTarget(), ywo.getYearAccess().getTarget().toString(), var, var.toString() +select sink, src, sink, + "Year field has been modified, but no appropriate check for LeapYear was found." diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected index 555002b40867..aa5e527e0759 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected @@ -1,8 +1,61 @@ -| test.cpp:617:2:617:11 | ... ++ | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:611:6:611:32 | AntiPattern_1_year_addition | AntiPattern_1_year_addition | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:613:13:613:14 | st | st | -| test.cpp:634:2:634:25 | ... += ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:627:6:627:32 | AntiPattern_simple_addition | AntiPattern_simple_addition | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:629:13:629:14 | st | st | -| test.cpp:763:2:763:19 | ... ++ | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:756:6:756:40 | AntiPattern_year_addition_struct_tm | AntiPattern_year_addition_struct_tm | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:759:12:759:19 | timeinfo | timeinfo | -| test.cpp:800:2:800:40 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:793:12:793:19 | timeinfo | timeinfo | -| test.cpp:803:2:803:43 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:793:12:793:19 | timeinfo | timeinfo | -| test.cpp:808:2:808:24 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:794:13:794:14 | st | st | -| test.cpp:811:2:811:33 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:791:6:791:23 | FalseNegativeTests | FalseNegativeTests | test.cpp:12:7:12:11 | wYear | wYear | test.cpp:794:13:794:14 | st | st | -| test.cpp:850:3:850:36 | ... = ... | $@: Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found. | test.cpp:818:6:818:23 | tp_intermediaryVar | tp_intermediaryVar | test.cpp:56:6:56:12 | tm_year | tm_year | test.cpp:70:18:70:19 | tm | tm | +#select +| test.cpp:422:14:422:14 | 2 | test.cpp:422:14:422:14 | 2 | test.cpp:422:14:422:14 | 2 | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:440:2:440:11 | ... ++ | test.cpp:440:2:440:11 | ... ++ | test.cpp:440:2:440:11 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:456:2:456:12 | ... ++ | test.cpp:456:2:456:12 | ... ++ | test.cpp:456:2:456:12 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:681:14:681:23 | yearsToAdd | test.cpp:681:14:681:23 | yearsToAdd | test.cpp:681:14:681:23 | yearsToAdd | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:769:22:769:23 | 80 | test.cpp:769:22:769:23 | 80 | test.cpp:769:22:769:23 | 80 | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:813:21:813:40 | ... + ... | test.cpp:813:21:813:40 | ... + ... | test.cpp:813:21:813:40 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:818:13:818:24 | ... + ... | test.cpp:818:13:818:24 | ... + ... | test.cpp:818:13:818:24 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:857:33:857:36 | year | test.cpp:854:14:854:31 | ... + ... | test.cpp:857:33:857:36 | year | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:857:33:857:36 | year | test.cpp:854:35:854:40 | -- ... | test.cpp:857:33:857:36 | year | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:954:14:954:25 | ... + ... | test.cpp:954:14:954:25 | ... + ... | test.cpp:954:14:954:25 | ... + ... | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:972:3:972:12 | ... ++ | test.cpp:972:3:972:12 | ... ++ | test.cpp:972:3:972:12 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1075:2:1075:11 | ... ++ | test.cpp:1075:2:1075:11 | ... ++ | test.cpp:1075:2:1075:11 | ... ++ | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1095:16:1095:23 | increment_arg output argument | test.cpp:1083:2:1083:4 | ... ++ | test.cpp:1095:16:1095:23 | increment_arg output argument | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1099:27:1099:35 | increment_arg_by_pointer output argument | test.cpp:1087:2:1087:7 | ... ++ | test.cpp:1099:27:1099:35 | increment_arg_by_pointer output argument | Year field has been modified, but no appropriate check for LeapYear was found. | +| test.cpp:1153:14:1153:26 | ... - ... | test.cpp:1153:14:1153:26 | ... - ... | test.cpp:1153:14:1153:26 | ... - ... | Year field has been modified, but no appropriate check for LeapYear was found. | +edges +| test.cpp:854:7:854:31 | ... = ... | test.cpp:857:33:857:36 | year | provenance | | +| test.cpp:854:14:854:31 | ... + ... | test.cpp:854:7:854:31 | ... = ... | provenance | | +| test.cpp:854:35:854:40 | -- ... | test.cpp:857:33:857:36 | year | provenance | | +| test.cpp:1082:26:1082:26 | *x | test.cpp:1095:16:1095:23 | increment_arg output argument | provenance | | +| test.cpp:1083:2:1083:4 | ... ++ | test.cpp:1082:26:1082:26 | *x | provenance | | +| test.cpp:1086:37:1086:37 | *x | test.cpp:1099:27:1099:35 | increment_arg_by_pointer output argument | provenance | | +| test.cpp:1087:2:1087:7 | ... ++ | test.cpp:1086:37:1086:37 | *x | provenance | | +nodes +| test.cpp:422:14:422:14 | 2 | semmle.label | 2 | +| test.cpp:440:2:440:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:456:2:456:12 | ... ++ | semmle.label | ... ++ | +| test.cpp:482:3:482:12 | ... ++ | semmle.label | ... ++ | +| test.cpp:500:2:500:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:526:14:526:23 | yearsToAdd | semmle.label | yearsToAdd | +| test.cpp:553:2:553:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:592:2:592:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:647:2:647:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:665:14:665:25 | yearAddition | semmle.label | yearAddition | +| test.cpp:681:14:681:23 | yearsToAdd | semmle.label | yearsToAdd | +| test.cpp:744:2:744:19 | ... ++ | semmle.label | ... ++ | +| test.cpp:769:22:769:23 | 80 | semmle.label | 80 | +| test.cpp:792:2:792:19 | ... ++ | semmle.label | ... ++ | +| test.cpp:813:21:813:40 | ... + ... | semmle.label | ... + ... | +| test.cpp:818:13:818:24 | ... + ... | semmle.label | ... + ... | +| test.cpp:854:7:854:31 | ... = ... | semmle.label | ... = ... | +| test.cpp:854:14:854:31 | ... + ... | semmle.label | ... + ... | +| test.cpp:854:35:854:40 | -- ... | semmle.label | -- ... | +| test.cpp:857:33:857:36 | year | semmle.label | year | +| test.cpp:875:4:875:15 | ... ++ | semmle.label | ... ++ | +| test.cpp:954:14:954:25 | ... + ... | semmle.label | ... + ... | +| test.cpp:972:3:972:12 | ... ++ | semmle.label | ... ++ | +| test.cpp:990:3:990:12 | ... ++ | semmle.label | ... ++ | +| test.cpp:1075:2:1075:11 | ... ++ | semmle.label | ... ++ | +| test.cpp:1082:26:1082:26 | *x | semmle.label | *x | +| test.cpp:1083:2:1083:4 | ... ++ | semmle.label | ... ++ | +| test.cpp:1086:37:1086:37 | *x | semmle.label | *x | +| test.cpp:1087:2:1087:7 | ... ++ | semmle.label | ... ++ | +| test.cpp:1095:16:1095:23 | increment_arg output argument | semmle.label | increment_arg output argument | +| test.cpp:1099:27:1099:35 | increment_arg_by_pointer output argument | semmle.label | increment_arg_by_pointer output argument | +| test.cpp:1133:3:1133:15 | -- ... | semmle.label | -- ... | +| test.cpp:1153:14:1153:26 | ... - ... | semmle.label | ... - ... | +subpaths +testFailures +| test.cpp:1075:2:1075:11 | ... ++ | Unexpected result: Alert | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref index 22a30d72b689..12bb5eb1e69b 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref @@ -1 +1,2 @@ -Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +query: Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql \ No newline at end of file diff --git a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp index beb2c4061496..3bd830121d03 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp @@ -46,6 +46,8 @@ typedef struct _TIME_DYNAMIC_ZONE_INFORMATION { BOOLEAN DynamicDaylightTimeDisabled; } DYNAMIC_TIME_ZONE_INFORMATION, *PDYNAMIC_TIME_ZONE_INFORMATION; +typedef const wchar_t* LPCWSTR; + struct tm { int tm_sec; // seconds after the minute - [0, 60] including leap second @@ -71,6 +73,30 @@ struct logtime { long usec; }; +/* + * Data structure representing a broken-down timestamp. + * + * CAUTION: the IANA timezone library (src/timezone/) follows the POSIX + * convention that tm_mon counts from 0 and tm_year is relative to 1900. + * However, Postgres' datetime functions generally treat tm_mon as counting + * from 1 and tm_year as relative to 1 BC. Be sure to make the appropriate + * adjustments when moving from one code domain to the other. + */ +struct pg_tm +{ + int tm_sec; + int tm_min; + int tm_hour; + int tm_mday; + int tm_mon; /* see above */ + int tm_year; /* see above */ + int tm_wday; + int tm_yday; + int tm_isdst; + long int tm_gmtoff; + const char *tm_zone; +}; + BOOL SystemTimeToFileTime( const SYSTEMTIME* lpSystemTime, @@ -111,6 +137,10 @@ TzSpecificLocalTimeToSystemTimeEx( LPSYSTEMTIME lpUniversalTime ); +int _wtoi( + const wchar_t *str +); + void GetSystemTime( LPSYSTEMTIME lpSystemTime ); @@ -389,7 +419,7 @@ void AntiPattern_unchecked_filetime_conversion2a() GetSystemTime(&st); // BUG - UncheckedLeapYearAfterYearModification - st.wYear += 2; + st.wYear += 2; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // BUG - UncheckedReturnValueForTimeFunctions SystemTimeToFileTime(&st, &ft); @@ -407,7 +437,7 @@ void AntiPattern_unchecked_filetime_conversion2b() GetSystemTime(&st); // BUG - UncheckedLeapYearAfterYearModification - st.wYear++; + st.wYear++; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // BUG - UncheckedReturnValueForTimeFunctions SystemTimeToFileTime(&st, &ft); @@ -423,7 +453,7 @@ void AntiPattern_unchecked_filetime_conversion2b(SYSTEMTIME* st) FILETIME ft; // BUG - UncheckedLeapYearAfterYearModification - st->wYear++; + st->wYear++; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // BUG - UncheckedReturnValueForTimeFunctions SystemTimeToFileTime(st, &ft); @@ -605,24 +635,26 @@ void CorrectPattern_NotManipulated_DateFromAPI_1(HANDLE hWatchdog) ///////////////////////////////////////////////////////////////// /** - * Positive Case - Anti-pattern 1: [year ±n, month, day] - * Years is incremented by some integer but a leap year is not handled. + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Year is incremented by some integer and checked through a conversion through an inter procedural function check */ void AntiPattern_1_year_addition() { SYSTEMTIME st; GetSystemTime(&st); - // BUG - UncheckedLeapYearAfterYearModification - st.wYear++; // BUg V2 + // Safe, checked interprocedurally through Correct_filetime_conversion_check + st.wYear++; // Usage of potentially invalid date Correct_filetime_conversion_check(st); } + + /** - * Positive Case - Anti-pattern 1: [year ±n, month, day] - * Years is incremented by some integer but a leap year is not handled. + * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Years is incremented by some integer and checked through a conversion through an inter procedural function check */ void AntiPattern_simple_addition(int yearAddition) { @@ -630,8 +662,7 @@ void AntiPattern_simple_addition(int yearAddition) GetSystemTime(&st); - // BUG - UncheckedLeapYearAfterYearModification - st.wYear += yearAddition; // Bug V2 + st.wYear += yearAddition; // Usage of potentially invalid date Correct_filetime_conversion_check(st); @@ -647,7 +678,7 @@ void AntiPattern_IncorrectGuard(int yearsToAdd) GetSystemTime(&st); // BUG - UncheckedLeapYearAfterYearModification - st.wYear += yearsToAdd; + st.wYear += yearsToAdd; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] // Incorrect Guard if (st.wMonth == 2 && st.wDay == 29) @@ -660,9 +691,6 @@ void AntiPattern_IncorrectGuard(int yearsToAdd) st.wDay = 28; } } - - // Potentially Unsafe to use - Correct_filetime_conversion_check(st); } /************************************************* @@ -725,10 +753,10 @@ void Correct_year_addition_struct_tm() } /** - * Negative Case - Anti-pattern 1: [year ±n, month, day] + * Positive Case - Anti-pattern 1: [year ±n, month, day] * Years is incremented by some integer and leap year is not handled correctly. */ -void Correct_LinuxPattern() +void Incorrect_LinuxPattern() { time_t rawtime; struct tm timeinfo; @@ -737,7 +765,8 @@ void Correct_LinuxPattern() errno_t err = gmtime_s(&timeinfo, &rawtime); /* from 1900 -> from 1980 */ - timeinfo.tm_year -= 80; + // BUG - UncheckedLeapYearAfterYearModification + timeinfo.tm_year -= 80; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] /* 0~11 -> 1~12 */ timeinfo.tm_mon++; /* 0~59 -> 0~29(2sec counts) */ @@ -751,7 +780,8 @@ void Correct_LinuxPattern() /** * Negative Case - Anti-pattern 1: [year ±n, month, day] - * Years is incremented by some integer and leap year is not handled correctly. + * Years is incremented by some integer and leap year is assumed checked through + * check of a conversion functions return value. */ void AntiPattern_year_addition_struct_tm() { @@ -759,36 +789,19 @@ void AntiPattern_year_addition_struct_tm() struct tm timeinfo; time(&rawtime); gmtime_s(&timeinfo, &rawtime); - // BUG - UncheckedLeapYearAfterYearModification - timeinfo.tm_year++; // Bug V2 + timeinfo.tm_year++; - // Usage of potentially invalid date + // mkgmtime result checked in nested call here, assume leap year conversion is potentially handled CorrectUsageOf_mkgmtime(timeinfo); } ///////////////////////////////////////////////////////// -/** - * Negative Case - Anti-pattern 1: [year ±n, month, day] - * False positive: Years is initialized to or incremented by some integer (but never used). -*/ -void FalsePositiveTests(int x) -{ - struct tm timeinfo; - SYSTEMTIME st; - - timeinfo.tm_year = x; - timeinfo.tm_year = 1970; - - st.wYear = x; - st.wYear = 1900 + x; -} /** * Positive Case - Anti-pattern 1: [year ±n, month, day] - * False positive: Years is initialized to or incremented by some integer (but never used). */ -void FalseNegativeTests(int x) +void test(int x) { struct tm timeinfo; SYSTEMTIME st; @@ -797,18 +810,12 @@ void FalseNegativeTests(int x) // BUG - UncheckedLeapYearAfterYearModification // Positive Case - Anti-pattern 1: [year ±n, month, day] - timeinfo.tm_year = x + timeinfo.tm_year; // Bug V2 - // BUG - UncheckedLeapYearAfterYearModification - // Positive Case - Anti-pattern 1: [year ±n, month, day] - timeinfo.tm_year = 1970 + timeinfo.tm_year; // Bug V2 + timeinfo.tm_year = x + timeinfo.tm_year; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] st.wYear = x; // BUG - UncheckedLeapYearAfterYearModification // Positive Case - Anti-pattern 1: [year ±n, month, day] - st.wYear = x + st.wYear; // Bug V2 - // BUG - UncheckedLeapYearAfterYearModification - // Positive Case - Anti-pattern 1: [year ±n, month, day] - st.wYear = (1986 + st.wYear) - 1; // Bug V2 + st.wYear = x + st.wYear; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] } /** @@ -844,10 +851,10 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) timestamp_remote.tm = tm_parsed; timestamp_remote.tm.tm_isdst = -1; timestamp_remote.usec = now.tv_nsec * 0.001; - for (year = tm_now.tm_year + 1;; --year) + for (year = tm_now.tm_year + 1;; --year) // $ Source { // assert(year >= tm_now.tm_year - 1); - timestamp_remote.tm.tm_year = year; + timestamp_remote.tm.tm_year = year; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] if (mktime(×tamp_remote.tm) < t_now + 7 * 24 * 60 * 60) break; } @@ -943,7 +950,8 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) GetSystemTime(&st); - st.wYear = st.wYear + 1; // BAD + // BUG - UncheckedLeapYearAfterYearModification + st.wYear = st.wYear + 1; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] SystemTimeToFileTime(&st, &ft); } @@ -960,14 +968,16 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) GetSystemTime(&st); - st.wYear++; // BAD Positive Case - Anti-pattern 1: [year ±n, month, day] + // BUG - UncheckedLeapYearAfterYearModification + st.wYear++; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] SystemTimeToFileTime(&st, &ft); } /** * Negative Case - Anti-pattern 1: [year ±n, month, day] - * Modification of SYSTEMTIME struct adding to year but no leap year guard is conducted. + * Modification of SYSTEMTIME struct adding to year but value passed to a + * conversion function that can be checked for success, and the result is checked. */ void modified5() { @@ -977,8 +987,13 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) GetSystemTime(&st); - st.wYear++; // Negative Case - Anti-pattern 1: [year ±n, month, day], guard condition below. + st.wYear++; + // Presumed safe usage, as if the conversion is incorrect, a user can handle the error. + // NOTE: it doesn't mean the user actually does the correct conversion and it it also + // doesn't mean it will error our in all cases that may be invalid. + // For example, if a leap year and the date is 28, we may want 29 if the time is meant + // to capture the end of the month, but 28 is still valid and will not error out. if (SystemTimeToFileTime(&st, &ft)) { ///... @@ -1004,3 +1019,136 @@ bool tp_intermediaryVar(struct timespec now, struct logtime ×tamp_remote) tm.tm_mday = tm.tm_mon == 2 && tm.tm_mday == 29 && !isLeapYear ? 28 : tm.tm_mday; return tm; } + +/** +* Negative Case - Anti-pattern 1: [year ±n, month, day] +* Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed. +*/ +bool +FMAPITimeToSysTimeW(LPCWSTR wszTime, SYSTEMTIME *psystime) +{ + // if (!wszTime || SafeIsBadReadPtr(wszTime, 1) || lstrlenW(wszTime) != cchMAPITime) + // return false; + // AssertTag(!SafeIsBadWritePtr(psystime, sizeof(SYSTEMTIME)), 0x0004289a /* tag_abc80 */); + // memset(psystime, 0, sizeof(SYSTEMTIME)); + + psystime->wYear = (WORD)_wtoi(wszTime); + psystime->wMonth = (WORD)_wtoi(wszTime+5); + psystime->wDay = (WORD)_wtoi(wszTime+8); + psystime->wHour = (WORD)_wtoi(wszTime+11); + psystime->wMinute = (WORD)_wtoi(wszTime+14); + return true; +} + +/** +* Negative Case - Anti-pattern 1: [year ±n, month, day] +* Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed. +*/ +bool +ATime_HrGetSysTime(SYSTEMTIME *pst) +{ + // if (!FValidSysTime()) + // { + // TrapSzTag("ATime cannot be converted to SYSTEMTIME", 0x1e14f5c3 /* tag_4fpxd */); + // CORgTag(E_FAIL, 0x6c373230 /* tag_l720 */); + // } + + // pst->wYear = static_cast(m_lYear); + // pst->wMonth = static_cast(m_lMonth); + // //pst->wDayOfWeek = ???; + // pst->wDay = static_cast(m_lDay); + // pst->wHour = static_cast(m_lHour); + // pst->wMinute = static_cast(m_lMinute); + // pst->wSecond = static_cast(m_lSecond); + // pst->wMilliseconds = 0; +} + +/** +* Negative Case - Anti-pattern 1: [year ±n, month, day] +* Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed. +*/ +void fp_daymonth_guard(){ + SYSTEMTIME st; + FILETIME ft; + GetSystemTime(&st); + + st.wYear++; + + st.wDay = st.wMonth == 2 && st.wDay == 29 ? 28 : st.wDay; + + SystemTimeToFileTime(&st, &ft); +} + +void increment_arg(WORD &x){ + x++; // $ Source +} + +void increment_arg_by_pointer(WORD *x){ + (*x)++; // $ Source +} + + +void fn_year_set_through_out_arg(){ + SYSTEMTIME st; + GetSystemTime(&st); + // BAD, year incremented without check + increment_arg(st.wYear); // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] + + // GetSystemTime(&st); + // Bad, year incremented without check + increment_arg_by_pointer(&st.wYear); // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] +} + + +/* TODO: don't alert on simple copies from another struct where all three {year,month,day} are copied +void +GetEpochTime(struct pg_tm *tm) +{ + struct pg_tm *t0; + pg_time_t epoch = 0; + + t0 = pg_gmtime(&epoch); + + tm->tm_year = t0->tm_year; + tm->tm_mon = t0->tm_mon; + tm->tm_mday = t0->tm_mday; + tm->tm_hour = t0->tm_hour; + tm->tm_min = t0->tm_min; + tm->tm_sec = t0->tm_sec; + + tm->tm_year += 1900; + tm->tm_mon++; +} */ + +void +fp_guarded_by_month(struct pg_tm *tm){ + int woy = 52; + int MONTHS_PER_YEAR = 12; + /* + * If it is week 52/53 and the month is January, then the + * week must belong to the previous year. Also, some + * December dates belong to the next year. + */ + if (woy >= 52 && tm->tm_mon == 1) + --tm->tm_year; // Negative Test Case + if (woy <= 1 && tm->tm_mon == MONTHS_PER_YEAR) + ++tm->tm_year; // Negative Test Case +} + +typedef unsigned short CSHORT; + +typedef struct _TIME_FIELDS { + CSHORT Year; + CSHORT Month; + CSHORT Day; + CSHORT Hour; + CSHORT Minute; + CSHORT Second; + CSHORT Milliseconds; + CSHORT Weekday; +} TIME_FIELDS, *PTIME_FIELDS; + +void +tp_ptime(PTIME_FIELDS ptm){ + ptm->Year = ptm->Year - 1; // $ Alert[cpp/microsoft/public/leap-year/unchecked-after-arithmetic-year-modification] +} \ No newline at end of file