From 2511986324a4ada020cc2306d7a036b7ae24d49b Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Thu, 19 Sep 2024 14:11:58 +0200 Subject: [PATCH] Rust: Address PR review comments --- .../rust/controlflow/internal/Completion.qll | 12 ++- .../internal/ControlFlowGraphImpl.qll | 30 ++++--- .../rust/controlflow/internal/Scope.qll | 2 +- .../controlflow/internal/SuccessorType.qll | 1 - .../library-tests/controlflow/Cfg.expected | 90 ++++++++++--------- .../ql/test/library-tests/controlflow/test.rs | 6 +- 6 files changed, 78 insertions(+), 63 deletions(-) diff --git a/rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll b/rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll index 0d75a4f6125a..4a4c092f9e13 100644 --- a/rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll +++ b/rust/ql/lib/codeql/rust/controlflow/internal/Completion.qll @@ -1,4 +1,3 @@ -private import codeql.util.Option private import codeql.util.Boolean private import codeql.rust.controlflow.ControlFlowGraph private import rust @@ -8,9 +7,14 @@ private newtype TCompletion = TSimpleCompletion() or TBooleanCompletion(Boolean b) or TMatchCompletion(Boolean isMatch) or - TLoopCompletion(TLoopJumpType kind, TLabelType label) or - TReturnCompletion() or - TDivergeCompletion() // A completion that never reaches the successor (e.g. by panicking or spinning) + TLoopCompletion(TLoopJumpType kind, TLabelType label) { + label = TNoLabel() + or + kind = TBreakJump() and label = TLabel(any(BreakExpr b).getLabel().getName()) + or + kind = TContinueJump() and label = TLabel(any(ContinueExpr b).getLabel().getName()) + } or + TReturnCompletion() /** A completion of a statement or an expression. */ abstract class Completion extends TCompletion { diff --git a/rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll b/rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll index 6c4c25cbfb67..567a7f0c8116 100644 --- a/rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll +++ b/rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll @@ -55,11 +55,11 @@ module CfgInput implements InputSig { predicate scopeLast(CfgScope scope, AstNode last, Completion c) { scope.scopeLast(last, c) } } -module CfgImpl = Make; +private module CfgImpl = Make; import CfgImpl -/** Holds for a trivial pattern that is always guaranteed to match. */ +/** Holds if `p` is a trivial pattern that is always guaranteed to match. */ predicate trivialPat(Pat p) { p instanceof WildcardPat or p instanceof IdentPat } class AsmExprTree extends LeafTree instanceof AsmExpr { } @@ -90,7 +90,6 @@ class LogicalOrBinaryOpExprTree extends PreOrderTree instanceof BinaryExpr { child = [super.getRhs(), super.getLhs()] } - // override predicate first(AstNode node) { first(super.getLhs(), node) } override predicate succ(AstNode pred, AstNode succ, Completion c) { // Edge to the first node in the lhs pred = this and @@ -109,7 +108,7 @@ class LogicalOrBinaryOpExprTree extends PreOrderTree instanceof BinaryExpr { c.(BooleanCompletion).succeeded() or // Rhs. as the last node - last(super.getRhs(), node, c) // and + last(super.getRhs(), node, c) } } @@ -120,7 +119,6 @@ class LogicalAndBinaryOpExprTree extends PreOrderTree instanceof BinaryExpr { child = [super.getRhs(), super.getLhs()] } - // override predicate first(AstNode node) { first(super.getLhs(), node) } override predicate succ(AstNode pred, AstNode succ, Completion c) { // Edge to the first node in the lhs pred = this and @@ -283,15 +281,15 @@ class LetStmtTree extends PreOrderTree instanceof LetStmt { pred = this and first(super.getInitializer(), succ) and completionIsSimple(c) or // Edge from end of initializer to pattern. - last(super.getInitializer(), pred, c) and succ = super.getPat() + last(super.getInitializer(), pred, c) and first(super.getPat(), succ) or // Edge from failed pattern to `else` branch. - pred = super.getPat() and first(super.getElse(), succ) and c.(MatchCompletion).failed() + last(super.getPat(), pred, c) and first(super.getElse(), succ) and c.(MatchCompletion).failed() } override predicate last(AstNode node, Completion c) { // Edge out of a successfully matched pattern. - node = super.getPat() and c.(MatchCompletion).succeeded() + last(super.getPat(), node, c) and c.(MatchCompletion).succeeded() // NOTE: No edge out of the `else` branch as that is guaranteed to diverge. } } @@ -304,7 +302,7 @@ class LoopExprTree extends PostOrderTree instanceof LoopExpr { override predicate first(AstNode node) { first(super.getBody(), node) } /** Whether this `LoopExpr` captures the `c` completion. */ - predicate capturesLoopJumpCompletion(LoopJumpCompletion c) { + private predicate capturesLoopJumpCompletion(LoopJumpCompletion c) { not c.hasLabel() or c.getLabelName() = super.getLabel().getName() @@ -346,7 +344,11 @@ class MatchArmTree extends ControlFlowTree instanceof MatchArm { // Edge from pattern to guard/arm if match succeeds. pred = super.getPat() and c.(MatchCompletion).succeeded() and - (if super.hasGuard() then first(super.getGuard(), succ) else first(super.getExpr(), succ)) + ( + first(super.getGuard(), succ) + or + not super.hasGuard() and first(super.getExpr(), succ) + ) or // Edge from guard to arm if the guard succeeds. last(super.getGuard(), pred, c) and @@ -364,7 +366,9 @@ class MatchArmTree extends ControlFlowTree instanceof MatchArm { } class MatchExprTree extends PostOrderTree instanceof MatchExpr { - override predicate propagatesAbnormal(AstNode child) { child = super.getABranch().getExpr() } + override predicate propagatesAbnormal(AstNode child) { + child = [super.getExpr(), super.getABranch().getExpr()] + } override predicate first(AstNode node) { first(super.getExpr(), node) } @@ -380,7 +384,7 @@ class MatchExprTree extends PostOrderTree instanceof MatchExpr { ) or // Edge from the end of each arm to the match expression. - last(super.getBranch(_), pred, c) and succ = this and completionIsSimple(c) + last(super.getBranch(_), pred, c) and succ = this and completionIsNormal(c) } } @@ -436,7 +440,7 @@ class ReturnExprTree extends PostOrderTree instanceof ReturnExpr { } override predicate succ(AstNode pred, AstNode succ, Completion c) { - last(super.getExpr(), pred, c) and succ = this + last(super.getExpr(), pred, c) and succ = this and completionIsNormal(c) } } diff --git a/rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll b/rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll index ab8cc05782ec..bfc0d2ed3b9a 100644 --- a/rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll +++ b/rust/ql/lib/codeql/rust/controlflow/internal/Scope.qll @@ -17,7 +17,7 @@ final class FunctionScope extends CfgScope, Function { override predicate scopeLast(AstNode node, Completion c) { last(this.getBody(), node, c) } } -final class LambdaScope extends CfgScope, ClosureExpr { +final class ClosureScope extends CfgScope, ClosureExpr { override predicate scopeFirst(AstNode node) { first(this.getBody(), node) } override predicate scopeLast(AstNode node, Completion c) { last(this.getBody(), node, c) } diff --git a/rust/ql/lib/codeql/rust/controlflow/internal/SuccessorType.qll b/rust/ql/lib/codeql/rust/controlflow/internal/SuccessorType.qll index 040665b0b137..96598dd220bd 100644 --- a/rust/ql/lib/codeql/rust/controlflow/internal/SuccessorType.qll +++ b/rust/ql/lib/codeql/rust/controlflow/internal/SuccessorType.qll @@ -17,7 +17,6 @@ newtype TSuccessorType = TLoopSuccessor(TLoopJumpType kind, TLabelType label) or TReturnSuccessor() -// class TBreakSuccessor = TUnlabeledBreakSuccessor or TLabeledBreakSuccessor; /** The type of a control flow successor. */ abstract private class SuccessorTypeImpl extends TSuccessorType { /** Gets a textual representation of successor type. */ diff --git a/rust/ql/test/library-tests/controlflow/Cfg.expected b/rust/ql/test/library-tests/controlflow/Cfg.expected index 1f038fc4a880..baeb258d7b09 100644 --- a/rust/ql/test/library-tests/controlflow/Cfg.expected +++ b/rust/ql/test/library-tests/controlflow/Cfg.expected @@ -192,17 +192,17 @@ nodes | test.rs:106:44:112:1 | BlockExpr | semmle.order | 191 | | test.rs:107:5:111:5 | MatchExpr | semmle.order | 192 | | test.rs:107:11:107:21 | PathExpr | semmle.order | 193 | -| test.rs:108:9:108:15 | TupleStructPat | semmle.order | 194 | -| test.rs:108:20:108:20 | PathExpr | semmle.order | 195 | -| test.rs:108:20:108:25 | BinaryExpr | semmle.order | 196 | -| test.rs:108:24:108:25 | LiteralExpr | semmle.order | 197 | -| test.rs:108:30:108:30 | PathExpr | semmle.order | 198 | -| test.rs:108:30:108:34 | BinaryExpr | semmle.order | 199 | -| test.rs:108:34:108:34 | LiteralExpr | semmle.order | 200 | -| test.rs:109:9:109:15 | TupleStructPat | semmle.order | 201 | -| test.rs:109:20:109:20 | PathExpr | semmle.order | 202 | -| test.rs:110:9:110:12 | IdentPat | semmle.order | 203 | -| test.rs:110:17:110:17 | LiteralExpr | semmle.order | 204 | +| test.rs:108:9:108:23 | TupleStructPat | semmle.order | 194 | +| test.rs:108:28:108:28 | PathExpr | semmle.order | 195 | +| test.rs:108:28:108:33 | BinaryExpr | semmle.order | 196 | +| test.rs:108:32:108:33 | LiteralExpr | semmle.order | 197 | +| test.rs:108:38:108:38 | PathExpr | semmle.order | 198 | +| test.rs:108:38:108:42 | BinaryExpr | semmle.order | 199 | +| test.rs:108:42:108:42 | LiteralExpr | semmle.order | 200 | +| test.rs:109:9:109:23 | TupleStructPat | semmle.order | 201 | +| test.rs:109:28:109:28 | PathExpr | semmle.order | 202 | +| test.rs:110:9:110:20 | PathPat | semmle.order | 203 | +| test.rs:110:25:110:25 | LiteralExpr | semmle.order | 204 | | test.rs:115:5:120:5 | enter test_infinite_loop | semmle.order | 205 | | test.rs:116:9:118:9 | ExprStmt | semmle.order | 206 | | test.rs:116:14:118:9 | BlockExpr | semmle.order | 207 | @@ -618,36 +618,44 @@ edges | test.rs:106:44:112:1 | BlockExpr | test.rs:106:1:112:1 | exit test_match (normal) | semmle.order | 1 | | test.rs:107:5:111:5 | MatchExpr | test.rs:106:44:112:1 | BlockExpr | semmle.label | | | test.rs:107:5:111:5 | MatchExpr | test.rs:106:44:112:1 | BlockExpr | semmle.order | 1 | -| test.rs:107:11:107:21 | PathExpr | test.rs:108:9:108:15 | TupleStructPat | semmle.label | | -| test.rs:107:11:107:21 | PathExpr | test.rs:108:9:108:15 | TupleStructPat | semmle.order | 1 | -| test.rs:108:9:108:15 | TupleStructPat | test.rs:108:20:108:20 | PathExpr | semmle.label | match | -| test.rs:108:9:108:15 | TupleStructPat | test.rs:108:20:108:20 | PathExpr | semmle.order | 1 | -| test.rs:108:9:108:15 | TupleStructPat | test.rs:109:9:109:15 | TupleStructPat | semmle.label | no-match | -| test.rs:108:9:108:15 | TupleStructPat | test.rs:109:9:109:15 | TupleStructPat | semmle.order | 2 | -| test.rs:108:20:108:20 | PathExpr | test.rs:108:24:108:25 | LiteralExpr | semmle.label | | -| test.rs:108:20:108:20 | PathExpr | test.rs:108:24:108:25 | LiteralExpr | semmle.order | 1 | -| test.rs:108:20:108:25 | BinaryExpr | test.rs:108:30:108:30 | PathExpr | semmle.label | true | -| test.rs:108:20:108:25 | BinaryExpr | test.rs:108:30:108:30 | PathExpr | semmle.order | 1 | -| test.rs:108:20:108:25 | BinaryExpr | test.rs:109:9:109:15 | TupleStructPat | semmle.label | false | -| test.rs:108:20:108:25 | BinaryExpr | test.rs:109:9:109:15 | TupleStructPat | semmle.order | 2 | -| test.rs:108:24:108:25 | LiteralExpr | test.rs:108:20:108:25 | BinaryExpr | semmle.label | | -| test.rs:108:24:108:25 | LiteralExpr | test.rs:108:20:108:25 | BinaryExpr | semmle.order | 1 | -| test.rs:108:30:108:30 | PathExpr | test.rs:108:34:108:34 | LiteralExpr | semmle.label | | -| test.rs:108:30:108:30 | PathExpr | test.rs:108:34:108:34 | LiteralExpr | semmle.order | 1 | -| test.rs:108:30:108:34 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label | | -| test.rs:108:30:108:34 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | -| test.rs:108:34:108:34 | LiteralExpr | test.rs:108:30:108:34 | BinaryExpr | semmle.label | | -| test.rs:108:34:108:34 | LiteralExpr | test.rs:108:30:108:34 | BinaryExpr | semmle.order | 1 | -| test.rs:109:9:109:15 | TupleStructPat | test.rs:109:20:109:20 | PathExpr | semmle.label | match | -| test.rs:109:9:109:15 | TupleStructPat | test.rs:109:20:109:20 | PathExpr | semmle.order | 1 | -| test.rs:109:9:109:15 | TupleStructPat | test.rs:110:9:110:12 | IdentPat | semmle.label | no-match | -| test.rs:109:9:109:15 | TupleStructPat | test.rs:110:9:110:12 | IdentPat | semmle.order | 2 | -| test.rs:109:20:109:20 | PathExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label | | -| test.rs:109:20:109:20 | PathExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | -| test.rs:110:9:110:12 | IdentPat | test.rs:110:17:110:17 | LiteralExpr | semmle.label | match | -| test.rs:110:9:110:12 | IdentPat | test.rs:110:17:110:17 | LiteralExpr | semmle.order | 1 | -| test.rs:110:17:110:17 | LiteralExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label | | -| test.rs:110:17:110:17 | LiteralExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | +| test.rs:107:11:107:21 | PathExpr | test.rs:108:9:108:23 | TupleStructPat | semmle.label | | +| test.rs:107:11:107:21 | PathExpr | test.rs:108:9:108:23 | TupleStructPat | semmle.order | 1 | +| test.rs:108:9:108:23 | TupleStructPat | test.rs:107:5:111:5 | MatchExpr | semmle.label | no-match | +| test.rs:108:9:108:23 | TupleStructPat | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | +| test.rs:108:9:108:23 | TupleStructPat | test.rs:108:28:108:28 | PathExpr | semmle.label | match | +| test.rs:108:9:108:23 | TupleStructPat | test.rs:108:28:108:28 | PathExpr | semmle.order | 2 | +| test.rs:108:9:108:23 | TupleStructPat | test.rs:109:9:109:23 | TupleStructPat | semmle.label | no-match | +| test.rs:108:9:108:23 | TupleStructPat | test.rs:109:9:109:23 | TupleStructPat | semmle.order | 3 | +| test.rs:108:28:108:28 | PathExpr | test.rs:108:32:108:33 | LiteralExpr | semmle.label | | +| test.rs:108:28:108:28 | PathExpr | test.rs:108:32:108:33 | LiteralExpr | semmle.order | 1 | +| test.rs:108:28:108:33 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label | false | +| test.rs:108:28:108:33 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | +| test.rs:108:28:108:33 | BinaryExpr | test.rs:108:38:108:38 | PathExpr | semmle.label | true | +| test.rs:108:28:108:33 | BinaryExpr | test.rs:108:38:108:38 | PathExpr | semmle.order | 2 | +| test.rs:108:28:108:33 | BinaryExpr | test.rs:109:9:109:23 | TupleStructPat | semmle.label | false | +| test.rs:108:28:108:33 | BinaryExpr | test.rs:109:9:109:23 | TupleStructPat | semmle.order | 3 | +| test.rs:108:32:108:33 | LiteralExpr | test.rs:108:28:108:33 | BinaryExpr | semmle.label | | +| test.rs:108:32:108:33 | LiteralExpr | test.rs:108:28:108:33 | BinaryExpr | semmle.order | 1 | +| test.rs:108:38:108:38 | PathExpr | test.rs:108:42:108:42 | LiteralExpr | semmle.label | | +| test.rs:108:38:108:38 | PathExpr | test.rs:108:42:108:42 | LiteralExpr | semmle.order | 1 | +| test.rs:108:38:108:42 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label | | +| test.rs:108:38:108:42 | BinaryExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | +| test.rs:108:42:108:42 | LiteralExpr | test.rs:108:38:108:42 | BinaryExpr | semmle.label | | +| test.rs:108:42:108:42 | LiteralExpr | test.rs:108:38:108:42 | BinaryExpr | semmle.order | 1 | +| test.rs:109:9:109:23 | TupleStructPat | test.rs:107:5:111:5 | MatchExpr | semmle.label | no-match | +| test.rs:109:9:109:23 | TupleStructPat | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | +| test.rs:109:9:109:23 | TupleStructPat | test.rs:109:28:109:28 | PathExpr | semmle.label | match | +| test.rs:109:9:109:23 | TupleStructPat | test.rs:109:28:109:28 | PathExpr | semmle.order | 2 | +| test.rs:109:9:109:23 | TupleStructPat | test.rs:110:9:110:20 | PathPat | semmle.label | no-match | +| test.rs:109:9:109:23 | TupleStructPat | test.rs:110:9:110:20 | PathPat | semmle.order | 3 | +| test.rs:109:28:109:28 | PathExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label | | +| test.rs:109:28:109:28 | PathExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | +| test.rs:110:9:110:20 | PathPat | test.rs:107:5:111:5 | MatchExpr | semmle.label | no-match | +| test.rs:110:9:110:20 | PathPat | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | +| test.rs:110:9:110:20 | PathPat | test.rs:110:25:110:25 | LiteralExpr | semmle.label | match | +| test.rs:110:9:110:20 | PathPat | test.rs:110:25:110:25 | LiteralExpr | semmle.order | 2 | +| test.rs:110:25:110:25 | LiteralExpr | test.rs:107:5:111:5 | MatchExpr | semmle.label | | +| test.rs:110:25:110:25 | LiteralExpr | test.rs:107:5:111:5 | MatchExpr | semmle.order | 1 | | test.rs:115:5:120:5 | enter test_infinite_loop | test.rs:116:9:118:9 | ExprStmt | semmle.label | | | test.rs:115:5:120:5 | enter test_infinite_loop | test.rs:116:9:118:9 | ExprStmt | semmle.order | 1 | | test.rs:116:9:118:9 | ExprStmt | test.rs:117:13:117:13 | LiteralExpr | semmle.label | | diff --git a/rust/ql/test/library-tests/controlflow/test.rs b/rust/ql/test/library-tests/controlflow/test.rs index 9901b6aae4a1..a5a19505e143 100644 --- a/rust/ql/test/library-tests/controlflow/test.rs +++ b/rust/ql/test/library-tests/controlflow/test.rs @@ -105,9 +105,9 @@ mod logical_operators { fn test_match(maybe_digit: Option) -> { match maybe_digit { - Some(x) if x < 10 => x + 5, - Some(x) => x, - None => 5, + Option::Some(x) if x < 10 => x + 5, + Option::Some(x) => x, + Option::None => 5, } }