Merge tag 'codeql-cli/latest' into auto/sync-main-pr

Compatible with the latest released version of the CodeQL CLI

python/ql/lib/CHANGELOG.md

@@ -1,3 +1,13 @@
+## 4.0.4
+
+### Minor Analysis Improvements
+
+- Added the methods `getMinArguments` and `getMaxArguments` to the `Function` class. These return the minimum and maximum positional arguments that the given function accepts.
+
+### Bug Fixes
+
+- `MatchLiteralPattern`s such as `case None: ...` are now never pruned from the extracted source code. This fixes some situations where code was wrongly identified as unreachable.
+
 ## 4.0.3
 
 No user-facing changes.

python/ql/lib/change-notes/released/4.0.4.md

@@ -0,0 +1,9 @@
+## 4.0.4
+
+### Minor Analysis Improvements
+
+- Added the methods `getMinArguments` and `getMaxArguments` to the `Function` class. These return the minimum and maximum positional arguments that the given function accepts.
+
+### Bug Fixes
+
+- `MatchLiteralPattern`s such as `case None: ...` are now never pruned from the extracted source code. This fixes some situations where code was wrongly identified as unreachable.

python/ql/lib/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 4.0.3
+lastReleaseVersion: 4.0.4

python/ql/lib/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/python-all
-version: 4.0.3
+version: 4.0.4
 groups: python
 dbscheme: semmlecode.python.dbscheme
 extractor: python

python/ql/lib/semmle/python/Concepts.qll

@@ -181,7 +181,7 @@ module Path {
     }
   }
 
-  /** A data-flow node that checks that a path is safe to access. */
+  /** A data-flow node that checks that a path is safe to access in some way, for example by having a controlled prefix. */
   class SafeAccessCheck extends DataFlow::ExprNode {
     SafeAccessCheck() { this = DataFlow::BarrierGuard<safeAccessCheck/3>::getABarrierNode() }
   }
@@ -192,7 +192,7 @@ module Path {
 
   /** Provides a class for modeling new path safety checks. */
   module SafeAccessCheck {
-    /** A data-flow node that checks that a path is safe to access. */
+    /** A data-flow node that checks that a path is safe to access in some way, for example by having a controlled prefix. */
     abstract class Range extends DataFlow::GuardNode {
       /** Holds if this guard validates `node` upon evaluating to `branch`. */
       abstract predicate checks(ControlFlowNode node, boolean branch);

python/ql/lib/semmle/python/Exprs.qll

@@ -746,6 +746,24 @@ class Guard extends Guard_ {
   override Expr getASubExpression() { result = this.getTest() }
 }
 
+/** An annotation, such as the `int` part of `x: int` */
+class Annotation extends Expr {
+  Annotation() {
+    this = any(AnnAssign a).getAnnotation()
+    or
+    exists(Arguments args |
+      this in [
+          args.getAnAnnotation(),
+          args.getAKwAnnotation(),
+          args.getKwargannotation(),
+          args.getVarargannotation()
+        ]
+    )
+    or
+    this = any(FunctionExpr f).getReturns()
+  }
+}
+
 /* Expression Contexts */
 /** A context in which an expression used */
 class ExprContext extends ExprContext_ { }

python/ql/lib/semmle/python/Function.qll

@@ -163,6 +163,24 @@ class Function extends Function_, Scope, AstNode {
       ret.getValue() = result.getNode()
     )
   }
+
+  /** Gets the minimum number of positional arguments that can be correctly passed to this function. */
+  int getMinPositionalArguments() {
+    result = count(this.getAnArg()) - count(this.getDefinition().getArgs().getADefault())
+  }
+
+  /**
+   * Gets the maximum number of positional arguments that can be correctly passed to this function.
+   *
+   * If the function has a `*vararg` parameter, there is no upper limit on the number of positional
+   * arguments that can be passed to the function. In this case, this method returns a very large
+   * number (currently `INT_MAX`, 2147483647, but this may change in the future).
+   */
+  int getMaxPositionalArguments() {
+    if exists(this.getVararg())
+    then result = 2147483647 // INT_MAX
+    else result = count(this.getAnArg())
+  }
 }
 
 /** A def statement. Note that FunctionDef extends Assign as a function definition binds the newly created function */

python/ql/lib/semmle/python/objects/ObjectAPI.qll

@@ -738,21 +738,9 @@ class PythonFunctionValue extends FunctionValue {
     else result = "function " + this.getQualifiedName()
   }
 
-  override int minParameters() {
-    exists(Function f |
-      f = this.getScope() and
-      result = count(f.getAnArg()) - count(f.getDefinition().getArgs().getADefault())
-    )
-  }
+  override int minParameters() { result = this.getScope().getMinPositionalArguments() }
 
-  override int maxParameters() {
-    exists(Function f |
-      f = this.getScope() and
-      if exists(f.getVararg())
-      then result = 2147483647 // INT_MAX
-      else result = count(f.getAnArg())
-    )
-  }
+  override int maxParameters() { result = this.getScope().getMaxPositionalArguments() }
 
   /** Gets a control flow node corresponding to a return statement in this function */
   ControlFlowNode getAReturnedNode() { result = this.getScope().getAReturnValueFlowNode() }

python/ql/src/CHANGELOG.md

@@ -1,3 +1,13 @@
+## 1.4.6
+
+### Minor Analysis Improvements
+
+- The `py/special-method-wrong-signature` has been modernized and rewritten to no longer rely on outdated APIs. Moreover, the query no longer flags cases where a default value is never used, as these alerts were rarely useful.
+
+### Bug Fixes
+
+- The `py/unused-global-variable` now no longer flags variables that are only used in forward references (e.g. the `Foo` in `def bar(x: "Foo"): ...`).
+
 ## 1.4.5
 
 No user-facing changes.

python/ql/src/Functions/SignatureSpecialMethods.ql

@@ -4,6 +4,7 @@
  * @kind problem
  * @tags reliability
  *       correctness
+ *       quality
  * @problem.severity error
  * @sub-severity low
  * @precision high
@@ -11,12 +12,15 @@
  */
 
 import python
+import semmle.python.dataflow.new.internal.DataFlowDispatch as DD
 
 predicate is_unary_op(string name) {
   name in [
       "__del__", "__repr__", "__neg__", "__pos__", "__abs__", "__invert__", "__complex__",
       "__int__", "__float__", "__long__", "__oct__", "__hex__", "__str__", "__index__", "__enter__",
-      "__hash__", "__bool__", "__nonzero__", "__unicode__", "__len__", "__iter__", "__reversed__"
+      "__hash__", "__bool__", "__nonzero__", "__unicode__", "__len__", "__iter__", "__reversed__",
+      "__aenter__", "__aiter__", "__anext__", "__await__", "__ceil__", "__floor__", "__trunc__",
+      "__length_hint__", "__dir__", "__bytes__"
     ]
 }
 
@@ -28,91 +32,138 @@ predicate is_binary_op(string name) {
       "__and__", "__xor__", "__or__", "__ne__", "__radd__", "__rsub__", "__rmul__", "__rfloordiv__",
       "__rdiv__", "__rtruediv__", "__rmod__", "__rdivmod__", "__rpow__", "__rlshift__", "__gt__",
       "__rrshift__", "__rand__", "__rxor__", "__ror__", "__iadd__", "__isub__", "__imul__",
-      "__ifloordiv__", "__idiv__", "__itruediv__", "__ge__", "__imod__", "__idivmod__", "__ipow__",
-      "__ilshift__", "__irshift__", "__iand__", "__ixor__", "__ior__", "__coerce__", "__cmp__",
-      "__rcmp__", "__getattr___", "__getattribute___"
+      "__ifloordiv__", "__idiv__", "__itruediv__", "__ge__", "__imod__", "__ipow__", "__ilshift__",
+      "__irshift__", "__iand__", "__ixor__", "__ior__", "__coerce__", "__cmp__", "__rcmp__",
+      "__getattr__", "__getattribute__", "__buffer__", "__release_buffer__", "__matmul__",
+      "__rmatmul__", "__imatmul__", "__missing__", "__class_getitem__", "__mro_entries__",
+      "__format__"
     ]
 }
 
 predicate is_ternary_op(string name) {
-  name in ["__setattr__", "__set__", "__setitem__", "__getslice__", "__delslice__"]
+  name in ["__setattr__", "__set__", "__setitem__", "__getslice__", "__delslice__", "__set_name__"]
 }
 
