Merge remote-tracking branch 'upstream/master' into dataflow-ref-parameter

I've accepted the new test output, which shows that this branch fixes
two false negatives in the test cases from #2088.

Author: jbj

change-notes/1.23/analysis-csharp.md

@@ -16,6 +16,7 @@ The following changes in version 1.23 affect C# analysis in all applications.
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
 | Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | Fewer false positive results | More `null` checks are now taken into account, including `null` checks for `dynamic` expressions and `null` checks such as `object alwaysNull = null; if (x != alwaysNull) ...`. |
+| Missing Dispose call on local IDisposable (`cs/local-not-disposed`) | Fewer false positive results | The query has been rewritten in order to identify more dispose patterns. For example, a local `IDisposable` that is disposed of by passing through a fluent API is no longer reported. |
 
 ## Removal of old queries
 
@@ -38,5 +39,6 @@ The following changes in version 1.23 affect C# analysis in all applications.
   disabled by default and can be enabled for individual configurations by
   overriding `int explorationLimit()`.
 * `foreach` statements where the body is guaranteed to be executed at least once, such as `foreach (var x in new string[]{ "a", "b", "c" }) { ... }`, are now recognized by all analyses based on the control flow graph (such as SSA, data flow and taint tracking).
+* Fixed the control flow graph for `switch` statements where the `default` case was not the last case. This had caused the remaining cases to be unreachable. `SwitchStmt.getCase(int i)` now puts the `default` case last.
 
 ## Changes to autobuilder

change-notes/1.23/analysis-java.md

