Merge pull request #239 from smowton/smowton/feature/find-noreturn-user-functions

Switch from using mustPanic to mayReturnNormally to construct a call-expression's CFG
2026-01-29 14:23:03 +01:00 · 2020-07-07 13:37:18 +01:00
parent d8ff2d1641 6e5ee47ade
commit 47a858610d
9 changed files with 143 additions and 17 deletions
--- a/ql/src/semmle/go/Scopes.qll
+++ b/ql/src/semmle/go/Scopes.qll
@@ -347,9 +347,26 @@ class Function extends ValueEntity, @functionobject {
    this.(DeclaredFunction).getFuncDecl() = result.getACallee()
  }

+  /** Gets the declaration of this function, if any. */
+  FuncDecl getFuncDecl() { none() }
+
  /** Holds if this function has no observable side effects. */
  predicate mayHaveSideEffects() { none() }

+  /**
+   * Holds if this function may return without panicking, exiting the process, or looping forever.
+   *
+   * This predicate is an over-approximation: it may hold for functions that can never
+   * return normally, but it never fails to hold for functions that can.
+   *
+   * Note this is declared here and not in `DeclaredFunction` so that library models can override this
+   * by extending `Function` rather than having to remember to extend `DeclaredFunction`.
+   */
+  predicate mayReturnNormally() {
+    not mustPanic() and
+    (ControlFlow::mayReturnNormally(getFuncDecl()) or not exists(getBody()))
+  }
+
  /**
   * Holds if calling this function may cause a runtime panic.
   *
@@ -493,8 +510,7 @@ class Method extends Function {

 /** A declared function. */
 class DeclaredFunction extends Function, DeclaredEntity, @declfunctionobject {
-  /** Gets the declaration of this function. */
-  FuncDecl getFuncDecl() { result.getNameExpr() = this.getDeclaration() }
+  override FuncDecl getFuncDecl() { result.getNameExpr() = this.getDeclaration() }

  override BlockStmt getBody() { result = getFuncDecl().getBody() }

--- a/ql/src/semmle/go/controlflow/ControlFlowGraph.qll
+++ b/ql/src/semmle/go/controlflow/ControlFlowGraph.qll
@@ -259,6 +259,14 @@ module ControlFlow {
   * Gets the exit node of function or file `root`.
   */
  Node exitNode(Root root) { result = MkExitNode(root) }
+
+  /**
+   * Holds if the function `f` may return without panicking, exiting the process, or looping forever.
+   *
+   * This is defined conservatively, and so may also hold of a function that in fact
+   * cannot return normally, but never fails to hold of a function that can return normally.
+   */
+  predicate mayReturnNormally(FuncDecl f) { CFG::mayReturnNormally(f.getBody()) }
 }

 class Write = ControlFlow::WriteNode;
--- a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll
+++ b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll
@@ -918,7 +918,7 @@ module CFG {
    }

    override Completion getCompletion() {
-      not getTarget().mustPanic() and
+      (not exists(getTarget()) or getTarget().mayReturnNormally()) and
      result = Done()
      or
      (not exists(getTarget()) or getTarget().mayPanic()) and
@@ -1924,6 +1924,17 @@ module CFG {
    )
  }

+  /**
+   * Holds if the function `f` may return without panicking, exiting the process, or looping forever.
+   *
+   * This is defined conservatively, and so may also hold of a function that in fact
+   * cannot return normally, but never fails to hold of a function that can return normally.
+   */
+  cached
+  predicate mayReturnNormally(ControlFlowTree root) {
+    exists(ControlFlow::Node last, Completion cmpl | lastNode(root, last, cmpl) and cmpl != Panic())
+  }
+
  /** Gets a successor of `nd`, that is, a node that is executed after `nd`. */
  cached
  ControlFlow::Node succ(ControlFlow::Node nd) { any(ControlFlowTree tree).succ(nd, result) }
--- a/ql/src/semmle/go/frameworks/Stdlib.qll
+++ b/ql/src/semmle/go/frameworks/Stdlib.qll
@@ -453,6 +453,13 @@ module OS {
      inp.isParameter(0) and outp.isResult()
    }
  }
+
+  /** The `os.Exit` function, which ends the process. */
+  private class Exit extends Function {
+    Exit() { hasQualifiedName("os", "Exit") }
+
+    override predicate mayReturnNormally() { none() }
+  }
 }

 /** Provides models of commonly used functions in the `path` package. */
@@ -786,6 +793,13 @@ module Log {

    override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
  }
