Merge remote-tracking branch 'upstream/rc/1.23' into mergeback-20191202

Conflicts solved:
	javascript/extractor/src/com/semmle/js/extractor/Main.java
	javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.qhelp

@@ -18,49 +18,39 @@ optimizing compiler.
 <recommendation>
 <p>
 Solutions to this problem can be thought of as falling into one of two
-categories: (1) rewrite the signed expression so that overflow cannot occur
-but the signedness remains, or (2) rewrite (or cast) the signed expression
-into unsigned form.
+categories:
 </p>
 
-<p>
-Below we list examples of expressions where signed overflow may
-occur, along with proposed solutions.  The list should not be
-considered exhaustive.
-</p>
+<ol>
+<li>Rewrite the signed expression so that overflow cannot occur
+but the signedness remains.</li>
+<li>Change the variables and all their uses to be unsigned.</li>
+</ol>
 
 <p>
-Given <code>unsigned short i, delta</code> and <code>i + delta &lt; i</code>, 
-it is possible to rewrite it as <code>(unsigned short)(i + delta)&nbsp;&lt;&nbsp;i</code>.
-Note that <code>i + delta</code>does not actually overflow, due to <code>int</code> promotion
+The following cases all fall into the first category.
 </p>
 
-<p>
-Given <code>unsigned short i, delta</code> and <code>i + delta &lt; i</code>,
-it is also possible to rewrite it as <code>USHORT_MAX - delta</code>. It must be true
-that <code>delta &gt; 0</code> and the <code>limits.h</code> or <code>climits</code>
+<ol>
+<li>
+Given <code>unsigned short n1, delta</code> and <code>n1 + delta &lt; n1</code>,
+it is possible to rewrite it as <code>(unsigned short)(n1 + delta)&nbsp;&lt;&nbsp;n1</code>.
+Note that <code>n1 + delta</code> does not actually overflow, due to <code>int</code> promotion.
+</li>
+
+<li>
+Given <code>unsigned short n1, delta</code> and <code>n1 + delta &lt; n1</code>,
+it is also possible to rewrite it as <code>n1 &gt; USHORT_MAX - delta</code>. The
+<code>limits.h</code> or <code>climits</code> header must then be included.
+</li>
+
+<li>
+Given <code>int n1, delta</code> and <code>n1 + delta &lt; n1</code>,
+it is possible to rewrite it as <code>n1 &gt; INT_MAX - delta</code>. It must be true
+that <code>delta &gt;= 0</code> and the <code>limits.h</code> or <code>climits</code>
 header has been included.
-</p>
-
-<p>
-Given <code>int i, delta</code> and <code>i + delta &lt; i</code>,
-it is possible to rewrite it as <code>INT_MAX - delta</code>. It must be true
-that <code>delta &gt; 0</code> and the <code>limits.h</code> or <code>climits</code>
-header has been included.
-</p>
-
-<p>
-Given <code>int i, delta</code> and <code>i + delta &lt; i</code>,
-it is also possible to rewrite it as <code>(unsigned)i + delta &lt; i</code>.
-Note that program semantics are affected by this change.
-</p>
-
-<p>
-Given <code>int i, delta</code> and <code>i + delta &lt; i</code>,
-it is also possible to rewrite it as <code>unsigned int i, delta</code> and
-<code>i + delta &lt; i</code>.  Note that program semantics are 
-affected by this change.
-</p>
+</li>
+</ol>
 </recommendation>
 
 <example>
@@ -68,7 +58,7 @@ affected by this change.
 In the following example, even though <code>delta</code> has been declared
 <code>unsigned short</code>, C/C++ type promotion rules require that its
 type is promoted to the larger type used in the addition and comparison,
-namely a <code>signed int</code>.  Addition is performed on 
+namely a <code>signed int</code>.  Addition is performed on
 signed integers, and may have undefined behavior if an overflow occurs.
 As a result, the entire (comparison) expression may also have an undefined
 result.
@@ -87,10 +77,10 @@ are avoided.
 <sample src="SignedOverflowCheck-good1.cpp" />
 <p>
 In the following example, even though both <code>n</code> and <code>delta</code>
