Merge pull request #20263 from joefarebrother/python-qual-exceptions

Python: Modernize the Unreachable Except Block query

Author: joefarebrother

python/ql/lib/semmle/python/frameworks/builtins.model.yml

@@ -25,20 +25,20 @@ is a super class of <code>Error</code>.
 
 <p>Reorganize the <code>except</code> blocks so that the more specific <code>except</code> 
 is defined first. Alternatively, if the more specific <code>except</code> block is 
-no longer required then it should be deleted.</p>
+no longer required, then it should be deleted.</p>
 
 </recommendation>
 <example>
-<p>In this example the <code>except Exception:</code> will handle <code>AttributeError</code> preventing the
+<p>In the following example, the <code>except Exception:</code> will handle <code>AttributeError</code> preventing the
 subsequent handler from ever executing.</p>
 <sample src="IncorrectExceptOrder.py" />
 
 
 </example>
 <references>
 
-<li>Python Language Reference: <a href="http://docs.python.org/2.7/reference/compound_stmts.html#try">The try statement</a>, 
-<a href="http://docs.python.org/2.7/reference/executionmodel.html#exceptions">Exceptions</a>.</li>
+<li>Python Language Reference: <a href="http://docs.python.org/3/reference/compound_stmts.html#try">The try statement</a>, 
+<a href="http://docs.python.org/3/reference/executionmodel.html#exceptions">Exceptions</a>.</li>
 
 
 </references>

python/ql/src/Exceptions/IncorrectExceptOrder.qhelp

@@ -1,5 +1,5 @@
 /**
- * @name Unreachable 'except' block
+ * @name Unreachable `except` block
  * @description Handling general exceptions before specific exceptions means that the specific
  *              handlers are never executed.
  * @kind problem
@@ -14,22 +14,95 @@
  */
 
 import python
+import semmle.python.dataflow.new.internal.DataFlowDispatch
+import semmle.python.ApiGraphs
+import semmle.python.frameworks.data.internal.ApiGraphModels
 