+
+  /** A fatal log function, which calls `os.Exit`. */
+  private class FatalLogFunction extends Function {
+    FatalLogFunction() { exists(string fn | fn.matches("Fatal%") | hasQualifiedName("log", fn)) }
+
+    override predicate mayReturnNormally() { none() }
+  }
 }

 /** Provides models of some functions in the `encoding/json` package. */
--- a/ql/src/semmle/go/frameworks/Testing.qll
+++ b/ql/src/semmle/go/frameworks/Testing.qll
@@ -83,3 +83,13 @@ module TestFile {
    }
  }
 }
+
+/** Provides classes modelling Ginkgo. */
+module Ginkgo {
+  /** The Ginkgo `Fail` function, which always panics. */
+  private class FailFunction extends Function {
+    FailFunction() { hasQualifiedName("github.com/onsi/ginkgo", "Fail") }
+
+    override predicate mustPanic() { any() }
+  }
+}
--- a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected
+++ b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected
@@ -655,6 +655,45 @@
 | main.go:84:11:84:12 | 19 | main.go:84:9:84:12 | ...+... |
 | main.go:84:15:84:15 | x | main.go:84:2:84:2 | assignment to x |
 | main.go:85:2:85:7 | return statement | main.go:82:18:82:18 | implicit read of a |
+| noretfunctions.go:0:0:0:0 | entry | noretfunctions.go:3:1:6:1 | skip |
+| noretfunctions.go:3:1:6:1 | skip | noretfunctions.go:8:6:8:12 | skip |
+| noretfunctions.go:8:1:8:1 | entry | noretfunctions.go:9:2:9:8 | selection of Exit |
+| noretfunctions.go:8:1:10:1 | function declaration | noretfunctions.go:12:6:12:11 | skip |
+| noretfunctions.go:8:6:8:12 | skip | noretfunctions.go:8:1:10:1 | function declaration |
+| noretfunctions.go:9:2:9:8 | selection of Exit | noretfunctions.go:9:10:9:10 | 1 |
+| noretfunctions.go:9:2:9:11 | call to Exit | noretfunctions.go:10:1:10:1 | exit |
+| noretfunctions.go:9:10:9:10 | 1 | noretfunctions.go:9:2:9:11 | call to Exit |
+| noretfunctions.go:12:1:12:1 | entry | noretfunctions.go:12:13:12:13 | argument corresponding to x |
+| noretfunctions.go:12:1:16:1 | function declaration | noretfunctions.go:18:6:18:12 | skip |
+| noretfunctions.go:12:6:12:11 | skip | noretfunctions.go:12:1:16:1 | function declaration |
+| noretfunctions.go:12:13:12:13 | argument corresponding to x | noretfunctions.go:12:13:12:13 | initialization of x |
+| noretfunctions.go:12:13:12:13 | initialization of x | noretfunctions.go:13:5:13:5 | x |
+| noretfunctions.go:13:5:13:5 | x | noretfunctions.go:13:10:13:10 | 0 |
+| noretfunctions.go:13:5:13:10 | ...!=... | noretfunctions.go:13:10:13:10 | ...!=... is false |
+| noretfunctions.go:13:5:13:10 | ...!=... | noretfunctions.go:13:10:13:10 | ...!=... is true |
+| noretfunctions.go:13:5:13:10 | ...!=... | noretfunctions.go:16:1:16:1 | exit |
+| noretfunctions.go:13:10:13:10 | 0 | noretfunctions.go:13:5:13:10 | ...!=... |
+| noretfunctions.go:13:10:13:10 | ...!=... is false | noretfunctions.go:16:1:16:1 | exit |
+| noretfunctions.go:13:10:13:10 | ...!=... is true | noretfunctions.go:14:3:14:9 | selection of Exit |
+| noretfunctions.go:14:3:14:9 | selection of Exit | noretfunctions.go:14:11:14:11 | x |
+| noretfunctions.go:14:3:14:12 | call to Exit | noretfunctions.go:16:1:16:1 | exit |
+| noretfunctions.go:14:11:14:11 | x | noretfunctions.go:14:3:14:12 | call to Exit |
+| noretfunctions.go:18:1:18:1 | entry | noretfunctions.go:18:16:18:17 | skip |
+| noretfunctions.go:18:1:18:17 | function declaration | noretfunctions.go:20:6:20:22 | skip |
+| noretfunctions.go:18:6:18:12 | skip | noretfunctions.go:18:1:18:17 | function declaration |
+| noretfunctions.go:18:16:18:17 | skip | noretfunctions.go:18:17:18:17 | exit |
+| noretfunctions.go:20:1:20:1 | entry | noretfunctions.go:21:2:21:10 | selection of Fatal |
+| noretfunctions.go:20:1:22:1 | function declaration | noretfunctions.go:24:6:24:23 | skip |
+| noretfunctions.go:20:6:20:22 | skip | noretfunctions.go:20:1:22:1 | function declaration |
+| noretfunctions.go:21:2:21:10 | selection of Fatal | noretfunctions.go:21:12:21:18 | "Oh no" |
+| noretfunctions.go:21:2:21:19 | call to Fatal | noretfunctions.go:22:1:22:1 | exit |
+| noretfunctions.go:21:12:21:18 | "Oh no" | noretfunctions.go:21:2:21:19 | call to Fatal |
+| noretfunctions.go:24:1:24:1 | entry | noretfunctions.go:25:2:25:11 | selection of Fatalf |
+| noretfunctions.go:24:1:26:1 | function declaration | noretfunctions.go:0:0:0:0 | exit |
+| noretfunctions.go:24:6:24:23 | skip | noretfunctions.go:24:1:26:1 | function declaration |
+| noretfunctions.go:25:2:25:11 | selection of Fatalf | noretfunctions.go:25:13:25:30 | "It's as I feared" |
+| noretfunctions.go:25:2:25:31 | call to Fatalf | noretfunctions.go:26:1:26:1 | exit |
+| noretfunctions.go:25:13:25:30 | "It's as I feared" | noretfunctions.go:25:2:25:31 | call to Fatalf |
 | stmts2.go:0:0:0:0 | entry | stmts2.go:3:6:3:11 | skip |
 | stmts2.go:3:1:3:1 | entry | stmts2.go:4:2:4:2 | skip |
 | stmts2.go:3:1:7:1 | function declaration | stmts2.go:9:6:9:11 | skip |