@@ -6,6 +6,7 @@ The following changes in version 1.23 affect Java analysis in all applications.
 
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
+| Dereferenced variable may be null (`java/dereferenced-value-may-be-null`) | Fewer false positives | Certain indirect null guards involving two auxiliary variables known to be equal can now be detected. |
 | Query built from user-controlled sources (`java/sql-injection`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. |
 | Query built from local-user-controlled sources (`java/sql-injection-local`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. |
 | Query built without neutralizing special characters (`java/concatenated-sql-query`) | More results | The query now identifies arguments to `Statement.executeLargeUpdate` and `Connection.prepareCall` as SQL expressions sinks. |

change-notes/1.23/analysis-javascript.md

@@ -16,6 +16,7 @@
 |---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Unused index variable (`js/unused-index-variable`)                        | correctness                                                       | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
 | Loop bound injection (`js/loop-bound-injection`)                          | security, external/cwe/cwe-834                                      | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
+| Suspicious method name (`js/suspicious-method-name-declaration`)          | correctness, typescript, methods                                  | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 

change-notes/1.23/analysis-python.md

@@ -11,4 +11,4 @@
 |-----------|----------|-------------|
 | Clear-text logging of sensitive information (`py/clear-text-logging-sensitive-data`) | security, external/cwe/cwe-312 | Finds instances where sensitive information is logged without encryption or hashing. Results are shown on LGTM by default. |
 | Clear-text storage of sensitive information (`py/clear-text-storage-sensitive-data`) | security, external/cwe/cwe-312 | Finds instances where sensitive information is stored without encryption or hashing. Results are shown on LGTM by default. |
-
+| Binding a socket to all network interfaces (`py/bind-socket-all-network-interfaces`) | security | Finds instances where a socket is bound to all network interfaces. Results are shown on LGTM by default. |

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

@@ -30,7 +30,7 @@ private predicate additionalLogicalCheck(Expr e, string operation, int valueToCh
 /**
  * An `Operation` that seems to be checking for leap year.
  */
-class CheckForLeapYearOperation extends Operation {
+class CheckForLeapYearOperation extends Expr {
   CheckForLeapYearOperation() {
     exists(BinaryArithmeticOperation bo | bo = this |
       bo.getAnOperand().getValue().toInt() = 4 and
@@ -39,8 +39,6 @@ class CheckForLeapYearOperation extends Operation {
       additionalLogicalCheck(this.getEnclosingElement(), "%", 400)
     )
   }
-
-  override string getOperator() { result = "LeapYearCheck" }
 }
 
 /**

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

@@ -7,7 +7,7 @@
  * on each other without introducing mutual recursion among those configurations.
  */
 
-private import DataFlowImplCommon
+private import DataFlowImplCommon::Public
 private import DataFlowImplSpecific::Private
 import DataFlowImplSpecific::Public
 
@@ -1075,6 +1075,7 @@ private predicate flowCandFwd0(Node node, boolean fromArg, AccessPathFront apf,
     flowCandFwd(mid, fromArg, _, config) and
     store(mid, f, node) and
     nodeCand(node, unbind(config)) and
+    readStoreCand(f, unbind(config)) and
     apf.headUsesContent(f)
   )
   or
@@ -1175,12 +1176,12 @@ private predicate flowCand0(Node node, boolean toReturn, AccessPathFront apf, Co
   exists(Content f, AccessPathFront apf0 |
     flowCandStore(node, f, toReturn, apf0, config) and
     apf0.headUsesContent(f) and
-    consCand(f, apf, unbind(config))
+    consCand(f, apf, config)
   )
   or
   exists(Content f, AccessPathFront apf0 |
     flowCandRead(node, f, toReturn, apf0, config) and
-    consCandFwd(f, apf0, unbind(config)) and
+    consCandFwd(f, apf0, config) and
     apf.headUsesContent(f)
   )
 }
@@ -1221,8 +1222,8 @@ private newtype TAccessPath =
   TConsCons(Content f1, Content f2, int len) { consCand(f1, TFrontHead(f2), _) and len in [2 .. 5] }
 
 /**
- * Conceptually a list of `Content`s followed by a `Type`, but only the first
- * element of the list and its length are tracked. If data flows from a source to
+ * Conceptually a list of `Content`s followed by a `Type`, but only the first two
+ * elements of the list and its length are tracked. If data flows from a source to
  * a given node with a given `AccessPath`, this indicates the sequence of
  * dereference operations needed to get from the value in the node to the
  * tracked object. The final type indicates the type of the tracked object.
@@ -1260,7 +1261,7 @@ abstract private class AccessPath extends TAccessPath {
 
 private class AccessPathNil extends AccessPath, TNil {
   override string toString() {
-    exists(DataFlowType t | this = TNil(t) | result = concat(" : " + ppReprType(t)))
+    exists(DataFlowType t | this = TNil(t) | result = concat(": " + ppReprType(t)))
   }
 
   override AccessPathFront getFront() {
@@ -1276,7 +1277,7 @@ private class AccessPathConsNil extends AccessPathCons, TConsNil {
   override string toString() {
     exists(Content f, DataFlowType t | this = TConsNil(f, t) |
       // The `concat` becomes "" if `ppReprType` has no result.
-      result = f.toString() + concat(" : " + ppReprType(t))
+      result = "[" + f.toString() + "]" + concat(" : " + ppReprType(t))
     )
   }
 
@@ -1293,8 +1294,8 @@ private class AccessPathConsCons extends AccessPathCons, TConsCons {
   override string toString() {
     exists(Content f1, Content f2, int len | this = TConsCons(f1, f2, len) |
       if len = 2
-      then result = f1.toString() + ", " + f2.toString()
-      else result = f1.toString() + ", " + f2.toString() + ", ... (" + len.toString() + ")"
+      then result = "[" + f1.toString() + ", " + f2.toString() + "]"
+      else result = "[" + f1.toString() + ", " + f2.toString() + ", ... (" + len.toString() + ")]"
     )
   }
 
@@ -1625,7 +1626,7 @@ abstract class PathNode extends TPathNode {
     this instanceof PathNodeSink and result = ""
     or
     exists(string s | s = this.(PathNodeMid).getAp().toString() |
-      if s = "" then result = "" else result = " [" + s + "]"
+      if s = "" then result = "" else result = " " + s
     )
   }
 
@@ -2070,7 +2071,7 @@ private module FlowExploration {
 
   private class PartialAccessPathNil extends PartialAccessPath, TPartialNil {
     override string toString() {
-      exists(DataFlowType t | this = TPartialNil(t) | result = concat(" : " + ppReprType(t)))
+      exists(DataFlowType t | this = TPartialNil(t) | result = concat(": " + ppReprType(t)))
     }
 
     override AccessPathFront getFront() {
@@ -2082,8 +2083,8 @@ private module FlowExploration {
     override string toString() {
       exists(Content f, int len | this = TPartialCons(f, len) |
         if len = 1
-        then result = f.toString()
-        else result = f.toString() + ", ... (" + len.toString() + ")"
+        then result = "[" + f.toString() + "]"
+        else result = "[" + f.toString() + ", ... (" + len.toString() + ")]"
       )
     }
 
@@ -2160,7 +2161,7 @@ private module FlowExploration {
 
     private string ppAp() {
       exists(string s | s = this.(PartialPathNodePriv).getAp().toString() |
-        if s = "" then result = "" else result = " [" + s + "]"
+        if s = "" then result = "" else result = " " + s
       )
     }
 

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

@@ -7,7 +7,7 @@
  * on each other without introducing mutual recursion among those configurations.
  */
 
-private import DataFlowImplCommon
+private import DataFlowImplCommon::Public
 private import DataFlowImplSpecific::Private
 import DataFlowImplSpecific::Public
 
@@ -1075,6 +1075,7 @@ private predicate flowCandFwd0(Node node, boolean fromArg, AccessPathFront apf,
     flowCandFwd(mid, fromArg, _, config) and
     store(mid, f, node) and
     nodeCand(node, unbind(config)) and
+    readStoreCand(f, unbind(config)) and
     apf.headUsesContent(f)
   )
   or
@@ -1175,12 +1176,12 @@ private predicate flowCand0(Node node, boolean toReturn, AccessPathFront apf, Co
   exists(Content f, AccessPathFront apf0 |
     flowCandStore(node, f, toReturn, apf0, config) and
     apf0.headUsesContent(f) and
-    consCand(f, apf, unbind(config))
+    consCand(f, apf, config)
   )
   or
   exists(Content f, AccessPathFront apf0 |
     flowCandRead(node, f, toReturn, apf0, config) and
-    consCandFwd(f, apf0, unbind(config)) and
+    consCandFwd(f, apf0, config) and
     apf.headUsesContent(f)
   )
 }
@@ -1221,8 +1222,8 @@ private newtype TAccessPath =
   TConsCons(Content f1, Content f2, int len) { consCand(f1, TFrontHead(f2), _) and len in [2 .. 5] }
 
 /**
- * Conceptually a list of `Content`s followed by a `Type`, but only the first
- * element of the list and its length are tracked. If data flows from a source to
+ * Conceptually a list of `Content`s followed by a `Type`, but only the first two
+ * elements of the list and its length are tracked. If data flows from a source to
  * a given node with a given `AccessPath`, this indicates the sequence of
  * dereference operations needed to get from the value in the node to the
  * tracked object. The final type indicates the type of the tracked object.
@@ -1260,7 +1261,7 @@ abstract private class AccessPath extends TAccessPath {
 
 private class AccessPathNil extends AccessPath, TNil {
   override string toString() {
-    exists(DataFlowType t | this = TNil(t) | result = concat(" : " + ppReprType(t)))
+    exists(DataFlowType t | this = TNil(t) | result = concat(": " + ppReprType(t)))
   }
 
   override AccessPathFront getFront() {
@@ -1276,7 +1277,7 @@ private class AccessPathConsNil extends AccessPathCons, TConsNil {
   override string toString() {
     exists(Content f, DataFlowType t | this = TConsNil(f, t) |
       // The `concat` becomes "" if `ppReprType` has no result.
-      result = f.toString() + concat(" : " + ppReprType(t))
+      result = "[" + f.toString() + "]" + concat(" : " + ppReprType(t))
     )
   }
 
@@ -1293,8 +1294,8 @@ private class AccessPathConsCons extends AccessPathCons, TConsCons {
   override string toString() {
     exists(Content f1, Content f2, int len | this = TConsCons(f1, f2, len) |
       if len = 2
-      then result = f1.toString() + ", " + f2.toString()
-      else result = f1.toString() + ", " + f2.toString() + ", ... (" + len.toString() + ")"
+      then result = "[" + f1.toString() + ", " + f2.toString() + "]"
+      else result = "[" + f1.toString() + ", " + f2.toString() + ", ... (" + len.toString() + ")]"
     )
   }
 
@@ -1625,7 +1626,7 @@ abstract class PathNode extends TPathNode {
     this instanceof PathNodeSink and result = ""
     or
     exists(string s | s = this.(PathNodeMid).getAp().toString() |
-      if s = "" then result = "" else result = " [" + s + "]"
+      if s = "" then result = "" else result = " " + s
     )
   }
 
@@ -2070,7 +2071,7 @@ private module FlowExploration {
 
   private class PartialAccessPathNil extends PartialAccessPath, TPartialNil {
     override string toString() {
-      exists(DataFlowType t | this = TPartialNil(t) | result = concat(" : " + ppReprType(t)))
+      exists(DataFlowType t | this = TPartialNil(t) | result = concat(": " + ppReprType(t)))
     }
 
     override AccessPathFront getFront() {
@@ -2082,8 +2083,8 @@ private module FlowExploration {
     override string toString() {
       exists(Content f, int len | this = TPartialCons(f, len) |
         if len = 1
-        then result = f.toString()
-        else result = f.toString() + ", ... (" + len.toString() + ")"
+        then result = "[" + f.toString() + "]"
+        else result = "[" + f.toString() + ", ... (" + len.toString() + ")]"
       )
     }
 
@@ -2160,7 +2161,7 @@ private module FlowExploration {
 
     private string ppAp() {
       exists(string s | s = this.(PartialPathNodePriv).getAp().toString() |
-        if s = "" then result = "" else result = " [" + s + "]"
+        if s = "" then result = "" else result = " " + s
       )
     }
 

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

@@ -7,7 +7,7 @@
  * on each other without introducing mutual recursion among those configurations.
  */
 
-private import DataFlowImplCommon
+private import DataFlowImplCommon::Public
 private import DataFlowImplSpecific::Private
 import DataFlowImplSpecific::Public
 
@@ -1075,6 +1075,7 @@ private predicate flowCandFwd0(Node node, boolean fromArg, AccessPathFront apf,
     flowCandFwd(mid, fromArg, _, config) and
     store(mid, f, node) and
     nodeCand(node, unbind(config)) and
+    readStoreCand(f, unbind(config)) and
     apf.headUsesContent(f)
   )
   or
@@ -1175,12 +1176,12 @@ private predicate flowCand0(Node node, boolean toReturn, AccessPathFront apf, Co
   exists(Content f, AccessPathFront apf0 |
     flowCandStore(node, f, toReturn, apf0, config) and
     apf0.headUsesContent(f) and
-    consCand(f, apf, unbind(config))
+    consCand(f, apf, config)
   )
   or
   exists(Content f, AccessPathFront apf0 |
     flowCandRead(node, f, toReturn, apf0, config) and
-    consCandFwd(f, apf0, unbind(config)) and
+    consCandFwd(f, apf0, config) and
     apf.headUsesContent(f)
   )
 }
@@ -1221,8 +1222,8 @@ private newtype TAccessPath =
   TConsCons(Content f1, Content f2, int len) { consCand(f1, TFrontHead(f2), _) and len in [2 .. 5] }
 
 /**
- * Conceptually a list of `Content`s followed by a `Type`, but only the first
- * element of the list and its length are tracked. If data flows from a source to
+ * Conceptually a list of `Content`s followed by a `Type`, but only the first two
+ * elements of the list and its length are tracked. If data flows from a source to
  * a given node with a given `AccessPath`, this indicates the sequence of
  * dereference operations needed to get from the value in the node to the
  * tracked object. The final type indicates the type of the tracked object.
@@ -1260,7 +1261,7 @@ abstract private class AccessPath extends TAccessPath {
 
 private class AccessPathNil extends AccessPath, TNil {
   override string toString() {
-    exists(DataFlowType t | this = TNil(t) | result = concat(" : " + ppReprType(t)))
+    exists(DataFlowType t | this = TNil(t) | result = concat(": " + ppReprType(t)))
   }
 
   override AccessPathFront getFront() {
@@ -1276,7 +1277,7 @@ private class AccessPathConsNil extends AccessPathCons, TConsNil {
   override string toString() {
     exists(Content f, DataFlowType t | this = TConsNil(f, t) |
       // The `concat` becomes "" if `ppReprType` has no result.
-      result = f.toString() + concat(" : " + ppReprType(t))
+      result = "[" + f.toString() + "]" + concat(" : " + ppReprType(t))
     )
   }
 
@@ -1293,8 +1294,8 @@ private class AccessPathConsCons extends AccessPathCons, TConsCons {
   override string toString() {
     exists(Content f1, Content f2, int len | this = TConsCons(f1, f2, len) |
       if len = 2
-      then result = f1.toString() + ", " + f2.toString()
-      else result = f1.toString() + ", " + f2.toString() + ", ... (" + len.toString() + ")"
+      then result = "[" + f1.toString() + ", " + f2.toString() + "]"
+      else result = "[" + f1.toString() + ", " + f2.toString() + ", ... (" + len.toString() + ")]"
     )
   }
 
@@ -1625,7 +1626,7 @@ abstract class PathNode extends TPathNode {
     this instanceof PathNodeSink and result = ""
     or
     exists(string s | s = this.(PathNodeMid).getAp().toString() |
-      if s = "" then result = "" else result = " [" + s + "]"
+      if s = "" then result = "" else result = " " + s
     )
   }
 
@@ -2070,7 +2071,7 @@ private module FlowExploration {
 
   private class PartialAccessPathNil extends PartialAccessPath, TPartialNil {
     override string toString() {
-      exists(DataFlowType t | this = TPartialNil(t) | result = concat(" : " + ppReprType(t)))
+      exists(DataFlowType t | this = TPartialNil(t) | result = concat(": " + ppReprType(t)))
     }
 
     override AccessPathFront getFront() {
@@ -2082,8 +2083,8 @@ private module FlowExploration {
     override string toString() {
       exists(Content f, int len | this = TPartialCons(f, len) |
         if len = 1
-        then result = f.toString()
-        else result = f.toString() + ", ... (" + len.toString() + ")"
+        then result = "[" + f.toString() + "]"
+        else result = "[" + f.toString() + ", ... (" + len.toString() + ")]"
       )
     }
 
@@ -2160,7 +2161,7 @@ private module FlowExploration {
 
     private string ppAp() {
       exists(string s | s = this.(PartialPathNodePriv).getAp().toString() |
-        if s = "" then result = "" else result = " [" + s + "]"
+        if s = "" then result = "" else result = " " + s
       )
     }