-predicate incorrect_except_order(ExceptStmt ex1, ClassValue cls1, ExceptStmt ex2, ClassValue cls2) {
+predicate builtinException(string name) {
+  typeModel("builtins.BaseException~Subclass", "builtins." + name, "")
+}
+
+predicate builtinExceptionSubclass(string base, string sub) {
+  typeModel("builtins." + base + "~Subclass", "builtins." + sub, "")
+}
+
+newtype TExceptType =
+  TClass(Class c) or
+  TBuiltin(string name) { builtinException(name) }
+
+class ExceptType extends TExceptType {
+  Class asClass() { this = TClass(result) }
+
+  string asBuiltinName() { this = TBuiltin(result) }
+
+  predicate isBuiltin() { this = TBuiltin(_) }
+
+  string getName() {
+    result = this.asClass().getName()
+    or
+    result = this.asBuiltinName()
+  }
+
+  string toString() { result = this.getName() }
+
+  DataFlow::Node getAUse() {
+    result = classTracker(this.asClass())
+    or
+    API::builtin(this.asBuiltinName()).asSource().flowsTo(result)
+  }
+
+  ExceptType getADirectSuperclass() {
+    result.asClass() = getADirectSuperclass(this.asClass())
+    or
+    result.isBuiltin() and
+    result.getAUse().asExpr() = this.asClass().getABase()
+    or
+    builtinExceptionSubclass(result.asBuiltinName(), this.asBuiltinName()) and
+    this != result
+  }
+
+  /**
+   * Holds if this element is at the specified location.
+   * The location spans column `startColumn` of line `startLine` to
+   * column `endColumn` of line `endLine` in file `filepath`.
+   * For more information, see
+   * [Providing locations in CodeQL queries](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
+   */
+  predicate hasLocationInfo(
+    string filePath, int startLine, int startColumn, int endLine, int endColumn
+  ) {
+    this.asClass()
+        .getLocation()
+        .hasLocationInfo(filePath, startLine, startColumn, endLine, endColumn)
+    or
+    this.isBuiltin() and
+    filePath = "" and
+    startLine = 0 and
+    startColumn = 0 and
+    endLine = 0 and
+    endColumn = 0
+  }
+}
+
+predicate incorrectExceptOrder(ExceptStmt ex1, ExceptType cls1, ExceptStmt ex2, ExceptType cls2) {
   exists(int i, int j, Try t |
     ex1 = t.getHandler(i) and
     ex2 = t.getHandler(j) and
     i < j and
-    cls1 = except_class(ex1) and
-    cls2 = except_class(ex2) and
-    cls1 = cls2.getASuperType()
+    cls1 = exceptClass(ex1) and
+    cls2 = exceptClass(ex2) and
+    cls1 = cls2.getADirectSuperclass*()
   )
 }
 
-ClassValue except_class(ExceptStmt ex) { ex.getType().pointsTo(result) }
+ExceptType exceptClass(ExceptStmt ex) { ex.getType() = result.getAUse().asExpr() }
 
-from ExceptStmt ex1, ClassValue cls1, ExceptStmt ex2, ClassValue cls2
-where incorrect_except_order(ex1, cls1, ex2, cls2)
-select ex2,
-  "Except block for $@ is unreachable; the more general $@ for $@ will always be executed in preference.",
-  cls2, cls2.getName(), ex1, "except block", cls1, cls1.getName()
+from ExceptStmt ex1, ExceptType cls1, ExceptStmt ex2, ExceptType cls2, string msg
+where
+  incorrectExceptOrder(ex1, cls1, ex2, cls2) and
+  if cls1 = cls2
+  then msg = "This except block handling $@ is unreachable; as $@ also handles $@."
+  else
+    msg =
+      "This except block handling $@ is unreachable; as $@ for the more general $@ always subsumes it."
+select ex2, msg, cls2, cls2.getName(), ex1, "this except block", cls1, cls1.getName()

python/ql/src/Exceptions/IncorrectExceptOrder.ql

@@ -0,0 +1,31 @@
+from shared_subclass_functions import wrap_in_template
+import sys
+import yaml
+from pathlib import Path
+
+py_version = sys.version.split()[0]
+VERSION = f"process-builtin-exceptions 0.0.1; Python {py_version}"
+
+builtins_model_path = Path(__file__).parent.parent.parent.parent / "lib/semmle/python/frameworks/builtins.model.yml"
+
+def write_data(data, path: Path):
+    f = path.open("w+")
+    f.write(f"# {VERSION}\n")
+    yaml.dump(data, indent=2, stream=f, Dumper=yaml.CDumper)
+
+builtin_names = dir(__builtins__)
+builtin_dict = {x: getattr(__builtins__,x) for x in builtin_names}
+
+
+builtin_exceptions = {v for v in builtin_dict.values() if type(v) is type and issubclass(v, BaseException)}
+
+data = []
+for sub in builtin_exceptions:
+    for base in sub.__mro__:
+        if issubclass(base, BaseException):
+            basename = base.__name__ 
+            subname = sub.__name__ 
+            row = [f"builtins.{basename}~Subclass", f"builtins.{subname}", ""]
+            data.append(row)
+            
+write_data(wrap_in_template(data), builtins_model_path)

python/ql/src/meta/ClassHierarchy/process-builtin-exceptions.py

@@ -1,2 +1,2 @@
 | exceptions_test.py:7:5:7:11 | ExceptStmt | Except block directly handles BaseException. |
-| exceptions_test.py:71:5:71:25 | ExceptStmt | Except block directly handles BaseException. |
+| exceptions_test.py:97:5:97:25 | ExceptStmt | Except block directly handles BaseException. |

python/ql/test/query-tests/Exceptions/general/CatchingBaseException.expected

@@ -1,2 +1,5 @@
 | exceptions_test.py:7:5:7:11 | ExceptStmt | 'except' clause does nothing but pass and there is no explanatory comment. |
 | exceptions_test.py:13:5:13:21 | ExceptStmt | 'except' clause does nothing but pass and there is no explanatory comment. |
+| exceptions_test.py:72:1:72:18 | ExceptStmt | 'except' clause does nothing but pass and there is no explanatory comment. |
+| exceptions_test.py:85:1:85:17 | ExceptStmt | 'except' clause does nothing but pass and there is no explanatory comment. |
+| exceptions_test.py:89:1:89:17 | ExceptStmt | 'except' clause does nothing but pass and there is no explanatory comment. |

python/ql/test/query-tests/Exceptions/general/EmptyExcept.expected

@@ -1,4 +1,4 @@
 | exceptions_test.py:51:5:51:25 | ExceptStmt | Non-exception $@ in exception handler which will never match raised exception. | exceptions_test.py:33:1:33:28 | ControlFlowNode for ClassExpr | class 'NotException1' |
 | exceptions_test.py:54:5:54:25 | ExceptStmt | Non-exception $@ in exception handler which will never match raised exception. | exceptions_test.py:36:1:36:28 | ControlFlowNode for ClassExpr | class 'NotException2' |
-| exceptions_test.py:112:5:112:22 | ExceptStmt | Non-exception $@ in exception handler which will never match raised exception. | exceptions_test.py:107:12:107:14 | ControlFlowNode for FloatLiteral | instance of 'float' |
+| exceptions_test.py:138:5:138:22 | ExceptStmt | Non-exception $@ in exception handler which will never match raised exception. | exceptions_test.py:133:12:133:14 | ControlFlowNode for FloatLiteral | instance of 'float' |
 | pypy_test.py:14:5:14:14 | ExceptStmt | Non-exception $@ in exception handler which will never match raised exception. | pypy_test.py:14:12:14:13 | ControlFlowNode for IntegerLiteral | instance of 'int' |

python/ql/test/query-tests/Exceptions/general/IllegalExceptionHandlerType.expected

@@ -1 +1 @@
-| exceptions_test.py:64:1:64:22 | ExceptStmt | Except block for $@ is unreachable; the more general $@ for $@ will always be executed in preference. | file://:0:0:0:0 | builtin-class AttributeError | AttributeError | exceptions_test.py:62:1:62:17 | ExceptStmt | except block | file://:0:0:0:0 | builtin-class Exception | Exception |
+| exceptions_test.py:64:1:64:22 | ExceptStmt | This except block handling $@ is unreachable; as $@ for the more general $@ always subsumes it. | file://:0:0:0:0 | AttributeError | AttributeError | exceptions_test.py:62:1:62:17 | ExceptStmt | this except block | file://:0:0:0:0 | Exception | Exception |

python/ql/test/query-tests/Exceptions/general/IncorrectExceptOrder.expected

@@ -1 +1,2 @@
-Exceptions/IncorrectExceptOrder.ql
+query: Exceptions/IncorrectExceptOrder.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

python/ql/test/query-tests/Exceptions/general/IncorrectExceptOrder.qlref

@@ -1,2 +1,2 @@
-| exceptions_test.py:170:11:170:24 | NotImplemented | NotImplemented is not an Exception. Did you mean NotImplementedError? |
-| exceptions_test.py:173:11:173:26 | NotImplemented() | NotImplemented is not an Exception. Did you mean NotImplementedError? |
+| exceptions_test.py:196:11:196:24 | NotImplemented | NotImplemented is not an Exception. Did you mean NotImplementedError? |
+| exceptions_test.py:199:11:199:26 | NotImplemented() | NotImplemented is not an Exception. Did you mean NotImplementedError? |