@@ -1063,7 +1102,6 @@
 | stmts.go:77:17:77:22 | ...-... | stmts.go:79:3:79:7 | test5 |
 | stmts.go:77:21:77:22 | 19 | stmts.go:77:17:77:22 | ...-... |
 | stmts.go:79:3:79:7 | test5 | stmts.go:79:9:79:13 | false |
-| stmts.go:79:3:79:14 | call to test5 | stmts.go:82:9:82:9 | x |
 | stmts.go:79:3:79:14 | call to test5 | stmts.go:107:1:107:1 | exit |
 | stmts.go:79:9:79:13 | false | stmts.go:79:3:79:14 | call to test5 |
 | stmts.go:82:9:82:9 | x | stmts.go:83:7:83:7 | 1 |
@@ -1079,8 +1117,6 @@
 | stmts.go:84:10:84:10 | case 3 | stmts.go:85:3:85:7 | test5 |
 | stmts.go:84:10:84:10 | case 3 | stmts.go:88:9:88:9 | x |
 | stmts.go:85:3:85:7 | test5 | stmts.go:85:9:85:12 | true |
-| stmts.go:85:3:85:13 | call to test5 | stmts.go:88:9:88:9 | x |
-| stmts.go:85:3:85:13 | call to test5 | stmts.go:107:1:107:1 | exit |
 | stmts.go:85:9:85:12 | true | stmts.go:85:3:85:13 | call to test5 |
 | stmts.go:88:9:88:9 | x | stmts.go:89:7:89:7 | 1 |
 | stmts.go:88:9:88:9 | x | stmts.go:96:9:96:9 | x |
@@ -1088,16 +1124,12 @@
 | stmts.go:89:7:89:7 | case 1 | stmts.go:90:3:90:7 | test5 |
 | stmts.go:89:7:89:7 | case 1 | stmts.go:92:7:92:11 | ...-... |
 | stmts.go:90:3:90:7 | test5 | stmts.go:90:9:90:13 | false |
-| stmts.go:90:3:90:14 | call to test5 | stmts.go:91:3:91:13 | skip |
-| stmts.go:90:3:90:14 | call to test5 | stmts.go:107:1:107:1 | exit |
 | stmts.go:90:9:90:13 | false | stmts.go:90:3:90:14 | call to test5 |
 | stmts.go:91:3:91:13 | skip | stmts.go:93:3:93:7 | test5 |
 | stmts.go:92:7:92:11 | ...-... | stmts.go:92:7:92:11 | case ...-... |
 | stmts.go:92:7:92:11 | case ...-... | stmts.go:93:3:93:7 | test5 |
 | stmts.go:92:7:92:11 | case ...-... | stmts.go:96:9:96:9 | x |
 | stmts.go:93:3:93:7 | test5 | stmts.go:93:9:93:12 | true |
