Addressed code review comments

Author: Denis Levin

csharp/ql/src/Likely Bugs/LeapYear/UnsafeYearConstruction.qhelp

@@ -3,22 +3,21 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-  <include src="LeapYear.qhelp" />
-  
-<p>When creating or creating a <code>System.DateTime</code> object by setting the year, month, day in the contructor (other parameters optional) by performing an arithmetic operation on a different <code>DateTime</code> object, there is a risk that the date you are setting is an invalid one.</p>
-<p>On a leap year, such code may throw an <code>ArgumentOutOfRangeException</code> with a message of <code>"Year, Month, and Day parameters describe an un-representable DateTime."</code></p>
-
+  <p>When creating a <code>System.DateTime</code> object by setting the year, month, and day in the constructor by performing an arithmetic operation on a different <code>DateTime</code> object, there is a risk that the date you are setting is invalid.</p>
+  <p>On a leap year, such code may throw an <code>ArgumentOutOfRangeException</code> with a message of <code>"Year, Month, and Day parameters describe an un-representable DateTime."</code></p>
 </overview>
 <recommendation>
-<p>Creating a <code>System.DateTime</code> object based on a different <code>System.DateTime</code> object, use the appropriate methods to manipulate the date instead of arithmetic.</p>
-
+  <p>Creating a <code>System.DateTime</code> object based on a different <code>System.DateTime</code> object, use the appropriate methods to manipulate the date instead of arithmetic.</p>
 </recommendation>
 <example>
-<p>In this example, we are adding/decreasing 1 year to the current date when creating a new <code>System.DateTime</code> object. This may work most of the time, but on any given February 29th, the resulting value will be invalid.</p>
-<sample src="UnsafeYearConstructionBad.cs" />
-
-<p>To fix this bug, we add/substract years to the current date by calling <code>AddYears</code> method on it.</p>
-<sample src="UnsafeYearConstructionGood.cs" />
+  <p>In this example, we are incrementing/decrementing the current date by one year when creating a new <code>System.DateTime</code> object. This may work most of the time, but on any given February 29th, the resulting value will be invalid.</p>
+  <sample src="UnsafeYearConstructionBad.cs" />
+  <p>To fix this bug, we add/substract years to the current date by calling <code>AddYears</code> method on it.</p>
+  <sample src="UnsafeYearConstructionGood.cs" />
 </example>
-
+<references>
+  <li>
+    <a href="https://docs.microsoft.com/en-us/dotnet/api/system.datetime?view=netframework-4.8">System.DateTime Struct</a>.
+  </li>
+</references>
 </qhelp>

csharp/ql/src/Likely Bugs/LeapYear/UnsafeYearConstruction.ql

@@ -1,35 +1,39 @@
 /**
- * @name Unsafe year argument for DateTime constructor
- * @description Constructing a DateTime object by setting the year argument by manipulating the year of a different DateTime object
- * @kind problem
+ * @name Unsafe year argument for 'DateTime' constructor
+ * @description Constructing a 'DateTime' struct by setting the year argument to an increment or decrement of the year of a different 'DateTime' struct
+ * @kind path-problem
  * @problem.severity error
- * @id cs/leap-year/unsafe-year-contruction
+ * @id cs/unsafe-year-construction
  * @precision high
  * @tags security
- *       leap-year
+ *       date-time
+ *       reliability
  */
 
 import csharp
+import DataFlow::PathGraph
 import semmle.code.csharp.dataflow.TaintTracking
 
