diff --git a/change-notes/2020-05-18-redundant-recover.md b/change-notes/2020-05-18-redundant-recover.md new file mode 100644 index 00000000000..cca5e8fe490 --- /dev/null +++ b/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. diff --git a/ql/src/RedundantCode/RedundantRecover.qhelp b/ql/src/RedundantCode/RedundantRecover.qhelp new file mode 100644 index 00000000000..7470b63c898 --- /dev/null +++ b/ql/src/RedundantCode/RedundantRecover.qhelp @@ -0,0 +1,54 @@ + + + + +

+The built-in recover function is only useful inside deferred +functions. Calling it in a function that is never deferred means that it will +always return nil and it will never regain control of a panicking +goroutine. The same is true of calling recover directly in a defer +statement. +

+
+ + +

+Carefully inspect the code to determine whether it is a mistake that should be +fixed. +

+
+ + +

+In the example below, the function fun1 is intended to recover +from the panic. However, the function that is deferred calls another function, +which then calls recover: +

+ +

+This problem can be fixed by deferring the call to the function which calls +recover: +

+ + +

+In the following example, recover is called directly in a defer +statement, which has no effect, so the panic is not caught. +

+ +

+We can fix this by instead deferring an anonymous function which calls +recover. +

+ +
+ + +
  • + Defer, Panic, and Recover - The Go Blog. +
  • +
    + +
    diff --git a/ql/src/RedundantCode/RedundantRecover.ql b/ql/src/RedundantCode/RedundantRecover.ql new file mode 100644 index 00000000000..7e509bc4862 --- /dev/null +++ b/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" diff --git a/ql/src/RedundantCode/RedundantRecover1.go b/ql/src/RedundantCode/RedundantRecover1.go new file mode 100644 index 00000000000..d058dd0dfde --- /dev/null +++ b/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") +} diff --git a/ql/src/RedundantCode/RedundantRecover1Good.go b/ql/src/RedundantCode/RedundantRecover1Good.go new file mode 100644 index 00000000000..b017e050dc4 --- /dev/null +++ b/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") +} diff --git a/ql/src/RedundantCode/RedundantRecover2.go b/ql/src/RedundantCode/RedundantRecover2.go new file mode 100644 index 00000000000..4365cb7c9fe --- /dev/null +++ b/ql/src/RedundantCode/RedundantRecover2.go @@ -0,0 +1,6 @@ +package main + +func fun2() { + defer recover() + panic("2") +} diff --git a/ql/src/RedundantCode/RedundantRecover2Good.go b/ql/src/RedundantCode/RedundantRecover2Good.go new file mode 100644 index 00000000000..d34e5c82b63 --- /dev/null +++ b/ql/src/RedundantCode/RedundantRecover2Good.go @@ -0,0 +1,6 @@ +package main + +func fun2Good() { + defer func() { recover() }() + panic("2") +} diff --git a/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.expected b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.expected new file mode 100644 index 00000000000..fdc175c8cf0 --- /dev/null +++ b/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 | diff --git a/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.qlref b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.qlref new file mode 100644 index 00000000000..c8997068734 --- /dev/null +++ b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.qlref @@ -0,0 +1 @@ +RedundantCode/RedundantRecover.ql diff --git a/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover1.go b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover1.go new file mode 100644 index 00000000000..d058dd0dfde --- /dev/null +++ b/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") +} diff --git a/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover1Good.go b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover1Good.go new file mode 100644 index 00000000000..b017e050dc4 --- /dev/null +++ b/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") +} diff --git a/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2.go b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2.go new file mode 100644 index 00000000000..4365cb7c9fe --- /dev/null +++ b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2.go @@ -0,0 +1,6 @@ +package main + +func fun2() { + defer recover() + panic("2") +} diff --git a/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2Good.go b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2Good.go new file mode 100644 index 00000000000..d34e5c82b63 --- /dev/null +++ b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2Good.go @@ -0,0 +1,6 @@ +package main + +func fun2Good() { + defer func() { recover() }() + panic("2") +} diff --git a/ql/test/query-tests/RedundantCode/RedundantRecover/tst.go b/ql/test/query-tests/RedundantCode/RedundantRecover/tst.go new file mode 100644 index 00000000000..0533a060931 --- /dev/null +++ b/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() +}