From fbee7fe9836dfbe4e3caaf80a59bff2dc27c5500 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 13 May 2020 09:43:50 +0100 Subject: [PATCH 1/3] Add new query for redundant calls to recover --- change-notes/2020-05-18-redundant-recover.md | 2 + ql/src/RedundantCode/RedundantRecover.qhelp | 53 +++++++++++++++++++ ql/src/RedundantCode/RedundantRecover.ql | 33 ++++++++++++ ql/src/RedundantCode/RedundantRecover1.go | 16 ++++++ ql/src/RedundantCode/RedundantRecover1Good.go | 14 +++++ ql/src/RedundantCode/RedundantRecover2.go | 6 +++ ql/src/RedundantCode/RedundantRecover2Good.go | 6 +++ .../RedundantRecover.expected | 3 ++ .../RedundantRecover/RedundantRecover.qlref | 1 + .../RedundantRecover/RedundantRecover1.go | 16 ++++++ .../RedundantRecover/RedundantRecover1Good.go | 14 +++++ .../RedundantRecover/RedundantRecover2.go | 6 +++ .../RedundantRecover/RedundantRecover2Good.go | 6 +++ .../RedundantCode/RedundantRecover/tst.go | 49 +++++++++++++++++ 14 files changed, 225 insertions(+) create mode 100644 change-notes/2020-05-18-redundant-recover.md create mode 100644 ql/src/RedundantCode/RedundantRecover.qhelp create mode 100644 ql/src/RedundantCode/RedundantRecover.ql create mode 100644 ql/src/RedundantCode/RedundantRecover1.go create mode 100644 ql/src/RedundantCode/RedundantRecover1Good.go create mode 100644 ql/src/RedundantCode/RedundantRecover2.go create mode 100644 ql/src/RedundantCode/RedundantRecover2Good.go create mode 100644 ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.expected create mode 100644 ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.qlref create mode 100644 ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover1.go create mode 100644 ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover1Good.go create mode 100644 ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2.go create mode 100644 ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover2Good.go create mode 100644 ql/test/query-tests/RedundantCode/RedundantRecover/tst.go 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..5b8151be60b --- /dev/null +++ b/change-notes/2020-05-18-redundant-recover.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* A new query go/redundant-recover has been added to detect redundant calls to recover. diff --git a/ql/src/RedundantCode/RedundantRecover.qhelp b/ql/src/RedundantCode/RedundantRecover.qhelp new file mode 100644 index 00000000000..0033b640df2 --- /dev/null +++ b/ql/src/RedundantCode/RedundantRecover.qhelp @@ -0,0 +1,53 @@ + + + + +

+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 deferring the call to the function which callsrecover: +

+ + +

+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..58ab5fb1149 --- /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, f.getName() 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..88545b5a503 --- /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 | callRecover1 | +| RedundantRecover2.go:4:8:4:16 | call to recover | Deferred calls to 'recover' have no effect | RedundantRecover2.go:3:1:6:1 | function declaration | fun2 | +| 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 | callRecover3 | 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() +} From 23a7db5d4d22294a15ab998cf7bbe2f0e8f46858 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 18 May 2020 17:05:49 +0100 Subject: [PATCH 2/3] Minor textual corrections --- ql/src/RedundantCode/RedundantRecover.qhelp | 3 ++- ql/src/RedundantCode/RedundantRecover.ql | 4 ++-- .../RedundantRecover/RedundantRecover.expected | 6 +++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/ql/src/RedundantCode/RedundantRecover.qhelp b/ql/src/RedundantCode/RedundantRecover.qhelp index 0033b640df2..7470b63c898 100644 --- a/ql/src/RedundantCode/RedundantRecover.qhelp +++ b/ql/src/RedundantCode/RedundantRecover.qhelp @@ -28,7 +28,8 @@ which then calls recover:

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

    diff --git a/ql/src/RedundantCode/RedundantRecover.ql b/ql/src/RedundantCode/RedundantRecover.ql index 58ab5fb1149..7e509bc4862 100644 --- a/ql/src/RedundantCode/RedundantRecover.ql +++ b/ql/src/RedundantCode/RedundantRecover.ql @@ -23,11 +23,11 @@ where f = recoverCall.getEnclosingCallable() and ( isDeferred(recoverCall) and - msg = "Deferred calls to 'recover' have no effect" + 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, f.getName() +select recoverCall, msg, f, "the enclosing function" diff --git a/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.expected b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.expected index 88545b5a503..fdc175c8cf0 100644 --- a/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.expected +++ b/ql/test/query-tests/RedundantCode/RedundantRecover/RedundantRecover.expected @@ -1,3 +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 | callRecover1 | -| RedundantRecover2.go:4:8:4:16 | call to recover | Deferred calls to 'recover' have no effect | RedundantRecover2.go:3:1:6:1 | function declaration | fun2 | -| 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 | callRecover3 | +| 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 | From 275be36e4aa7c0697d0bc1b9a5e2fdf639bdb915 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Tue, 19 May 2020 06:31:47 +0100 Subject: [PATCH 3/3] Update change-notes/2020-05-18-redundant-recover.md Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- change-notes/2020-05-18-redundant-recover.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/2020-05-18-redundant-recover.md b/change-notes/2020-05-18-redundant-recover.md index 5b8151be60b..cca5e8fe490 100644 --- a/change-notes/2020-05-18-redundant-recover.md +++ b/change-notes/2020-05-18-redundant-recover.md @@ -1,2 +1,2 @@ lgtm,codescanning -* A new query go/redundant-recover has been added to detect redundant calls to recover. +* A new query "Redundant call to recover" (`go/redundant-recover`) has been added. The query detects calls to `recover` that have no effect.