Merge pull request #147 from owen-mc/redundant-recover

Go: Add query for redundant calls to recover

change-notes/2020-05-18-redundant-recover.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* A new query "Redundant call to recover" (`go/redundant-recover`) has been added. The query detects calls to `recover` that have no effect.

ql/src/RedundantCode/RedundantRecover.qhelp

@@ -0,0 +1,54 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+The built-in <code>recover</code> function is only useful inside deferred
+functions. Calling it in a function that is never deferred means that it will
+always return <code>nil</code> and it will never regain control of a panicking
+goroutine. The same is true of calling <code>recover</code> directly in a defer
+statement.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Carefully inspect the code to determine whether it is a mistake that should be
+fixed.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the example below, the function <code>fun1</code> is intended to recover
+from the panic. However, the function that is deferred calls another function,
+which then calls <code>recover</code>:
+</p>
+<sample src="RedundantRecover1.go" />
+<p>
+This problem can be fixed by deferring the call to the function which calls
+<code>recover</code>:
+</p>
+<sample src="RedundantRecover1Good.go" />
+
+<p>
+In the following example, <code>recover</code> is called directly in a defer
+statement, which has no effect, so the panic is not caught.
+</p>
+<sample src="RedundantRecover2.go" />
+<p>
+We can fix this by instead deferring an anonymous function which calls
+<code>recover</code>.
+</p>
+<sample src="RedundantRecover2Good.go" />
+</example>
+
+<references>
+<li>
+  <a href="https://blog.golang.org/defer-panic-and-recover">Defer, Panic, and Recover - The Go Blog</a>.
+</li>
+</references>
+
+</qhelp>

ql/src/RedundantCode/RedundantRecover.ql

@@ -0,0 +1,33 @@
+/**
+ * @name Redundant call to recover
+ * @description Calling 'recover' in a function which isn't called using a defer
+ *              statement has no effect. Also, putting 'recover' directly in a
+ *              defer statement has no effect.
+ * @kind problem
+ * @problem.severity warning
+ * @id go/redundant-recover
+ * @tags maintainability
+ *       correctness
+ * @precision high
+ */
+
+import go
+
+predicate isDeferred(DataFlow::CallNode call) {
+  exists(DeferStmt defer | defer.getCall() = call.asExpr())
+}
+
+from DataFlow::CallNode recoverCall, FuncDef f, string msg
+where
+  recoverCall.getTarget() = Builtin::recover() and
+  f = recoverCall.getEnclosingCallable() and
+  (
+    isDeferred(recoverCall) and
+    msg = "Deferred calls to 'recover' have no effect."
+    or
+    not isDeferred(recoverCall) and
+    exists(f.getACall()) and
+    not isDeferred(f.getACall()) and
+    msg = "This call to 'recover' has no effect because $@ is never called using a defer statement."
+  )
+select recoverCall, msg, f, "the enclosing function"

ql/src/RedundantCode/RedundantRecover1.go

@@ -0,0 +1,16 @@
+package main
+
+import "fmt"
+
+func callRecover1() {
+	if recover() != nil {
+		fmt.Printf("recovered")
+	}
+}
+
+func fun1() {
+	defer func() {
+		callRecover1()
+	}()
+	panic("1")
+}

ql/src/RedundantCode/RedundantRecover1Good.go

@@ -0,0 +1,14 @@
+package main
+
+import "fmt"
+
+func callRecover1Good() {
+	if recover() != nil {
+		fmt.Printf("recovered")
+	}
+}
+
+func fun1Good() {
+	defer callRecover1Good()
+	panic("1")
+}

ql/src/RedundantCode/RedundantRecover2.go

@@ -0,0 +1,6 @@
+package main
+
+func fun2() {
+	defer recover()
+	panic("2")
+}

ql/src/RedundantCode/RedundantRecover2Good.go

@@ -0,0 +1,6 @@
+package main
+
+func fun2Good() {
+	defer func() { recover() }()
+	panic("2")
+}

ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.expected

@@ -0,0 +1,3 @@
+| RedundantRecover1.go:6:5:6:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | RedundantRecover1.go:5:1:9:1 | function declaration | the enclosing function |
+| RedundantRecover2.go:4:8:4:16 | call to recover | Deferred calls to 'recover' have no effect. | RedundantRecover2.go:3:1:6:1 | function declaration | the enclosing function |
+| tst.go:8:5:8:13 | call to recover | This call to 'recover' has no effect because $@ is never called using a defer statement. | tst.go:5:1:11:1 | function declaration | the enclosing function |

ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.qlref

@@ -0,0 +1 @@
+RedundantCode/RedundantRecover.ql

ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover1.go

@@ -0,0 +1,16 @@
+package main
+
+import "fmt"
+
+func callRecover1() {
+	if recover() != nil {
+		fmt.Printf("recovered")
+	}
+}
+
+func fun1() {
+	defer func() {
+		callRecover1()
+	}()
+	panic("1")
+}

ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover1Good.go

@@ -0,0 +1,14 @@
+package main
+
+import "fmt"
+
+func callRecover1Good() {
+	if recover() != nil {
+		fmt.Printf("recovered")
+	}
+}
+
+func fun1Good() {
+	defer callRecover1Good()
+	panic("1")
+}

ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2.go

@@ -0,0 +1,6 @@
+package main
+
+func fun2() {
+	defer recover()
+	panic("2")
+}

ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2Good.go

@@ -0,0 +1,6 @@
+package main
+
+func fun2Good() {
+	defer func() { recover() }()
+	panic("2")
+}

ql/test/query-tests/RedundantCode/RedundantRecover/tst.go

@@ -0,0 +1,49 @@
+package main
+
+import "fmt"
+
+func callRecover3() {
+	// This will have no effect because panics do not propagate down the stack,
+	// only back up the stack
+	if recover() != nil {
+		fmt.Printf("recovered")
+	}
+}
+
+func fun3() {
+	panic("3")
+	callRecover3()
+}
+
+func callRecover4() {
+	// This is not flagged because callRecover4 is called in a defer statement
+	// at least once
+	if recover() != nil {
+		fmt.Printf("recovered")
+	}
+}
+
+func fun4a() {
+	panic("4")
+	callRecover4()
+}
+
+func fun4b() {
+	defer callRecover4()
+	panic("4")
+}
+
+func neverCalled() {
+	// This will not be flagged because it is not called from anywhere
+	if recover() != nil {
+		fmt.Printf("recovered")
+	}
+}
+
+func main() {
+	fun1()
+	fun2()
+	fun3()
+	fun4a()
+	fun4b()
+}