From 1dc427a2c5b14cc8b59d50003af97d49ba7b117c Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Mon, 22 Jun 2020 14:33:38 +0100 Subject: [PATCH 1/2] Cleanup: use TypeSwitchStmt.getAssign, not a raw child accessor --- ql/src/semmle/go/Stmt.qll | 3 +++ ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/ql/src/semmle/go/Stmt.qll b/ql/src/semmle/go/Stmt.qll index e47b0fedf1c..0bfbbea1eef 100644 --- a/ql/src/semmle/go/Stmt.qll +++ b/ql/src/semmle/go/Stmt.qll @@ -882,6 +882,9 @@ class TypeSwitchStmt extends @typeswitchstmt, SwitchStmt { /** Gets the assign statement of this type-switch statement. */ SimpleAssignStmt getAssign() { result = getChildStmt(1) } + /** Gets the test statement of this type-switch statement. This is a `SimpleAssignStmt` or `ExprStmt`. */ + Stmt getTest() { result = getChildStmt(1) } + /** Gets the expression whose type is examined by this `switch` statement. */ Expr getExpr() { result = getAssign().getRhs() or result = getChildStmt(1).(ExprStmt).getExpr() } diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll index 2013967c940..82d62210a1d 100644 --- a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll +++ b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll @@ -1761,7 +1761,7 @@ module CFG { or first = MkImplicitTrue(this) or - firstNode(this.(TypeSwitchStmt).getChildStmt(1), first) + firstNode(this.(TypeSwitchStmt).getTest(), first) ) } @@ -1775,7 +1775,7 @@ module CFG { last = MkImplicitTrue(this) and cmpl = Bool(true) or - lastNode(this.(TypeSwitchStmt).getChildStmt(1), last, cmpl) + lastNode(this.(TypeSwitchStmt).getTest(), last, cmpl) ) and ( not cmpl.isNormal() @@ -1811,13 +1811,13 @@ module CFG { ( firstNode(this.(ExpressionSwitchStmt).getExpr(), succ) or succ = MkImplicitTrue(this) or - firstNode(this.(TypeSwitchStmt).getChildStmt(1), succ) + firstNode(this.(TypeSwitchStmt).getTest(), succ) ) or ( lastNode(this.(ExpressionSwitchStmt).getExpr(), pred, normalCompletion()) or pred = MkImplicitTrue(this) or - lastNode(this.(TypeSwitchStmt).getChildStmt(1), pred, normalCompletion()) + lastNode(this.(TypeSwitchStmt).getTest(), pred, normalCompletion()) ) and ( firstNode(getNonDefaultCase(0), succ) From 4882f277f5b5d5b83b4a497f6da45fb7dae08d77 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 23 Jun 2020 15:30:11 +0100 Subject: [PATCH 2/2] Remove spurious control-flow edge around switch block without a test-expression Previously we thought it possible to get from top to bottom of a block like "switch { case f(): ... }", when in fact this is only possible if there are no case blocks to execute. I also add tests for two possible corner cases of a switch without a test-expression: a completely empty switch (the 'true' is indeed the last node) and switch with an empty default block (a single 'skip' is generated for the default block and the 'true' is not the last node) --- .../semmle/go/controlflow/ControlFlowGraphImpl.qll | 7 ++++--- .../ControlFlowNode_getASuccessor.expected | 13 ++++++++++--- .../semmle/go/controlflow/ControlFlowGraph/tst.go | 10 ++++++++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll index 82d62210a1d..b3cffde6a2c 100644 --- a/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll +++ b/ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll @@ -1772,9 +1772,6 @@ module CFG { ( lastNode(this.(ExpressionSwitchStmt).getExpr(), last, cmpl) or - last = MkImplicitTrue(this) and - cmpl = Bool(true) - or lastNode(this.(TypeSwitchStmt).getTest(), last, cmpl) ) and ( @@ -1783,6 +1780,10 @@ module CFG { not exists(this.getDefault()) ) or + last = MkImplicitTrue(this) and + cmpl = Bool(true) and + this.getNumCase() = 0 + or exists(CaseClause cc, int i, Completion inner | cc = this.getCase(i) and lastNode(cc, last, inner) | 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 1b1d4165a6b..0bc673ab1e6 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 @@ -1206,7 +1206,6 @@ | tst.go:3:12:3:12 | argument corresponding to x | tst.go:3:12:3:12 | initialization of x | | tst.go:3:12:3:12 | initialization of x | tst.go:4:2:4:2 | true | | tst.go:4:2:4:2 | true | tst.go:5:7:5:7 | x | -| tst.go:4:2:4:2 | true | tst.go:12:1:12:1 | exit | | tst.go:5:2:5:13 | skip | tst.go:12:1:12:1 | exit | | tst.go:5:7:5:7 | x | tst.go:5:11:5:12 | 23 | | tst.go:5:7:5:12 | ...<... | tst.go:5:7:5:12 | case ...<... | @@ -1232,12 +1231,11 @@ | tst.go:9:12:9:12 | ...<... is false | tst.go:12:1:12:1 | exit | | tst.go:9:12:9:12 | ...<... is true | tst.go:9:2:9:13 | skip | | tst.go:14:1:14:1 | entry | tst.go:14:13:14:17 | argument corresponding to value | -| tst.go:14:1:21:1 | function declaration | tst.go:0:0:0:0 | exit | +| tst.go:14:1:21:1 | function declaration | tst.go:23:6:23:11 | skip | | tst.go:14:6:14:11 | skip | tst.go:14:1:21:1 | function declaration | | tst.go:14:13:14:17 | argument corresponding to value | tst.go:14:13:14:17 | initialization of value | | tst.go:14:13:14:17 | initialization of value | tst.go:15:2:15:2 | true | | tst.go:15:2:15:2 | true | tst.go:16:7:16:11 | value | -| tst.go:15:2:15:2 | true | tst.go:21:1:21:1 | exit | | tst.go:16:2:16:34 | skip | tst.go:21:1:21:1 | exit | | tst.go:16:7:16:11 | value | tst.go:16:15:16:33 | ...*... | | tst.go:16:7:16:33 | ...<... | tst.go:16:7:16:33 | case ...<... | @@ -1254,3 +1252,12 @@ | tst.go:18:15:18:38 | ...*... | tst.go:18:7:18:38 | ...<... | | tst.go:18:38:18:38 | ...<... is false | tst.go:21:1:21:1 | exit | | tst.go:18:38:18:38 | ...<... is true | tst.go:18:2:18:39 | skip | +| tst.go:23:1:23:1 | entry | tst.go:24:2:24:2 | true | +| tst.go:23:1:25:1 | function declaration | tst.go:27:6:27:11 | skip | +| tst.go:23:6:23:11 | skip | tst.go:23:1:25:1 | function declaration | +| tst.go:24:2:24:2 | true | tst.go:25:1:25:1 | exit | +| tst.go:27:1:27:1 | entry | tst.go:28:2:28:2 | true | +| tst.go:27:1:31:1 | function declaration | tst.go:0:0:0:0 | exit | +| tst.go:27:6:27:11 | skip | tst.go:27:1:31:1 | function declaration | +| tst.go:28:2:28:2 | true | tst.go:29:2:29:9 | skip | +| tst.go:29:2:29:9 | skip | tst.go:31:1:31:1 | exit | diff --git a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/tst.go b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/tst.go index 8706681d796..ccba142881b 100644 --- a/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/tst.go +++ b/ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/tst.go @@ -19,3 +19,13 @@ func check2(value int64) { } } + +func check3() { + switch { } +} + +func check4() { + switch { + default: + } +}