-have been declared <code>unsigned short</code>, both are promoted to 
+have been declared <code>unsigned short</code>, both are promoted to
 <code>signed int</code> prior to addition.  Because we started out with the
 narrower <code>short</code> type, the addition is guaranteed not to overflow
-and is therefore defined.  But the fact that <code>n1 + delta</code> never 
+and is therefore defined.  But the fact that <code>n1 + delta</code> never
 overflows means that the condition <code>n1 + delta &lt; n1</code> will never
 hold true, which likely is not what the programmer intended. (see also the
 <code>cpp/bad-addition-overflow-check</code> query).
@@ -98,7 +88,7 @@ hold true, which likely is not what the programmer intended. (see also the
 <sample src="SignedOverflowCheck-bad2.cpp" />
 <p>
 The next example provides a solution to the previous one.  Even though
-<code>i + delta</code> does not overflow, casting it to an
+<code>n1 + delta</code> does not overflow, casting it to an
 <code>unsigned short</code> truncates the addition modulo 2^16,
 so that <code>unsigned short</code> "wrap around" may now be observed.
 Furthermore, since the left-hand side is now of type <code>unsigned short</code>,

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.ql

@@ -14,6 +14,7 @@
 import cpp
 private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 private import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
+private import semmle.code.cpp.commons.Exclusions
 
 from RelationalOperation ro, AddExpr add, Expr expr1, Expr expr2
 where
@@ -22,7 +23,7 @@ where
   ro.getAnOperand() = expr2 and
   globalValueNumber(expr1) = globalValueNumber(expr2) and
   add.getUnspecifiedType().(IntegralType).isSigned() and
-  not exists(MacroInvocation mi | mi.getAnAffectedElement() = add) and
+  not isFromMacroDefinition(ro) and
   exprMightOverflowPositively(add) and
   exists(Compilation c | c.getAFileCompiled() = ro.getFile() |
     not c.getAnArgument() = "-fwrapv" and

cpp/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql

@@ -44,7 +44,7 @@ Element friendlyLoc(Expr e) {
   not e instanceof Access and not e instanceof Call and result = e
 }
 
-from Loop l, RelationalOperation rel, Expr small, Expr large
+from Loop l, RelationalOperation rel, VariableAccess small, Expr large
 where
   small = rel.getLesserOperand() and
   large = rel.getGreaterOperand() and
@@ -60,7 +60,7 @@ where
   not getComparisonSize(large.(SubExpr).getLeftOperand().getExplicitlyConverted()) <= getComparisonSize(small) and
   not getComparisonSize(large.(RShiftExpr).getLeftOperand().getExplicitlyConverted()) <= getComparisonSize(small) and
   // ignore loop-invariant smaller variables
-  loopVariant(small.getAChild*(), l)
+  loopVariant(small, l)
 select rel,
   "Comparison between $@ of type " + small.getType().getName() + " and $@ of wider type " +
     large.getType().getName() + ".", friendlyLoc(small), small.toString(), friendlyLoc(large),

cpp/ql/src/semmle/code/cpp/Enclosing.qll

@@ -13,6 +13,11 @@ Element stmtEnclosingElement(Stmt s) {
 /**
  * Gets the enclosing element of expression `e`.
  */
+// The `pragma[nomagic]` is a workaround to prevent this cached stage (and all
+// subsequent stages) from being evaluated twice. See QL-888. It has the effect
+// of making the `Conversion` class predicate get the same optimization in all
+// queries.
+pragma[nomagic]
 cached
 Element exprEnclosingElement(Expr e) {
   result = exprEnclosingElement(e.getParent())

cpp/ql/src/semmle/code/cpp/commons/Exclusions.qll

@@ -94,11 +94,18 @@ predicate functionContainsPreprocCode(Function f) {
  * ```
  */
 predicate isFromMacroDefinition(Element e) {
-  exists(MacroInvocation mi |
-    // e is in mi
+  exists(MacroInvocation mi, Location eLocation, Location miLocation |
     mi.getAnExpandedElement() = e and
-    // and e was apparently not passed in as a macro parameter
-    e.getLocation().getStartLine() = mi.getLocation().getStartLine() and
-    e.getLocation().getStartColumn() = mi.getLocation().getStartColumn()
+    eLocation = e.getLocation() and
+    miLocation = mi.getLocation() and
+    // If the location of `e` coincides with the macro invocation, then `e` did
+    // not come from a macro argument. The inequalities here could also be
+    // equalities, but that confuses the join orderer into joining on the source
+    // locations too early.
+    // There are cases where the start location of a non-argument element comes
+    // right after the invocation's open parenthesis, so it appears to be more
+    // robust to match on the end location instead.
+    eLocation.getEndLine() >= miLocation.getEndLine() and
+    eLocation.getEndColumn() >= miLocation.getEndColumn()
   )
 }

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImplCommon.qll

@@ -360,7 +360,7 @@ private module ImplCommon {
      */
     cached
     predicate read(Node node1, Content f, Node node2) {
-      readStep(node1, f, node2) and storeStep(_, f, _)
+      readStep(node1, f, node2)
       or
       exists(DataFlowCall call, ReturnKind kind |
         read0(call, kind, node1, f) and

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImplCommon.qll

@@ -360,7 +360,7 @@ private module ImplCommon {
      */
     cached
     predicate read(Node node1, Content f, Node node2) {
-      readStep(node1, f, node2) and storeStep(_, f, _)
+      readStep(node1, f, node2)
       or
       exists(DataFlowCall call, ReturnKind kind |
         read0(call, kind, node1, f) and

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll

@@ -1,5 +1,6 @@
 import AliasAnalysis
 private import SimpleSSAImports
+import SimpleSSAPublicImports
 
 private class IntValue = Ints::IntValue;
 

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSAImports.qll

@@ -2,4 +2,3 @@ import semmle.code.cpp.ir.implementation.raw.IR
 import semmle.code.cpp.ir.internal.IntegerConstant as Ints
 import semmle.code.cpp.ir.implementation.internal.OperandTag
 import semmle.code.cpp.ir.internal.IRCppLanguage as Language
-import semmle.code.cpp.ir.internal.Overlap

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSAPublicImports.qll

@@ -0,0 +1 @@
+import semmle.code.cpp.ir.internal.Overlap

cpp/ql/src/semmle/uml/MagicDraw.qll

@@ -112,7 +112,7 @@ class UMLType extends UMLElement {
     else result = this.getUMLName()
   }
 
-  string toString() { result = this.getUMLName() }
+  override string toString() { result = this.getUMLName() }
 }
 
 /**
@@ -163,7 +163,7 @@ class UMLProperty extends UMLElement {
     result.getDeclaringType() = this.getUMLType().getCType()
   }
 
-  string toString() {
+  override string toString() {
     if this.isEnumConstant()
     then result = "- <<enum constant>> " + this.getUMLName()
     else result = "- " + this.getUMLName()
@@ -196,7 +196,7 @@ class UMLOperation extends UMLElement {
     result.getDeclaringType() = this.getUMLType().getCType()
   }
 
-  string toString() { result = "+ " + this.getUMLName() }
+  override string toString() { result = "+ " + this.getUMLName() }
 }
 
 /**
@@ -221,7 +221,7 @@ class UMLAssociation extends UMLProperty {
   /**
    * Gets the C field corresponding to this property, if any.
    */
-  Field getCField() {
+  override Field getCField() {
     result.hasName(this.getLabel()) and
     result.getDeclaringType() = this.getSource().getCType()
   }
@@ -271,7 +271,7 @@ class UMLInheritance extends UMLElement {
     )
   }
 
-  string toString() {
+  override string toString() {
     result = this.getUMLClient().getUMLName() + " implements " + this.getUMLSupplier().getUMLName()
   }
 }
@@ -303,5 +303,5 @@ class UMLPackage extends UMLElement {
     else result = this.getUMLName()
   }
 
-  string toString() { result = this.getUMLQualifiedName() }
+  override string toString() { result = this.getUMLQualifiedName() }
 }