-| stmts.go:93:3:93:13 | call to test5 | stmts.go:96:9:96:9 | x |
-| stmts.go:93:3:93:13 | call to test5 | stmts.go:107:1:107:1 | exit |
 | stmts.go:93:9:93:12 | true | stmts.go:93:3:93:13 | call to test5 |
 | stmts.go:96:9:96:9 | x | stmts.go:98:7:98:7 | 2 |
 | stmts.go:97:2:97:9 | skip | stmts.go:102:2:102:2 | true |
@@ -1105,12 +1137,8 @@
 | stmts.go:98:7:98:7 | case 2 | stmts.go:97:2:97:9 | skip |
 | stmts.go:98:7:98:7 | case 2 | stmts.go:99:3:99:7 | test5 |
 | stmts.go:99:3:99:7 | test5 | stmts.go:99:9:99:12 | true |
-| stmts.go:99:3:99:13 | call to test5 | stmts.go:102:2:102:2 | true |
-| stmts.go:99:3:99:13 | call to test5 | stmts.go:107:1:107:1 | exit |
 | stmts.go:99:9:99:12 | true | stmts.go:99:3:99:13 | call to test5 |
 | stmts.go:102:2:102:2 | true | stmts.go:105:7:105:10 | true |
-| stmts.go:104:3:104:7 | skip | stmts.go:107:1:107:1 | exit |
-| stmts.go:105:2:105:11 | skip | stmts.go:107:1:107:1 | exit |
 | stmts.go:105:7:105:10 | case true | stmts.go:105:10:105:10 | true is false |
 | stmts.go:105:7:105:10 | case true | stmts.go:105:10:105:10 | true is true |
 | stmts.go:105:7:105:10 | true | stmts.go:105:7:105:10 | case true |
@@ -1137,12 +1165,9 @@
 | stmts.go:114:7:114:13 | case float32 | stmts.go:115:3:115:7 | test5 |
 | stmts.go:114:7:114:13 | case float32 | stmts.go:119:9:119:9 | skip |
 | stmts.go:115:3:115:7 | test5 | stmts.go:115:9:115:12 | true |
-| stmts.go:115:3:115:13 | call to test5 | stmts.go:116:3:116:7 | test5 |
 | stmts.go:115:3:115:13 | call to test5 | stmts.go:123:1:123:1 | exit |
 | stmts.go:115:9:115:12 | true | stmts.go:115:3:115:13 | call to test5 |
 | stmts.go:116:3:116:7 | test5 | stmts.go:116:9:116:13 | false |
-| stmts.go:116:3:116:14 | call to test5 | stmts.go:119:9:119:9 | skip |
-| stmts.go:116:3:116:14 | call to test5 | stmts.go:123:1:123:1 | exit |
 | stmts.go:116:9:116:13 | false | stmts.go:116:3:116:14 | call to test5 |
 | stmts.go:119:9:119:9 | assignment to y | stmts.go:119:17:119:17 | y |
 | stmts.go:119:9:119:9 | skip | stmts.go:119:14:119:14 | x |
--- a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/NoretFunctions.expected
+++ b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/NoretFunctions.expected
@@ -0,0 +1,11 @@
+| file://:0:0:0:0 | Exit | package os |
+| file://:0:0:0:0 | Fatal | package log |
+| file://:0:0:0:0 | Fatalf | package log |
+| file://:0:0:0:0 | Fatalln | package log |
+| noretfunctions.go:8:6:8:12 | isNoRet | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph |
+| noretfunctions.go:20:6:20:22 | noRetUsesLogFatal | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph |
+| noretfunctions.go:24:6:24:23 | noRetUsesLogFatalf | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph |
+| stmts7.go:10:6:10:15 | canRecover | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph |
+| stmts.go:8:6:8:10 | test5 | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph |
+| stmts.go:44:6:44:10 | test6 | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph |
+| stmts.go:110:6:110:10 | test9 | package github.com/github/codeql-go/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph |
--- a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/NoretFunctions.ql
+++ b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/NoretFunctions.ql
@@ -0,0 +1,5 @@
+import go
+
+from Function f
+where not f.mayReturnNormally()
+select f, f.getPackage()
--- a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/noretfunctions.go
+++ b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/noretfunctions.go
@@ -0,0 +1,26 @@
+package main
+
+import (
+	"log"
+	"os"
+)
+
+func isNoRet() {
+	os.Exit(1)
+}
+
+func mayRet(x int) {
+	if x != 0 {
+		os.Exit(x)
+	}
+}
+
+func doesRet() {}
+
+func noRetUsesLogFatal() {
+	log.Fatal("Oh no")
+}
+
+func noRetUsesLogFatalf() {
+	log.Fatalf("It's as I feared")
+}