diff --git a/ql/src/semmle/go/Scopes.qll b/ql/src/semmle/go/Scopes.qll index 4df20739839..0b2b6ca01ae 100644 --- 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() } diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraph.qll b/ql/src/semmle/go/controlflow/ControlFlowGraph.qll index c0714c8fc0f..abf5eab4682 100644 --- 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; diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll index b3cffde6a2c..d3a0e98533f 100644 --- 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) } diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll index f14b0fbb45e..0ca103156ba 100644 --- 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. */ diff --git a/ql/src/semmle/go/frameworks/Testing.qll b/ql/src/semmle/go/frameworks/Testing.qll index e2940dd14d7..eedd62ecffd 100644 --- 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() } + } +} diff --git a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected index 0bc673ab1e6..a1bc1d77929 100644 --- 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 | diff --git a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/NoretFunctions.expected b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/NoretFunctions.expected new file mode 100644 index 00000000000..2fcfd4dc811 --- /dev/null +++ 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 | diff --git a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/NoretFunctions.ql b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/NoretFunctions.ql new file mode 100644 index 00000000000..b61493abb9f --- /dev/null +++ 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() diff --git a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/noretfunctions.go b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/noretfunctions.go new file mode 100644 index 00000000000..85282a09089 --- /dev/null +++ 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") +}