-class UnsafeYearCreationFromArithmeticConfiguration extends TaintTracking::Configuration  {
-  UnsafeYearCreationFromArithmeticConfiguration() { this = "UnsafeYearCreationFromArithmeticConfiguration" }
+class UnsafeYearCreationFromArithmeticConfiguration extends TaintTracking::Configuration {
+  UnsafeYearCreationFromArithmeticConfiguration() {
+    this = "UnsafeYearCreationFromArithmeticConfiguration"
+  }
 
-  override predicate isSource(DataFlow::Node source) { 
-    exists( ArithmeticOperation ao, PropertyAccess pa |
-      ao = source.asExpr() | 
-      pa = ao.getAChild*()
-      and pa.getProperty().getQualifiedName().matches("%DateTime.Year")
+  override predicate isSource(DataFlow::Node source) {
+    exists(ArithmeticOperation ao, PropertyAccess pa | ao = source.asExpr() |
+      pa = ao.getAChild*() and
+      pa.getProperty().getQualifiedName().matches("System.DateTime.Year")
     )
   }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists( ObjectCreation oc |
-      sink.asExpr() = oc.getArgumentForName("year")
-      and oc.getObjectType().getABaseType*().hasQualifiedName("System.DateTime"))
+    exists(ObjectCreation oc |
+      sink.asExpr() = oc.getArgumentForName("year") and
+      oc.getObjectType().getABaseType*().hasQualifiedName("System.DateTime")
+    )
   }
 }
 
-from UnsafeYearCreationFromArithmeticConfiguration config, Expr sink, Expr source
-where config.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(sink))
-select sink, "This $@ based on a System.DateTime.Year property is used in a construction of a new System.DateTime object, flowing to the 'year' argument.", source, "arithmetic operation" 
+from UnsafeYearCreationFromArithmeticConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink, source, sink, "This $@ based on a 'System.DateTime.Year' property is used in a construction of a new 'System.DateTime' object, flowing to the 'year' argument.", source, "arithmetic operation"

csharp/ql/src/Likely Bugs/LeapYear/UnsafeYearConstructionBad.cs

@@ -1,13 +1,11 @@
 using System;
-
 public class UnsafeYearConstructionBad
 {
     public UnsafeYearConstructionBad()
     {
         DateTime Start;
         DateTime End;
         var now = DateTime.UtcNow;
-
         // the base-date +/1 n years may not be a valid date.
         Start = new DateTime(now.Year - 1, now.Month, now.Day, 0, 0, 0, DateTimeKind.Utc);
         End = new DateTime(now.Year + 1, now.Month, now.Day, 0, 0, 1, DateTimeKind.Utc);

csharp/ql/src/Likely Bugs/LeapYear/UnsafeYearConstructionGood.cs

@@ -1,13 +1,11 @@
 using System;
-
 public class UnsafeYearConstructionBad
 {
     public UnsafeYearConstructionBad()
     {
         DateTime Start;
         DateTime End;
         var now = DateTime.UtcNow;
-
         Start = now.AddYears(-1).Date;
         End = now.AddYears(-1).Date.AddSeconds(1);
     }

csharp/ql/src/Likely Bugs/MishandlingJapaneseEra.cs

@@ -1,39 +1,21 @@
 using  System;
-
 using System.Globalization;
-
 public class Example
-
 {
-
    public static void Main()
-
    {
-
       var cal = new JapaneseCalendar();
-
       // constructing date using current era 
       var dat = cal.ToDateTime(2, 1, 2, 0, 0, 0, 0);
-
       Console.WriteLine($"{dat:s}");
-
       // constructing date using current era 
       dat = new DateTime(2, 1, 2, cal);
-
       Console.WriteLine($"{dat:s}");
-
    }
-
 }
-
 // Output with the Heisei era current:
-
 //      1990-01-02T00:00:00
-
 //      1990-01-02T00:00:00
-
 // Output with the new era current:
-
 //      2020-01-02T00:00:00
-
 //      2020-01-02T00:00:00

csharp/ql/src/Likely Bugs/MishandlingJapaneseEra.qhelp

@@ -24,10 +24,13 @@
     <a href="https://devblogs.microsoft.com/dotnet/handling-a-new-era-in-the-japanese-calendar-in-net/">Handling a new era in the Japanese calendar in .NET</a>.
   </li>
   <li>
-    <a href="https://blogs.msdn.microsoft.com/shawnste/2018/04/12/the-japanese-calendars-y2k-moment/">The Japanese Calendar’s Y2K Moment</a>.
+    <a href="https://blogs.msdn.microsoft.com/shawnste/2018/04/12/the-japanese-calendars-y2k-moment/">The Japanese Calendar's Y2K Moment</a>.
   </li>
   <li>
     <a href="https://docs.microsoft.com/en-us/windows/desktop/Intl/era-handling-for-the-japanese-calendar/">Era Handling for the Japanese Calendar</a>.
   </li>
+  <li>
+   <a href="https://simple.wikipedia.org/wiki/List_of_Japanese_eras">List of Japanese Eras (Wikipedia)</a>
+  </li>
 </references>
 </qhelp>

csharp/ql/src/Likely Bugs/MishandlingJapaneseEra.ql

@@ -1,60 +1,72 @@
 /**
- * @name MishandlingJapaneseEraStartDate
- * @description Japanese Era should be handled with built-in JapaneseCalendar class. Aviod hard-coding Japanese era start dates and names. 
+ * @name Mishandling the Japanese era start date
+ * @description Japanese era should be handled with the built-in 'JapaneseCalendar' class. Avoid hard-coding Japanese era start dates and names.
  * @id cs/mishandling-japanese-era
  * @kind problem
  * @problem.severity warning
  * @precision medium
  * @tags reliability
- *       japanese-era
+ *       date-time
  */
 
 import csharp
 
+/**
+ * Holds if `year`, `month`, and `day` specify the start of a new era
+ * (see https://simple.wikipedia.org/wiki/List_of_Japanese_eras).
+ */
+predicate isEraStart(int year, int month, int day) {
+  year = 1989 and month = 1 and day = 8
+  or
+  year = 2019 and month = 5 and day = 1
+}
+
 predicate isExactEraStartDateCreation(ObjectCreation cr) {
-  (cr.getType().hasQualifiedName("System.DateTime") or cr.getType().hasQualifiedName("System.DateTimeOffset"))
-  and  
-    cr.getArgument(0).getValue() = "1989" and
-    cr.getArgument(1).getValue() = "1" and
-    cr.getArgument(2).getValue() = "8"
+  (
+    cr.getType().hasQualifiedName("System.DateTime") or
+    cr.getType().hasQualifiedName("System.DateTimeOffset")
+  ) and
+  isEraStart(cr.getArgument(0).getValue().toInt(), cr.getArgument(1).getValue().toInt(), cr.getArgument(2).getValue().toInt())
 }
 
 predicate isDateFromJapaneseCalendarToDateTime(MethodCall mc) {
-  (mc.getQualifier().getType().hasQualifiedName("System.Globalization.JapaneseCalendar") or mc.getQualifier().getType().hasQualifiedName("System.Globalization.JapaneseLunisolarCalendar"))
-  and 
-    mc.getTarget().hasName("ToDateTime") and
-    mc.getArgument(0).getValue() != "" and
-    (mc.getNumberOfArguments() = 7 or // implicitly current era
-     mc.getNumberOfArguments() = 8 and
-     mc.getArgument(7).getValue() = "0") // explicitly current era
+  (
+    mc.getQualifier().getType().hasQualifiedName("System.Globalization.JapaneseCalendar") or
+    mc.getQualifier().getType().hasQualifiedName("System.Globalization.JapaneseLunisolarCalendar")
+  ) and
+  mc.getTarget().hasName("ToDateTime") and
+  mc.getArgument(0).hasValue() and
+  (
+    mc.getNumberOfArguments() = 7 // implicitly current era
+    or
+    mc.getNumberOfArguments() = 8 and
+    mc.getArgument(7).getValue() = "0"
+  ) // explicitly current era
 }
 
 predicate isDateFromJapaneseCalendarCreation(ObjectCreation cr) {
-  (cr.getType().hasQualifiedName("System.DateTime") or cr.getType().hasQualifiedName("System.DateTimeOffset")) and
-  (cr.getArgumentForName("calendar").getType().hasQualifiedName("System.Globalization.JapaneseCalendar") or cr.getArgumentForName("calendar").getType().hasQualifiedName("System.Globalization.JapaneseLunisolarCalendar"))
-  and  
-    cr.getArgumentForName("year").getValue() != ""
-}
-
-predicate inEraArrayCreation(ArrayInitializer ai) {
-  ai.getElement(0).getValue() = "1867" and
-  ai.getElement(1).getValue() = "1911" and
-  ai.getElement(2).getValue() = "1925" and
-  ai.getElement(3).getValue() = "1988"
-}
-
-predicate isEraCollectionCreation(CollectionInitializer cs) {
-  cs.getElementInitializer(0).getValue() = "1867" and
-  cs.getElementInitializer(1).getValue() = "1911" and
-  cs.getElementInitializer(2).getValue() = "1925" and
-  cs.getElementInitializer(3).getValue() = "1988"
+  (
+    cr.getType().hasQualifiedName("System.DateTime") or
+    cr.getType().hasQualifiedName("System.DateTimeOffset")
+  ) and
+  (
+    cr
+        .getArgumentForName("calendar")
+        .getType()
+        .hasQualifiedName("System.Globalization.JapaneseCalendar") or
+    cr
+        .getArgumentForName("calendar")
+        .getType()
+        .hasQualifiedName("System.Globalization.JapaneseLunisolarCalendar")
+  ) and
+  cr.getArgumentForName("year").hasValue()
 }
 
 from Expr expr, string message
-where 
-  isDateFromJapaneseCalendarToDateTime(expr) and message = "DateTime created from Japanese calendar with explicit or current era and hard-coded year" or
-  isDateFromJapaneseCalendarCreation(expr) and message = "DateTime constructed from Japanese calendar with explicit or current era and hard-coded year" or
-  isEraCollectionCreation(expr) and message = "Hard-coded collection with Japanese era years" or
-  inEraArrayCreation (expr) and message = "Hard-coded array with Japanese era years" or
-  isExactEraStartDateCreation(expr) and message = "Hard-coded the beginning of the Japanese Heisei era"
+where
+  isDateFromJapaneseCalendarToDateTime(expr) and message = "'DateTime' created from Japanese calendar with explicit or current era and hard-coded year."
+  or
+  isDateFromJapaneseCalendarCreation(expr) and message = "'DateTime' constructed from Japanese calendar with explicit or current era and hard-coded year."
+  or
+  isExactEraStartDateCreation(expr) and message = "Hard-coded the beginning of the Japanese Heisei era."
 select expr, message

csharp/ql/test/query-tests/Likely Bugs/LeapYear/AntiPattern1/MishandlingJapaneseEra.expected

@@ -9,20 +9,20 @@ public class PipelineProperties
         public PipelineProperties()
         {
             var now = DateTime.UtcNow;
-            // BUG
+            // BAD
             this.Start = new DateTime(now.Year - 1, now.Month, now.Day, 0, 0, 0, DateTimeKind.Utc);
 
             var endYear = now.Year + 1;
-            // BUG
+            // BAD
             this.End = new DateTime(endYear, now.Month, now.Day, 0, 0, 1, DateTimeKind.Utc);
 
-            // OK
+            // GOOD
             this.Start = now.AddYears(-1).Date;
         }
 
         private void Test(int year, int month, int day)
         {
-            // BUG (arithmetic operation from StartTest)
+            // BAD (arithmetic operation from StartTest)
             this.Start = new DateTime(year, month, day);
         }