Corrections related to the review comments.

github · geoffw0 · Jun 18, 2019 · May 21, 2019 · Jun 13, 2019 · Jun 14, 2019
commit ad489db8156915c15f26e093477a427ac1fe9235
diff --git a/cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.qhelp b/cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.qhelp
@@ -4,7 +4,8 @@
 <qhelp>
 <overview>
   <p>
-    When eras change, date and time conversions that rely on hard-coded era start date need to be reviewed. Conversions relying on Japanese dates in the current era can produce an ambiguous date. 
+    When eras change, date and time conversions that rely on a hard-coded era start date need to be reviewed. Conversions relying on Japanese dates in the current era can produce an ambiguous date. 
+    The values for the current Japanese era dates should be read from a source that will be updated, such as the Windows registry.
   </p>
 </overview>
 

diff --git a/cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.ql b/cpp/ql/src/Likely Bugs/JapaneseEra/ConstructorOrMethodWithExactEraDate.ql
@@ -1,9 +1,9 @@
 /**
- * @name ConstructingJapaneseEraStartDate
- * @description Japanese era changes can lead to code behaving differently. Aviod hard-coding Japanese era start dates. The values should be read from registry.
+ * @name Hard-coded Japanese era start date
+ * @description Japanese era changes can lead to code behaving differently. Avoid hard-coding Japanese era start dates.
  * @kind problem
  * @problem.severity warning
- * @id cpp/JapaneseEra/Constructor-Or-Method-With-Exact-Era-Date
+ * @id cpp/japanese-era/constructor-or-method-with-exact-era-date
  * @precision medium
  * @tags reliability
  *       japanese-era
@@ -14,4 +14,4 @@ from Call cc, int i
 where cc.getArgument(i).getValue().toInt() = 1989 and
       cc.getArgument(i+1).getValue().toInt() = 1 and
       cc.getArgument(i+2).getValue().toInt() = 8
-select cc, "Call that appears to have hard-coded Japanese era start date as parameter" 
+select cc, "Call that appears to have hard-coded Japanese era start date as parameter." 
diff --git a/cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.qhelp b/cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.qhelp
@@ -4,7 +4,8 @@
 <qhelp>
 <overview>
   <p>
-    When eras change, date and time conversions that rely on hard-coded era start date need to be reviewed. Conversions relying on Japanese dates in the current era can produce an ambiguous date. 
+    When eras change, date and time conversions that rely on a hard-coded era start date need to be reviewed. Conversions relying on Japanese dates in the current era can produce an ambiguous date. 
+    The values for the current Japanese era dates should be read from a source that will be updated, such as the Windows registry.
   </p>
 </overview>
 

diff --git a/cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.ql b/cpp/ql/src/Likely Bugs/JapaneseEra/StructWithExactEraDate.ql
@@ -1,9 +1,9 @@
 /**
- * @name StructWithJapaneseEraStartDate
- * @description Japanese era changes can lead to code behaving differently. Aviod hard-coding Japanese era start dates. The values should be read from registry.
+ * @name Hard-coded Japanese era start date
+ * @description Japanese era changes can lead to code behaving differently. Avoid hard-coding Japanese era start dates.
  * @kind problem
  * @problem.severity warning
- * @id cpp/JapaneseEra/Struct-With-Exact-Era-Date
+ * @id cpp/japanese-era/struct-with-exact-era-date
  * @precision medium
  * @tags reliability
  *       japanese-era
@@ -17,4 +17,4 @@ from StructLikeClass s, YearFieldAccess year, MonthFieldAccess month, DayFieldAc
 where s.getAField().getAnAccess () = year and yearAssignment.getAnOperand() = year and yearAssignment.getAnOperand().getValue().toInt() = 1989 and
       s.getAField().getAnAccess () = month and monthAssignment.getAnOperand() = month and monthAssignment.getAnOperand().getValue().toInt() = 1 and 
       s.getAField().getAnAccess () = day and dayAssignment.getAnOperand() = day and dayAssignment.getAnOperand().getValue().toInt() = 8
-select year, "A time struct that is initialized with exact Japanese calendar era start date"
+select year, "A time struct that is initialized with exact Japanese calendar era start date."
diff --git a/cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.ql b/cpp/ql/src/Likely Bugs/Leap Year/Adding365daysPerYear.ql
@@ -1,6 +1,6 @@
 /**
- * @name Year field changed using an arithmetic operation is used on an unchekced time conversion function
- * @description A year field changed using an arithmetic operation is used on a time conversion function, but the return value of the function is not check for success or failure
+ * @name Year field changed using an arithmetic operation is used on an unchecked time conversion function
+ * @description A year field changed using an arithmetic operation is used on a time conversion function, but the return value of the function is not checked for success or failure.
  * @kind problem
  * @problem.severity error
  * @id cpp/leap-year/adding-365-days-per-year
@@ -13,9 +13,8 @@ import cpp
 import LeapYear
 import semmle.code.cpp.dataflow.DataFlow
 
-
 from Expr source, Expr sink, PossibleYearArithmeticOperationCheckConfiguration config
 where config.hasFlow(DataFlow::exprNode(source), DataFlow::exprNode(sink))
 select sink, "This arithmetic operation $@ uses a constant value of 365 ends up modifying the date/time located at $@, without considering leap year scenarios."
-    , source, source.toString()
-    , sink, sink.toString()
+  , source, source.toString()
+  , sink, sink.toString()
diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qhelp b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qhelp
@@ -2,14 +2,15 @@
   "-//Semmle//qhelp//EN"
   "qhelp.dtd">
 <qhelp>
-<overview>
-<p>The Gregorian Calendar, which has become the internationally accepted civil calendar, the leap year rule is: Every year that is exactly divisible by four is a leap year, except for years that are exactly divisible by 100, but these centurial years are leap years if they are exactly divisible by 400.</p>
-<p>A leap year bug occurs when software (in any language) is written without consideration of leap year logic, or with flawed logic to calculate leap years; which typically results in incorrect results.</p>
-<p>The impact of these bugs may range from almost unnoticeable bugs such as an incorrect date, to severe bugs that affect reliability, availability or even the security of the affected system.</p>
-</overview>
+  <overview>
+    <p>The leap year rule for the Gregorian calendar, which has become the internationally accepted civil calendar, is: every year that is exactly divisible by four is a leap year, except for years that are exactly divisible by 100, but these centurial years are leap years if they are exactly divisible by 400.</p>
+    <p>A leap year bug occurs when software (in any language) is written without consideration of leap year logic, or with flawed logic to calculate leap years; which typically results in incorrect results.</p>
+    <p>The impact of these bugs may range from almost unnoticeable bugs such as an incorrect date, to severe bugs that affect reliability, availability or even the security of the affected system.</p>
+  </overview>
 
-<references>
-<li>U.S. Naval Observatory Website - <a href="https://aa.usno.navy.mil/faq/docs/calendars.php"> Introduction to Calendars</a></li>
-<li>Wikipedia - <a href="https://en.wikipedia.org/wiki/Leap_year_bug"> Leap year bug</a> </li>
-<li>Microsoft Azure blog - <a href="https://azure.microsoft.com/en-us/blog/is-your-code-ready-for-the-leap-year/"> Is your code ready for the leap year?</a> </li>
-</references>
+  <references>
+    <li>U.S. Naval Observatory Website - <a href="https://aa.usno.navy.mil/faq/docs/calendars.php"> Introduction to Calendars</a></li>
+    <li>Wikipedia - <a href="https://en.wikipedia.org/wiki/Leap_year_bug"> Leap year bug</a> </li>
+    <li>Microsoft Azure blog - <a href="https://azure.microsoft.com/en-us/blog/is-your-code-ready-for-the-leap-year/"> Is your code ready for the leap year?</a> </li>
+  </references>
+</qhelp>
diff --git a/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll b/cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll
@@ -1,5 +1,5 @@
 /**
- * Provides a library for helping create leap year realted queries
+ * Provides a library for helping create leap year related queries
  */
 import cpp
 import semmle.code.cpp.dataflow.DataFlow