-predicate is_quad_op(string name) { name = "__setslice__" or name = "__exit__" }
+predicate is_quad_op(string name) { name in ["__setslice__", "__exit__", "__aexit__"] }
 
-int argument_count(PythonFunctionValue f, string name, ClassValue cls) {
-  cls.declaredAttribute(name) = f and
-  (
-    is_unary_op(name) and result = 1
-    or
-    is_binary_op(name) and result = 2
-    or
-    is_ternary_op(name) and result = 3
-    or
-    is_quad_op(name) and result = 4
-  )
+int argument_count(string name) {
+  is_unary_op(name) and result = 1
+  or
+  is_binary_op(name) and result = 2
+  or
+  is_ternary_op(name) and result = 3
+  or
+  is_quad_op(name) and result = 4
+}
+
+/**
+ * Returns 1 if `func` is a static method, and 0 otherwise. This predicate is used to adjust the
+ * number of expected arguments for a special method accordingly.
+ */
+int staticmethod_correction(Function func) {
+  if DD::isStaticmethod(func) then result = 1 else result = 0
 }
 
 predicate incorrect_special_method_defn(
-  PythonFunctionValue func, string message, boolean show_counts, string name, ClassValue owner
+  Function func, string message, boolean show_counts, string name, boolean is_unused_default
 ) {
-  exists(int required | required = argument_count(func, name, owner) |
+  exists(int required, int correction |
+    required = argument_count(name) - correction and correction = staticmethod_correction(func)
+  |
     /* actual_non_default <= actual */
-    if required > func.maxParameters()
-    then message = "Too few parameters" and show_counts = true
+    if required > func.getMaxPositionalArguments()
+    then message = "Too few parameters" and show_counts = true and is_unused_default = false
     else
-      if required < func.minParameters()
-      then message = "Too many parameters" and show_counts = true
+      if required < func.getMinPositionalArguments()
+      then message = "Too many parameters" and show_counts = true and is_unused_default = false
       else (
-        func.minParameters() < required and
-        not func.getScope().hasVarArg() and
-        message = (required - func.minParameters()) + " default values(s) will never be used" and
-        show_counts = false
+        func.getMinPositionalArguments() < required and
+        not func.hasVarArg() and
+        message =
+          (required - func.getMinPositionalArguments()) + " default values(s) will never be used" and
+        show_counts = false and
+        is_unused_default = true
       )
   )
 }
 
-predicate incorrect_pow(FunctionValue func, string message, boolean show_counts, ClassValue owner) {
-  owner.declaredAttribute("__pow__") = func and
-  (
-    func.maxParameters() < 2 and message = "Too few parameters" and show_counts = true
+predicate incorrect_pow(
+  Function func, string message, boolean show_counts, boolean is_unused_default
+) {
+  exists(int correction | correction = staticmethod_correction(func) |
+    func.getMaxPositionalArguments() < 2 - correction and
+    message = "Too few parameters" and
+    show_counts = true and
+    is_unused_default = false
     or
-    func.minParameters() > 3 and message = "Too many parameters" and show_counts = true
+    func.getMinPositionalArguments() > 3 - correction and
+    message = "Too many parameters" and
+    show_counts = true and
+    is_unused_default = false
     or
-    func.minParameters() < 2 and
-    message = (2 - func.minParameters()) + " default value(s) will never be used" and
-    show_counts = false
+    func.getMinPositionalArguments() < 2 - correction and
+    message = (2 - func.getMinPositionalArguments()) + " default value(s) will never be used" and
+    show_counts = false and
+    is_unused_default = true
     or
-    func.minParameters() = 3 and
+    func.getMinPositionalArguments() = 3 - correction and
     message = "Third parameter to __pow__ should have a default value" and
-    show_counts = false
+    show_counts = false and
+    is_unused_default = false
   )
 }
 
-predicate incorrect_get(FunctionValue func, string message, boolean show_counts, ClassValue owner) {
-  owner.declaredAttribute("__get__") = func and
-  (
-    func.maxParameters() < 3 and message = "Too few parameters" and show_counts = true
+predicate incorrect_round(
+  Function func, string message, boolean show_counts, boolean is_unused_default
+) {
+  exists(int correction | correction = staticmethod_correction(func) |
+    func.getMaxPositionalArguments() < 1 - correction and
+    message = "Too few parameters" and
+    show_counts = true and
+    is_unused_default = false
     or
-    func.minParameters() > 3 and message = "Too many parameters" and show_counts = true
+    func.getMinPositionalArguments() > 2 - correction and
+    message = "Too many parameters" and
+    show_counts = true and
+    is_unused_default = false
     or
-    func.minParameters() < 2 and
-    not func.getScope().hasVarArg() and
-    message = (2 - func.minParameters()) + " default value(s) will never be used" and
-    show_counts = false
+    func.getMinPositionalArguments() = 2 - correction and
+    message = "Second parameter to __round__ should have a default value" and
+    show_counts = false and
+    is_unused_default = false
   )
 }
 
-string should_have_parameters(PythonFunctionValue f, string name, ClassValue owner) {
-  exists(int i | i = argument_count(f, name, owner) | result = i.toString())
-  or
-  owner.declaredAttribute(name) = f and
-  (name = "__get__" or name = "__pow__") and
-  result = "2 or 3"
+predicate incorrect_get(
+  Function func, string message, boolean show_counts, boolean is_unused_default
+) {
+  exists(int correction | correction = staticmethod_correction(func) |
+    func.getMaxPositionalArguments() < 3 - correction and
+    message = "Too few parameters" and
+    show_counts = true and
+    is_unused_default = false
+    or
+    func.getMinPositionalArguments() > 3 - correction and
+    message = "Too many parameters" and
+    show_counts = true and
+    is_unused_default = false
+    or
+    func.getMinPositionalArguments() < 2 - correction and
+    not func.hasVarArg() and
+    message = (2 - func.getMinPositionalArguments()) + " default value(s) will never be used" and
+    show_counts = false and
+    is_unused_default = true
+  )
 }
 
-string has_parameters(PythonFunctionValue f) {
-  exists(int i | i = f.minParameters() |
+string should_have_parameters(string name) {
+  if name in ["__pow__", "__get__"]
+  then result = "2 or 3"
+  else result = argument_count(name).toString()
+}
+
+string has_parameters(Function f) {
+  exists(int i | i = f.getMinPositionalArguments() |
     i = 0 and result = "no parameters"
     or
     i = 1 and result = "1 parameter"
@@ -121,23 +172,44 @@ string has_parameters(PythonFunctionValue f) {
   )
 }
 
-from
-  PythonFunctionValue f, string message, string sizes, boolean show_counts, string name,
-  ClassValue owner
-where
+/** Holds if `f` is likely to be a placeholder, and hence not interesting enough to report. */
+predicate isLikelyPlaceholderFunction(Function f) {
+  // Body has only a single statement.
+  f.getBody().getItem(0) = f.getBody().getLastItem() and
   (
-    incorrect_special_method_defn(f, message, show_counts, name, owner)
+    // Body is a string literal. This is a common pattern for Zope interfaces.
+    f.getBody().getLastItem().(ExprStmt).getValue() instanceof StringLiteral
     or
-    incorrect_pow(f, message, show_counts, owner) and name = "__pow__"
+    // Body just raises an exception.
+    f.getBody().getLastItem() instanceof Raise
     or
-    incorrect_get(f, message, show_counts, owner) and name = "__get__"
+    // Body is a pass statement.
+    f.getBody().getLastItem() instanceof Pass
+  )
+}
+
+from
+  Function f, string message, string sizes, boolean show_counts, string name, Class owner,
+  boolean show_unused_defaults
+where
+  owner.getAMethod() = f and
+  f.getName() = name and
+  (
+    incorrect_special_method_defn(f, message, show_counts, name, show_unused_defaults)
+    or
+    incorrect_pow(f, message, show_counts, show_unused_defaults) and name = "__pow__"
+    or
+    incorrect_get(f, message, show_counts, show_unused_defaults) and name = "__get__"
+    or
+    incorrect_round(f, message, show_counts, show_unused_defaults) and
+    name = "__round__"
   ) and
+  not isLikelyPlaceholderFunction(f) and
+  show_unused_defaults = false and
   (
     show_counts = false and sizes = ""
     or
     show_counts = true and
-    sizes =
-      ", which has " + has_parameters(f) + ", but should have " +
-        should_have_parameters(f, name, owner)
+    sizes = ", which has " + has_parameters(f) + ", but should have " + should_have_parameters(name)
   )
 select f, message + " for special method " + name + sizes + ", in class $@.", owner, owner.getName()

python/ql/src/Metrics/Internal/TypeAnnotations.ql

@@ -0,0 +1,108 @@
+/**
+ * @name Type metrics
+ * @description Counts of various kinds of type annotations in Python code.
+ * @kind table
+ * @id py/type-metrics
+ */
+
+import python
+
+class BuiltinType extends Name {
+  BuiltinType() { this.getId() in ["int", "float", "str", "bool", "bytes", "None"] }
+}
+
+newtype TAnnotatable =
+  TAnnotatedFunction(FunctionExpr f) { exists(f.getReturns()) } or
+  TAnnotatedParameter(Parameter p) { exists(p.getAnnotation()) } or
+  TAnnotatedAssignment(AnnAssign a) { exists(a.getAnnotation()) }
+
+abstract class Annotatable extends TAnnotatable {
+  string toString() { result = "Annotatable" }
+
+  abstract Expr getAnnotation();
+}
+
+class AnnotatedFunction extends TAnnotatedFunction, Annotatable {
+  FunctionExpr function;
+
+  AnnotatedFunction() { this = TAnnotatedFunction(function) }
+
+  override Expr getAnnotation() { result = function.getReturns() }
+}
+
+class AnnotatedParameter extends TAnnotatedParameter, Annotatable {
+  Parameter parameter;
+
+  AnnotatedParameter() { this = TAnnotatedParameter(parameter) }
+
+  override Expr getAnnotation() { result = parameter.getAnnotation() }
+}
+
+class AnnotatedAssignment extends TAnnotatedAssignment, Annotatable {
+  AnnAssign assignment;
+
+  AnnotatedAssignment() { this = TAnnotatedAssignment(assignment) }
+
+  override Expr getAnnotation() { result = assignment.getAnnotation() }
+}
+
+/** Holds if `e` is a forward declaration of a type. */
+predicate is_forward_declaration(Expr e) { e instanceof StringLiteral }
+
+/** Holds if `e` is a type that may be difficult to analyze. */
+predicate is_complex_type(Expr e) {
+  e instanceof Subscript and not is_optional_type(e)
+  or
+  e instanceof Tuple
+  or
+  e instanceof List
+}
+
+/** Holds if `e` is a type of the form `Optional[...]`. */
+predicate is_optional_type(Subscript e) { e.getObject().(Name).getId() = "Optional" }
+
+/** Holds if `e` is a simple type, that is either an identifier (excluding built-in types) or an attribute of a simple type. */
+predicate is_simple_type(Expr e) {
+  e instanceof Name and not e instanceof BuiltinType
+  or
+  is_simple_type(e.(Attribute).getObject())
+}
+
+/** Holds if `e` is a built-in type. */
+predicate is_builtin_type(Expr e) { e instanceof BuiltinType }
+
+predicate type_count(
+  string kind, int total, int built_in_count, int forward_declaration_count, int simple_type_count,
+  int complex_type_count, int optional_type_count
+) {
+  kind = "Parameter annotation" and
+  total = count(AnnotatedParameter p) and
+  built_in_count = count(AnnotatedParameter p | is_builtin_type(p.getAnnotation())) and
+  forward_declaration_count =
+    count(AnnotatedParameter p | is_forward_declaration(p.getAnnotation())) and
+  simple_type_count = count(AnnotatedParameter p | is_simple_type(p.getAnnotation())) and
+  complex_type_count = count(AnnotatedParameter p | is_complex_type(p.getAnnotation())) and
+  optional_type_count = count(AnnotatedParameter p | is_optional_type(p.getAnnotation()))
+  or
+  kind = "Return type annotation" and
+  total = count(AnnotatedFunction f) and
+  built_in_count = count(AnnotatedFunction f | is_builtin_type(f.getAnnotation())) and
+  forward_declaration_count = count(AnnotatedFunction f | is_forward_declaration(f.getAnnotation())) and
+  simple_type_count = count(AnnotatedFunction f | is_simple_type(f.getAnnotation())) and
+  complex_type_count = count(AnnotatedFunction f | is_complex_type(f.getAnnotation())) and
+  optional_type_count = count(AnnotatedFunction f | is_optional_type(f.getAnnotation()))
+  or
+  kind = "Annotated assignment" and
+  total = count(AnnotatedAssignment a) and
+  built_in_count = count(AnnotatedAssignment a | is_builtin_type(a.getAnnotation())) and
+  forward_declaration_count =
+    count(AnnotatedAssignment a | is_forward_declaration(a.getAnnotation())) and
+  simple_type_count = count(AnnotatedAssignment a | is_simple_type(a.getAnnotation())) and
+  complex_type_count = count(AnnotatedAssignment a | is_complex_type(a.getAnnotation())) and
+  optional_type_count = count(AnnotatedAssignment a | is_optional_type(a.getAnnotation()))
+}
+
+from
+  string message, int total, int built_in, int forward_decl, int simple, int complex, int optional
+where type_count(message, total, built_in, forward_decl, simple, complex, optional)
+select message, total, built_in, forward_decl, simple, complex, optional

python/ql/src/Resources/FileNotAlwaysClosed.py

@@ -1,15 +0,0 @@
-f = open("filename")
-    ... # Actions to perform on file
-f.close()
-# File only closed if actions are completed successfully
-
-with open("filename") as f:
-    ...# Actions to perform on file
-# File always closed
-
-f = open("filename")
-try:
-    ... # Actions to perform on file
-finally:
-    f.close()
-# File always closed

python/ql/src/Resources/FileNotAlwaysClosed.qhelp

@@ -4,32 +4,30 @@
 <qhelp>
 
 <overview>
-<p> If a file is opened then it should always be closed again, even if an
-exception is raised.
-Failing to ensure that all files are closed may result in failure due to too
-many open files.</p>
-
+<p>When a file is opened, it should always be closed. 
+</p>
+<p>A file opened for writing that is not closed when the application exits may result in data loss, where not all of the data written may be saved to the file.
+A file opened for reading or writing that is not closed may also use up file descriptors, which is a resource leak that in long running applications could lead to a failure to open additional files. 
+</p>
 </overview>
 <recommendation>
 
-<p>Ensure that if you open a file it is always closed on exiting the method.
-Wrap the code between the <code>open()</code> and <code>close()</code>
-functions in a <code>with</code> statement or use a <code>try...finally</code>
-statement. Using a <code>with</code> statement is preferred as it is shorter
-and more readable.</p>
+<p>Ensure that opened files are always closed, including when an exception could be raised.
+The best practice is often to use a <code>with</code> statement to automatically clean up resources.
+Otherwise, ensure that <code>.close()</code> is called in a <code>try...except</code> or <code>try...finally</code> 
+block to handle any possible exceptions.
+</p>
 
 </recommendation>
 <example>
-<p>The following code shows examples of different ways of closing a file. In the first example, the
-file is closed only if the method is exited successfully. In the other examples, the file is always
-closed on exiting the method.</p>
+<p>In the following examples, in the case marked BAD, the file may not be closed if an exception is raised. In the cases marked GOOD, the file is always closed.</p>
 
-<sample src="FileNotAlwaysClosed.py" />
+<sample src="examples/FileNotAlwaysClosed.py" />
 
 </example>
 <references>
 
-
+<li>Python Documentation: <a href="https://docs.python.org/3/tutorial/inputoutput.html#reading-and-writing-files">Reading and writing files</a>.</li>
 <li>Python Language Reference: <a href="http://docs.python.org/reference/compound_stmts.html#the-with-statement">The with statement</a>,
  <a href="http://docs.python.org/reference/compound_stmts.html#the-try-statement">The try statement</a>.</li>
 <li>Python PEP 343: <a href="http://www.python.org/dev/peps/pep-0343">The "with" Statement</a>.</li>

python/ql/src/Resources/FileNotAlwaysClosed.ql

@@ -1,74 +1,26 @@
 /**
  * @name File is not always closed
- * @description Opening a file without ensuring that it is always closed may cause resource leaks.
+ * @description Opening a file without ensuring that it is always closed may lead to data loss or resource leaks.
  * @kind problem
  * @tags efficiency
  *       correctness
  *       resources
+ *       quality
  *       external/cwe/cwe-772
  * @problem.severity warning
  * @sub-severity high
- * @precision medium
+ * @precision high
  * @id py/file-not-closed
  */
 
 import python
-import FileOpen
+import FileNotAlwaysClosedQuery
 
-/**
- * Whether resource is opened and closed in  in a matched pair of methods,
- * either `__enter__` and `__exit__` or `__init__` and `__del__`
- */
-predicate opened_in_enter_closed_in_exit(ControlFlowNode open) {
-  file_not_closed_at_scope_exit(open) and
-  exists(FunctionValue entry, FunctionValue exit |
-    open.getScope() = entry.getScope() and
-    exists(ClassValue cls |
-      cls.declaredAttribute("__enter__") = entry and cls.declaredAttribute("__exit__") = exit
-      or
-      cls.declaredAttribute("__init__") = entry and cls.declaredAttribute("__del__") = exit
-    ) and
-    exists(AttrNode attr_open, AttrNode attrclose |
-      attr_open.getScope() = entry.getScope() and
-      attrclose.getScope() = exit.getScope() and
-      expr_is_open(attr_open.(DefinitionNode).getValue(), open) and
-      attr_open.getName() = attrclose.getName() and
-      close_method_call(_, attrclose)
-    )
-  )
-}
-
-predicate file_not_closed_at_scope_exit(ControlFlowNode open) {
-  exists(EssaVariable v |
-    BaseFlow::reaches_exit(v) and
-    var_is_open(v, open) and
-    not file_is_returned(v, open)
-  )
-  or
-  call_to_open(open) and
-  not exists(AssignmentDefinition def | def.getValue() = open) and
-  not exists(Return r | r.getValue() = open.getNode())
-}
-
-predicate file_not_closed_at_exception_exit(ControlFlowNode open, ControlFlowNode exit) {
-  exists(EssaVariable v |
-    exit.(RaisingNode).viableExceptionalExit(_, _) and
-    not closes_arg(exit, v.getSourceVariable()) and
-    not close_method_call(exit, v.getAUse().(NameNode)) and
-    var_is_open(v, open) and
-    v.getAUse() = exit.getAChild*()
-  )
-}
-
-/* Check to see if a file is opened but not closed or returned */
-from ControlFlowNode defn, string message
+from FileOpen fo, string msg
 where
-  not opened_in_enter_closed_in_exit(defn) and
-  (
-    file_not_closed_at_scope_exit(defn) and message = "File is opened but is not closed."
-    or
-    not file_not_closed_at_scope_exit(defn) and
-    file_not_closed_at_exception_exit(defn, _) and
-    message = "File may not be closed if an exception is raised."
-  )
-select defn.getNode(), message
+  fileNotClosed(fo) and
+  msg = "File is opened but is not closed."
+  or
+  fileMayNotBeClosedOnException(fo, _) and
+  msg = "File may not be closed if an exception is raised."
+select fo, msg

python/ql/src/Resources/FileNotAlwaysClosedQuery.qll

@@ -0,0 +1,150 @@
+/** Definitions for reasoning about whether files are closed. */
+
+import python
+import semmle.python.dataflow.new.internal.DataFlowDispatch
+import semmle.python.ApiGraphs
+
+/** A CFG node where a file is opened. */
+abstract class FileOpenSource extends DataFlow::CfgNode { }
+
+/** A call to the builtin `open` or `os.open`. */
+class FileOpenCall extends FileOpenSource {
+  FileOpenCall() {
+    this = [API::builtin("open").getACall(), API::moduleImport("os").getMember("open").getACall()]
+  }
+}
+
+private DataFlow::TypeTrackingNode fileOpenInstance(DataFlow::TypeTracker t) {
+  t.start() and
+  result instanceof FileOpenSource
+  or
+  exists(DataFlow::TypeTracker t2 | result = fileOpenInstance(t2).track(t2, t))
+}
+
+/**
+ * A call that returns an instance of an open file object.
+ * This includes calls to methods that transitively call `open` or similar.
+ */
+class FileOpen extends DataFlow::CallCfgNode {
+  FileOpen() { fileOpenInstance(DataFlow::TypeTracker::end()).flowsTo(this) }
+}
+
+/** A call that may wrap a file object in a wrapper class or `os.fdopen`. */
+class FileWrapperCall extends DataFlow::CallCfgNode {
+  DataFlow::Node wrapped;
+
+  FileWrapperCall() {
+    wrapped = this.getArg(_).getALocalSource() and
+    this.getFunction() = classTracker(_)
+    or
+    wrapped = this.getArg(0) and
+    this = API::moduleImport("os").getMember("fdopen").getACall()
+    or
+    wrapped = this.getArg(0) and
+    this = API::moduleImport("django").getMember("http").getMember("FileResponse").getACall()
+  }
+
+  /** Gets the file that this call wraps. */
+  DataFlow::Node getWrapped() { result = wrapped }
+}
+
+/** A node where a file is closed. */
+abstract class FileClose extends DataFlow::CfgNode {
+  /** Holds if this file close will occur if an exception is thrown at `raises`. */
+  predicate guardsExceptions(DataFlow::CfgNode raises) {
+    this.asCfgNode() = raises.asCfgNode().getAnExceptionalSuccessor().getASuccessor*()
+    or
+    // The expression is after the close call.
+    // This also covers the body of a `with` statement.
+    raises.asCfgNode() = this.asCfgNode().getASuccessor*()
+  }
+}
+
+/** A call to the `.close()` method of a file object. */
+class FileCloseCall extends FileClose {
+  FileCloseCall() { exists(DataFlow::MethodCallNode mc | mc.calls(this, "close")) }
+}
+
+/** A call to `os.close`. */
+class OsCloseCall extends FileClose {
+  OsCloseCall() { this = API::moduleImport("os").getMember("close").getACall().getArg(0) }
+}
+
+/** A `with` statement. */
+class WithStatement extends FileClose {
+  WithStatement() { this.asExpr() = any(With w).getContextExpr() }
+}
+
+/** Holds if an exception may be raised at `raises` if `file` is a file object. */
+private predicate mayRaiseWithFile(DataFlow::CfgNode file, DataFlow::CfgNode raises) {
+  // Currently just consider any method called on `file`; e.g. `file.write()`; as potentially raising an exception
+  raises.(DataFlow::MethodCallNode).getObject() = file and
+  not file instanceof FileOpen and
+  not file instanceof FileClose
+}
+
+/** Holds if data flows from `nodeFrom` to `nodeTo` in one step that also includes file wrapper classes. */
+private predicate fileAdditionalLocalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+  exists(FileWrapperCall fw | nodeFrom = fw.getWrapped() and nodeTo = fw)
+}
+
+private predicate fileLocalFlowHelper0(
+  DataFlow::LocalSourceNode nodeFrom, DataFlow::LocalSourceNode nodeTo
+) {
+  exists(DataFlow::Node nodeMid |
+    nodeFrom.flowsTo(nodeMid) and fileAdditionalLocalFlowStep(nodeMid, nodeTo)
+  )
+}
+
+private predicate fileLocalFlowHelper1(
+  DataFlow::LocalSourceNode nodeFrom, DataFlow::LocalSourceNode nodeTo
+) {
+  fileLocalFlowHelper0*(nodeFrom, nodeTo)
+}
+
+/** Holds if data flows from `source` to `sink`, including file wrapper classes. */
+pragma[inline]
+private predicate fileLocalFlow(FileOpen source, DataFlow::Node sink) {
+  exists(DataFlow::LocalSourceNode mid | fileLocalFlowHelper1(source, mid) and mid.flowsTo(sink))
+}
+
+/** Holds if the file opened at `fo` is closed. */
+predicate fileIsClosed(FileOpen fo) { exists(FileClose fc | fileLocalFlow(fo, fc)) }
+
+/** Holds if the file opened at `fo` is returned to the caller. This makes the caller responsible for closing the file. */
+predicate fileIsReturned(FileOpen fo) {
+  exists(Return ret, Expr retVal |
+    (
+      retVal = ret.getValue()
+      or
+      retVal = ret.getValue().(List).getAnElt()
+      or
+      retVal = ret.getValue().(Tuple).getAnElt()
+    ) and
+    fileLocalFlow(fo, DataFlow::exprNode(retVal))
+  )
+}
+
+/** Holds if the file opened at `fo` is stored in a field. We assume that another method is then responsible for closing the file. */
+predicate fileIsStoredInField(FileOpen fo) {
+  exists(DataFlow::AttrWrite aw | fileLocalFlow(fo, aw.getValue()))
+}
+
+/** Holds if the file opened at `fo` is not closed, and is expected to be closed. */
+predicate fileNotClosed(FileOpen fo) {
+  not fileIsClosed(fo) and
+  not fileIsReturned(fo) and
+  not fileIsStoredInField(fo)
+}
+
+predicate fileMayNotBeClosedOnException(FileOpen fo, DataFlow::Node raises) {
+  fileIsClosed(fo) and
+  exists(DataFlow::CfgNode fileRaised |
+    mayRaiseWithFile(fileRaised, raises) and
+    fileLocalFlow(fo, fileRaised) and
+    not exists(FileClose fc |
+      fileLocalFlow(fo, fc) and
+      fc.guardsExceptions(raises)
+    )
+  )
+}

python/ql/src/Resources/FileOpen.qll

@@ -1,4 +1,8 @@
-/** Contains predicates concerning when and where files are opened and closed. */
+/**
+ * DEPRECATED: Use FileNotAlwaysClosedQuery instead.
+ * Contains predicates concerning when and where files are opened and closed.
+ */
+deprecated module;
 
 import python
 import semmle.python.pointsto.Filters

python/ql/src/Resources/examples/FileNotAlwaysClosed.py

@@ -0,0 +1,17 @@
+def bad():
+    f = open("filename", "w")
+    f.write("could raise exception") # BAD: This call could raise an exception, leading to the file not being closed.
+    f.close()
+
+
+def good1():
+    with open("filename", "w") as f:
+        f.write("always closed") # GOOD: The `with` statement ensures the file is always closed.
+
+def good2():
+    f = open("filename", "w")
+    try:
+       f.write("always closed")
+    finally:
+        f.close() # GOOD: The `finally` block always ensures the file is closed.
+   

python/ql/src/Security/CWE-918/ServerSideRequestForgery-end.inc.qhelp

@@ -5,8 +5,11 @@
   <recommendation>
 
     <p>To guard against SSRF attacks you should avoid putting user-provided input directly
-      into a request URL. Instead, either maintain a list of authorized URLs on the server and choose
-      from that list based on the input provided, or perform proper validation of the input.
+      into a request URL. On the application level, maintain a list of authorized URLs on the server and choose
+      from that list based on the input provided. If that is not possible, one should verify the IP address for all user-controlled 
+      requests to ensure they are not private. This requires saving the verified IP address of each domain,
+      then utilizing a custom HTTP adapter to ensure that future requests to that domain use the verified IP address. 
+      On the network level, you can segment the vulnerable application into its own LAN or block access to specific devices.
     </p>
 
   </recommendation>

python/ql/src/Variables/UnusedModuleVariable.ql

@@ -34,6 +34,14 @@ predicate complex_all(Module m) {
   )
 }
 
+predicate used_in_forward_declaration(Name used, Module mod) {
+  exists(StringLiteral s, Annotation annotation |
+    s.getS() = used.getId() and
+    s.getEnclosingModule() = mod and
+    annotation.getASubExpression*() = s
+  )
+}
+
 predicate unused_global(Name unused, GlobalVariable v) {
   not exists(ImportingStmt is | is.contains(unused)) and
   forex(DefinitionNode defn | defn.getNode() = unused |
@@ -55,7 +63,8 @@ predicate unused_global(Name unused, GlobalVariable v) {
     unused.defines(v) and
     not name_acceptable_for_unused_variable(v) and
     not complex_all(unused.getEnclosingModule())
-  )
+  ) and
+  not used_in_forward_declaration(unused, unused.getEnclosingModule())
 }
 
 from Name unused, GlobalVariable v

python/ql/src/change-notes/released/1.4.6.md

@@ -0,0 +1,9 @@
+## 1.4.6
+
+### Minor Analysis Improvements
+
+- The `py/special-method-wrong-signature` has been modernized and rewritten to no longer rely on outdated APIs. Moreover, the query no longer flags cases where a default value is never used, as these alerts were rarely useful.
+
+### Bug Fixes
+
+- The `py/unused-global-variable` now no longer flags variables that are only used in forward references (e.g. the `Foo` in `def bar(x: "Foo"): ...`).

python/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 1.4.5
+lastReleaseVersion: 1.4.6

python/ql/src/codeql-suites/python-ccr.qls

@@ -1 +0,0 @@
-[]

python/ql/src/codeql-suites/python-code-quality.qls

@@ -0,0 +1,5 @@
+- queries: .
+- include:
+    id:
+      - py/not-named-self 
+      - py/not-named-cls

python/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/python-queries
-version: 1.4.5
+version: 1.4.6
 groups:
   - python
   - queries

python/ql/test/query-tests/Functions/general/SignatureSpecialMethods.expected

@@ -1,9 +1,6 @@
-| om_test.py:59:5:59:28 | Function WrongSpecials.__div__ | Too many parameters for special method __div__, which has 3 parameters, but should have 2, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:62:5:62:22 | Function WrongSpecials.__mul__ | Too few parameters for special method __mul__, which has 1 parameter, but should have 2, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:65:5:65:29 | Function WrongSpecials.__neg__ | Too many parameters for special method __neg__, which has 2 parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:68:5:68:35 | Function WrongSpecials.__exit__ | Too few parameters for special method __exit__, which has 3 parameters, but should have 4, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:71:5:71:19 | Function WrongSpecials.__repr__ | Too few parameters for special method __repr__, which has no parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:74:5:74:46 | Function WrongSpecials.__add__ | 1 default values(s) will never be used for special method __add__, in class $@. | om_test.py:57:1:57:28 | class WrongSpecials | WrongSpecials |
-| om_test.py:97:15:97:34 | Function NotOKSpecials.lambda | Too few parameters for special method __sub__, which has 1 parameter, but should have 2, in class $@. | om_test.py:95:1:95:28 | class NotOKSpecials | NotOKSpecials |
-| protocols.py:107:1:107:12 | Function f | Too few parameters for special method __add__, which has 1 parameter, but should have 2, in class $@. | protocols.py:110:1:110:29 | class MissingMethods | MissingMethods |
-| protocols.py:107:1:107:12 | Function f | Too few parameters for special method __set__, which has 1 parameter, but should have 3, in class $@. | protocols.py:110:1:110:29 | class MissingMethods | MissingMethods |
+| om_test.py:59:5:59:28 | Function __div__ | Too many parameters for special method __div__, which has 3 parameters, but should have 2, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:62:5:62:22 | Function __mul__ | Too few parameters for special method __mul__, which has 1 parameter, but should have 2, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:65:5:65:29 | Function __neg__ | Too many parameters for special method __neg__, which has 2 parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:68:5:68:35 | Function __exit__ | Too few parameters for special method __exit__, which has 3 parameters, but should have 4, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:71:5:71:19 | Function __repr__ | Too few parameters for special method __repr__, which has no parameters, but should have 1, in class $@. | om_test.py:57:1:57:28 | Class WrongSpecials | WrongSpecials |
+| om_test.py:83:5:83:18 | Function __del__ | Too few parameters for special method __del__, which has no parameters, but should have 1, in class $@. | om_test.py:81:1:81:25 | Class OKSpecials | OKSpecials |

python/ql/test/query-tests/Functions/general/om_test.py

@@ -69,11 +69,11 @@ class WrongSpecials(object):
         return arg0 == arg1
 
     def __repr__():
-        pass
+        return ""
 
     def __add__(self, other="Unused default"):
-       pass
-   
+       return 4
+
     @staticmethod
     def __abs__():
         return 42
@@ -105,3 +105,7 @@ class LoggingDict(dict):
 
         
 
+class MoreSpecialMethods:
+    @staticmethod
+    def __abs__():
+        return 42

python/ql/test/query-tests/Resources/Dataflow.expected

@@ -1,119 +0,0 @@
-| f1_0 = open() | open |  |
-| f1_1 = MethodCallsiteRefinement(f1_0) | open |  |
-| f1_2 = MethodCallsiteRefinement(f1_1) | closed | exit |
-| f2_0 = open() | open | exit |
-| f3_0 = open() | open |  |
-| f3_1 = MethodCallsiteRefinement(f3_0) | closed | exit |
-| f4_0 = with | closed |  |
-| f4_1 = MethodCallsiteRefinement(f4_0) | closed | exit |
-| f5_0 = open() | open |  |
-| f5_1 = MethodCallsiteRefinement(f5_0) | open |  |
-| f5_2 = MethodCallsiteRefinement(f5_1) | closed | exit |
-| f5_3 = phi(f5_0, f5_1) | open |  |
-| f6_0 = None | closed |  |
-| f6_1 = open() | open |  |
-| f6_2 = MethodCallsiteRefinement(f6_1) | open |  |
-| f6_3 = phi(f6_0, f6_1, f6_2) | open |  |
-| f6_4 = Pi(f6_2) [true] | open |  |
-| f6_5 = MethodCallsiteRefinement(f6_4) | closed |  |
-| f6_6 = Pi(f6_3) [true] | open |  |
-| f6_7 = Pi(f6_2) [false] | closed |  |
-| f6_8 = phi(f6_5, f6_7) | closed | exit |
-| f7_0 = None | closed |  |
-| f7_1 = open() | open |  |
-| f7_2 = MethodCallsiteRefinement(f7_1) | open |  |
-| f7_3 = phi(f7_0, f7_1, f7_2) | open |  |
-| f7_4 = Pi(f7_2) [true] | open |  |
-| f7_5 = MethodCallsiteRefinement(f7_4) | closed |  |
-| f7_6 = Pi(f7_3) [true] | open |  |
-| f7_7 = Pi(f7_2) [false] | closed |  |
-| f7_8 = phi(f7_5, f7_7) | closed | exit |
-| f8_0 = None | closed |  |
-| f8_1 = open() | open |  |
-| f8_2 = MethodCallsiteRefinement(f8_1) | open |  |
-| f8_3 = phi(f8_0, f8_1, f8_2) | open |  |
-| f8_4 = Pi(f8_2) [true] | closed |  |
-| f8_5 = MethodCallsiteRefinement(f8_4) | closed |  |
-| f8_6 = Pi(f8_3) [true] | closed |  |
-| f8_7 = Pi(f8_2) [false] | open |  |
-| f8_8 = phi(f8_5, f8_7) | open | exit |
-| f9_0 = None | closed |  |
-| f9_1 = open() | open |  |
-| f9_2 = MethodCallsiteRefinement(f9_1) | open |  |
-| f9_3 = phi(f9_0, f9_1, f9_2) | open |  |
-| f9_4 = Pi(f9_2) [true] | closed |  |
-| f9_5 = MethodCallsiteRefinement(f9_4) | closed |  |
-| f9_6 = Pi(f9_3) [true] | closed |  |
-| f9_7 = Pi(f9_2) [false] | open |  |
-| f9_8 = phi(f9_5, f9_7) | open | exit |
-| f10_0 = open() | open |  |
-| f10_1 = MethodCallsiteRefinement(f10_0) | open |  |
-| f10_2 = MethodCallsiteRefinement(f10_1) | open |  |
-| f10_3 = MethodCallsiteRefinement(f10_2) | closed |  |
-| f10_4 = phi(f10_0, f10_1, f10_2, f10_3) | open |  |
-| f10_5 = MethodCallsiteRefinement(f10_4) | closed |  |
-| f10_6 = phi(f10_3, f10_5) | closed | exit |
-| f11_0 = open() | open |  |
-| f11_1 = MethodCallsiteRefinement(f11_0) | open |  |
-| f11_2 = MethodCallsiteRefinement(f11_1) | open |  |
-| f11_3 = MethodCallsiteRefinement(f11_2) | closed |  |
-| f11_4 = phi(f11_0, f11_1, f11_2, f11_3) | open |  |
-| f11_5 = MethodCallsiteRefinement(f11_4) | closed |  |
-| f11_6 = phi(f11_3, f11_5) | closed | exit |
-| f12_0 = open() | open |  |
-| f12_1 = MethodCallsiteRefinement(f12_0) | open |  |
-| f12_2 = MethodCallsiteRefinement(f12_1) | open |  |
-| f12_3 = MethodCallsiteRefinement(f12_2) | closed |  |
-| f12_4 = phi(f12_0, f12_1, f12_2, f12_3) | open |  |
-| f12_5 = MethodCallsiteRefinement(f12_4) | closed |  |
-| f12_6 = phi(f12_3, f12_5) | closed | exit |
-| f13_0 = open() | open |  |
-| f13_1 = MethodCallsiteRefinement(f13_0) | open | exit |
-| f14_0 = opener_func2() | open |  |
-| f14_1 = MethodCallsiteRefinement(f14_0) | open |  |
-| f14_2 = MethodCallsiteRefinement(f14_1) | closed | exit |
-| f15_0 = opener_func2() | open |  |
-| f15_1 = ArgumentRefinement(f15_0) | closed | exit |
-| f16_0 = ScopeEntryDefinition | closed |  |
-| f16_1 = open() | open |  |
-| f16_2 = MethodCallsiteRefinement(f16_1) | open |  |
-| f16_3 = MethodCallsiteRefinement(f16_2) | closed |  |
-| f16_4 = phi(f16_0, f16_1, f16_2, f16_3) | open |  |
-| f16_5 = phi(f16_3, f16_4) | open | exit |
-| f17_0 = open() | open |  |
-| f17_1 = MethodCallsiteRefinement(f17_0) | open |  |
-| f17_2 = MethodCallsiteRefinement(f17_1) | open |  |
-| f17_3 = MethodCallsiteRefinement(f17_2) | closed |  |
-| f17_4 = phi(f17_0, f17_1, f17_2, f17_3) | open |  |
-| f17_5 = MethodCallsiteRefinement(f17_4) | closed |  |
-| f17_6 = phi(f17_3, f17_5) | closed | exit |
-| f18_0 = open() | closed |  |
-| f18_1 = MethodCallsiteRefinement(f18_0) | closed | exit |
-| f20_0 = open() | open |  |
-| f20_1 = ArgumentRefinement(f20_0) | closed | exit |
-| f21_0 = open() | open |  |
-| f21_1 = MethodCallsiteRefinement(f21_0) | open |  |
-| f21_2 = MethodCallsiteRefinement(f21_1) | closed |  |
-| f21_3 = phi(f21_1, f21_2) | open |  |
-| f21_4 = phi(f21_0, f21_1, f21_2) | open |  |
-| f21_5 = Pi(f21_3) [true] | open |  |
-| f21_6 = MethodCallsiteRefinement(f21_5) | closed |  |
-| f21_7 = Pi(f21_4) [true] | open |  |
-| f21_8 = Pi(f21_3) [false] | closed |  |
-| f21_9 = phi(f21_6, f21_8) | closed | exit |
-| f22_0 = open() | open |  |
-| f22_1 = MethodCallsiteRefinement(f22_0) | open |  |
-| f22_2 = MethodCallsiteRefinement(f22_1) | closed |  |
-| f22_3 = phi(f22_1, f22_2) | open |  |
-| f22_4 = phi(f22_0, f22_1, f22_2) | open |  |
-| f22_5 = Pi(f22_3) [true] | closed |  |
-| f22_6 = MethodCallsiteRefinement(f22_5) | closed |  |
-| f22_7 = Pi(f22_4) [true] | closed |  |
-| f22_8 = Pi(f22_3) [false] | open |  |
-| f22_9 = phi(f22_6, f22_8) | open | exit |
-| f_0 = FunctionExpr | closed | exit |
-| file_0 = open() | open |  |
-| file_1 = open() | open |  |
-| file_2 = None | closed |  |
-| file_3 = phi(file_0, file_1, file_2) | open | exit |
-| fp_0 = ParameterDefinition | closed | exit |

python/ql/test/query-tests/Resources/Dataflow.ql

@@ -1,14 +0,0 @@
-import python
-import Resources.FileOpen
-
-from EssaVariable v, EssaDefinition def, string open, string exit
-where
-  def = v.getDefinition() and
-  v.getSourceVariable().getName().charAt(0) = "f" and
-  (
-    var_is_open(v, _) and open = "open"
-    or
-    not var_is_open(v, _) and open = "closed"
-  ) and
-  if BaseFlow::reaches_exit(v) then exit = "exit" else exit = ""
-select v.getRepresentation() + " = " + v.getDefinition().getRepresentation(), open, exit

python/ql/test/query-tests/Resources/FileNotAlwaysClosed.expected

@@ -1,9 +0,0 @@
-| resources_test.py:4:10:4:25 | open() | File may not be closed if an exception is raised. |
-| resources_test.py:9:10:9:25 | open() | File is opened but is not closed. |
-| resources_test.py:49:14:49:29 | open() | File is opened but is not closed. |
-| resources_test.py:58:14:58:29 | open() | File is opened but is not closed. |
-| resources_test.py:79:11:79:26 | open() | File may not be closed if an exception is raised. |
-| resources_test.py:108:11:108:20 | open() | File is opened but is not closed. |
-| resources_test.py:112:11:112:28 | opener_func2() | File may not be closed if an exception is raised. |
-| resources_test.py:129:15:129:24 | open() | File is opened but is not closed. |
-| resources_test.py:237:11:237:26 | open() | File is opened but is not closed. |

python/ql/test/query-tests/Resources/FileNotAlwaysClosed.qlref

@@ -1 +0,0 @@
-Resources/FileNotAlwaysClosed.ql

python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py

@@ -1,12 +1,12 @@
 #File not always closed
 
 def not_close1():
-    f1 = open("filename")
+    f1 = open("filename") # $ notClosedOnException
     f1.write("Error could occur")
-    f1.close()
+    f1.close() 
 
 def not_close2():
-    f2 = open("filename")
+    f2 = open("filename") # $ notClosed
 
 def closed3():
     f3 = open("filename")
@@ -46,19 +46,19 @@ def closed7():
 def not_closed8():
     f8 = None
     try:
-        f8 = open("filename")
+        f8 = open("filename") # $ MISSING:notClosedOnException 
         f8.write("Error could occur")
     finally:
-        if f8 is None:
+        if f8 is None: # We don't precisely consider this condition, so this result is MISSING. However, this seems uncommon.
             f8.close()
 
 def not_closed9():
     f9 = None
     try:
-        f9 = open("filename")
+        f9 = open("filename") # $ MISSING:notAlwaysClosed
         f9.write("Error could occur")
     finally:
-        if not f9:
+        if not f9: # We don't precisely consider this condition, so this result is MISSING.However, this seems uncommon.
             f9.close()
 
 def not_closed_but_cant_tell_locally():
@@ -76,19 +76,19 @@ def closed10():
 
 #Not closed by handling the wrong exception
 def not_closed11():
-    f11 = open("filename")
+    f11 = open("filename") # $ MISSING:notAlwaysClosed
     try:
         f11.write("IOError could occur")
         f11.write("IOError could occur")
         f11.close()
-    except AttributeError:
+    except AttributeError: # We don't consider the type of exception handled here, so this result is MISSING.
         f11.close()
 
-def doesnt_raise():
+def doesnt_raise(*args):
     pass
 
 def mostly_closed12():
-    f12 = open("filename")
+    f12 = open("filename") 
     try:
         f12.write("IOError could occur")
         f12.write("IOError could occur")
@@ -105,11 +105,11 @@ def opener_func2(name):
     return t1
 
 def not_closed13(name):
-    f13 = open(name)
+    f13 = open(name) # $ notClosed
     f13.write("Hello")
 
 def may_not_be_closed14(name):
-    f14 = opener_func2(name)
+    f14 = opener_func2(name) # $ notClosedOnException
     f14.write("Hello")
     f14.close()
 
@@ -120,13 +120,13 @@ def closer2(t3):
     closer1(t3)
 
 def closed15():
-    f15 = opener_func2()
-    closer2(f15)
+    f15 = opener_func2() # $ SPURIOUS:notClosed
+    closer2(f15) # We don't detect that this call closes the file, so this result is SPURIOUS.
 
 
 def may_not_be_closed16(name):
     try:
-        f16 = open(name)
+        f16 = open(name) # $ notClosedOnException
         f16.write("Hello")
         f16.close()
     except IOError:
@@ -138,13 +138,13 @@ def may_raise():
 
 #Not handling all exceptions, but we'll tolerate the false negative
 def not_closed17():
-    f17 = open("filename")
+    f17 = open("filename") # $ MISSING:notClosedOnException
     try:
         f17.write("IOError could occur")
         f17.write("IOError could occur")
         may_raise("ValueError could occur") # FN here.
         f17.close()
-    except IOError:
+    except IOError: # We don't detect that a ValueErrror could be raised that isn't handled here, so this result is MISSING.
         f17.close()
 
 #ODASA-3779
@@ -234,13 +234,47 @@ def closed21(path):
 
 
 def not_closed22(path):
-    f22 = open(path, "wb")
+    f22 = open(path, "wb") # $ MISSING:notClosedOnException
     try:
         f22.write(b"foo")
         may_raise()
         if foo:
             f22.close()
     finally:
-        if f22.closed: # Wrong sense
+        if f22.closed: # We don't precisely consider this condition, so this result is MISSING. However, this seems uncommon.
             f22.close()
 
+def not_closed23(path):
+    f23 = open(path, "w") # $ notClosed
+    wr = FileWrapper(f23)
+
+def closed24(path):
+    f24 = open(path, "w")
+    try:
+        f24.write("hi")
+    except:
+        pass 
+    f24.close()
+
+def closed25(path):
+    from django.http import FileResponse 
+    return FileResponse(open(path))
+
+import os
+def closed26(path):
+    fd = os.open(path)
+    os.close(fd)
+
+def not_closed27(path):
+    fd = os.open(path, "w") # $notClosedOnException
+    f27 = os.fdopen(fd, "w")
+    f27.write("hi")
+    f27.close()
+
+def closed28(path):
+    fd = os.open(path, os.O_WRONLY) 
+    f28 = os.fdopen(fd, "w")
+    try:
+        f28.write("hi")
+    finally:
+        f28.close()

python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql

@@ -0,0 +1,25 @@
+import python
+import Resources.FileNotAlwaysClosedQuery
+import utils.test.InlineExpectationsTest
+
+module MethodArgTest implements TestSig {
+  string getARelevantTag() { result = ["notClosed", "notClosedOnException"] }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(DataFlow::CfgNode el, FileOpen fo |
+      el = fo and
+      element = el.toString() and
+      location = el.getLocation() and
+      value = "" and
+      (
+        fileNotClosed(fo) and
+        tag = "notClosed"
+        or
+        fileMayNotBeClosedOnException(fo, _) and
+        tag = "notClosedOnException"
+      )
+    )
+  }
+}
+
+import MakeTest<MethodArgTest>

python/ql/test/query-tests/Statements/unreachable/UnreachableCode.expected

@@ -4,6 +4,3 @@
 | test.py:21:5:21:38 | For | This statement is unreachable. |
 | test.py:28:9:28:21 | ExprStmt | This statement is unreachable. |
 | test.py:84:5:84:21 | ExceptStmt | This statement is unreachable. |
-| test.py:158:9:159:16 | Case | This statement is unreachable. |
-| test.py:162:13:162:16 | Pass | This statement is unreachable. |
-| test.py:167:13:167:16 | Pass | This statement is unreachable. |

python/ql/test/query-tests/Variables/unused/variables_test.py

@@ -137,3 +137,21 @@ def test_dict_unpacking(queryset, field_name, value):
     for tag in value.split(','):
         queryset = queryset.filter(**{field_name + '__name': tag})
     return queryset
+
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    ParamAnnotation = int
+    ReturnAnnotation = int
+    AssignmentAnnotation = int
+    ForwardParamAnnotation = int
+    ForwardReturnAnnotation = int
+    ForwardAssignmentAnnotation = int
+
+def test_direct_annotation(x: ParamAnnotation) -> ReturnAnnotation:
+    if x:
+        y : AssignmentAnnotation = 1
+
+def test_forward_annotation(x: "ForwardParamAnnotation") -> "ForwardReturnAnnotation":
+    if x:
+        y : "ForwardAssignmentAnnotation" = 1