@@ -8,7 +8,6 @@ import semmle.code.cpp.commons.DateTime
 
 /**
  * Get the top-level BinaryOperation enclosing the expression e
- * Not 
  */
 BinaryOperation getATopLevelBinaryOperationExpression(Expr e)
 {
@@ -73,7 +72,7 @@ abstract class LeapYearFieldAccess extends YearFieldAccess {
    * Holds if the field access is a modification, 
    * 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
    */

diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql
@@ -1,6 +1,6 @@
 /**
  * @name Year field changed using an arithmetic operation without checking for leap year
- * @description A that field that represents a year is being modified by an arithmetic operation, but no proper check for leap year can be detected afterwards.
+ * @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
  * @problem.severity error
  * @id cpp/leap-year/unchecked-after-arithmetic-year-modification
@@ -14,48 +14,45 @@ import LeapYear
 
 from Variable var, LeapYearFieldAccess yfa 
 where
-    exists( VariableAccess va |
-    yfa.getQualifier() = va
-        and var.getAnAccess() = va
-        // The year is modified with an arithmetic operation. Avoid values that are likely false positives
-        and yfa.isModifiedByArithmeticOperationNotForNormalization()
-        // Avoid false positives
-        and 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 = yfa.getASuccessor*()
-            )
-            // If there is a data flow from the variable that was modified to a function that seems to check for leap year 
-            or exists(
-                VariableAccess source,
-                ChecksForLeapYearFunctionCall fc, 
-                LeapYearCheckConfiguration config |
-                source = var.getAnAccess()
-                and config.hasFlow( DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument()))
-            )
-            // If there is a data flow from the field that was modified to a function that seems to check for leap year
-            or exists(
-                VariableAccess vacheck,
-                YearFieldAccess yfacheck,
-                ChecksForLeapYearFunctionCall fc, 
-                LeapYearCheckConfiguration config |
-                vacheck = var.getAnAccess()
-                and yfacheck.getQualifier() = vacheck
-                and config.hasFlow( DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument()))
-            )
-            // If there is a successor or predecessor that sets the month = 1
-            or exists( 
-                MonthFieldAccess  mfa, AssignExpr ae |
-                    mfa.getQualifier() = var.getAnAccess()
-                    and mfa.isModified()
-                    and ( mfa = yfa.getASuccessor*()
-                          or yfa = mfa.getASuccessor*())
-                    and ae = mfa.getEnclosingElement()
-                    and ae.getAnOperand().getValue().toInt() = 1 
-            )
+   exists(VariableAccess va |
+     yfa.getQualifier() = va
+     and var.getAnAccess() = va
+     // The year is modified with an arithmetic operation. Avoid values that are likely false positives
+     and yfa.isModifiedByArithmeticOperationNotForNormalization()
+     // Avoid false positives
+     and 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 = yfa.getASuccessor*()
         )
-    )
+        // If there is a data flow from the variable that was modified to a function that seems to check for leap year 
+        or exists(VariableAccess source,
+          ChecksForLeapYearFunctionCall fc, 
+          LeapYearCheckConfiguration config |
+          source = var.getAnAccess()
+          and config.hasFlow( DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument()))
+       )
+       // If there is a data flow from the field that was modified to a function that seems to check for leap year
+       or exists(VariableAccess vacheck,
+         YearFieldAccess yfacheck,
+         ChecksForLeapYearFunctionCall fc, 
+         LeapYearCheckConfiguration config |
+         vacheck = var.getAnAccess()
+         and yfacheck.getQualifier() = vacheck
+         and config.hasFlow( DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument()))
+       )
+       // If there is a successor or predecessor that sets the month = 1
+       or exists(MonthFieldAccess  mfa, AssignExpr ae |
+          mfa.getQualifier() = var.getAnAccess()
+          and mfa.isModified()
+          and (mfa = yfa.getASuccessor*()
+               or yfa = mfa.getASuccessor*())
+          and ae = mfa.getEnclosingElement()
+          and ae.getAnOperand().getValue().toInt() = 1 
+       )
+     )
+   )
 select yfa
   , "Field $@ on variable $@ has been modified, but no appropriate check for LeapYear was found.", yfa.getTarget(), yfa.getTarget().toString(), var, var.toString()
diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationBad.c b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModificationBad.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++;
 
 // The following code may fail

diff --git a/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.qhelp b/cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.qhelp
@@ -5,9 +5,9 @@
 <overview>
   <include src="LeapYear.qhelp" />
 
-<p>When using a function that transform a date structure, and the year on the input argument for teh API has been manipulated, it is important to check for the return value of the function to make sure it succeeded.</p>
+<p>When using a function that transforms a date structure, and the year on the input argument for the API has been manipulated, it is important to check for the return value of the function to make sure it succeeded.</p>
 <p>Otherwise, the function may have failed, and the output parameter may contain invalid data that can cause any number of problems on the affected system.</p>
-<p>The following is a list of the function that this query covers:</p>
+<p>The following is a list of the functions that this query covers:</p>
   <list>
   <li><code>FileTimeToSystemTime</code></li>
   <li><code>SystemTimeToFileTime</code></li>
@@ -19,10 +19,10 @@
   <li><code>RtlTimeToSecondsSince1970</code></li>
   <li><code>_mkgmtime</code></li>
 </list>
-  
+
 </overview>
 <recommendation>
-<p>When calling an API that transforms a date variable that was manipulated, always check for the return value to verify if the API call succeeded or failed.</p>
+<p>When calling an API that transforms a date variable that was manipulated, always check for the return value to verify that the API call succeeded.</p>
 
 </recommendation>
 <example>