Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions rust/ql/lib/codeql/rust/controlflow/CfgNodes.qll
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ class AstCfgNode extends CfgNode {
AstNode node;

AstCfgNode() { node = this.getAstNode() }

/** Gets the underlying ast node. */
AstNode getAstNode() { result = node }
}

/** A CFG node that corresponds to a parameter in the AST. */
Expand All @@ -22,6 +25,14 @@ class ParamCfgNode extends AstCfgNode {
Param getParam() { result = node }
}

/** A CFG node that corresponds to a parameter in the AST. */
class PatCfgNode extends AstCfgNode {
override Pat node;

/** Gets the underlying pattern. */
Pat getPat() { result = node }
}

/** A CFG node that corresponds to an expression in the AST. */
class ExprCfgNode extends AstCfgNode {
override Expr node;
Expand Down
50 changes: 32 additions & 18 deletions rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,19 @@
override Location getLocation() { none() }
}

/** A data flow node that corresponds to a CFG node for an AST node. */
abstract private class AstCfgFlowNode extends Node {

Check warning

Code scanning / CodeQL

UnusedField Warning

This class declares the
field n
but does not bind it in the characteristic predicate of any class between it and
ParameterNode
.
This class declares the
field n
but does not bind it in the characteristic predicate of any class between it and
ParameterNode
.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is a real problem. n is used in one of the predicates in AstCfgFlowNode and the subclasses do bind it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be able to suppress it with something like

    bindingset[this, n]
    AstCfgFlowNode() { exists(this) and exists(n) }

AstCfgNode n;

override CfgNode getCfgNode() { result = n }

override CfgScope getCfgScope() { result = n.getAstNode().getEnclosingCfgScope() }

override Location getLocation() { result = n.getAstNode().getLocation() }

override string toString() { result = n.getAstNode().toString() }
}

/**
* A node in the data flow graph that corresponds to an expression in the
* AST.
Expand All @@ -131,39 +144,34 @@
* to multiple `ExprNode`s, just like it may correspond to multiple
* `ControlFlow::Node`s.
*/
class ExprNode extends Node, TExprNode {
ExprCfgNode n;
class ExprNode extends AstCfgFlowNode, TExprNode {
override ExprCfgNode n;

ExprNode() { this = TExprNode(n) }

override CfgScope getCfgScope() { result = this.asExpr().getEnclosingCfgScope() }

override Location getLocation() { result = n.getExpr().getLocation() }
override Expr asExpr() { result = n.getExpr() }
}

override string toString() { result = n.getExpr().toString() }
final class PatNode extends AstCfgFlowNode, TPatNode {
override PatCfgNode n;

override Expr asExpr() { result = n.getExpr() }
PatNode() { this = TPatNode(n) }

override CfgNode getCfgNode() { result = n }
/** Gets the Pat in the AST that this node corresponds to. */
Pat getPat() { result = n.getPat() }
}

/**
* The value of a parameter at function entry, viewed as a node in a data
* flow graph.
*/
final class ParameterNode extends Node, TParameterNode {
ParamCfgNode parameter;
final class ParameterNode extends AstCfgFlowNode, TParameterNode {
override ParamCfgNode n;

ParameterNode() { this = TParameterNode(parameter) }

override CfgScope getCfgScope() { result = parameter.getParam().getEnclosingCfgScope() }

override Location getLocation() { result = parameter.getLocation() }

override string toString() { result = parameter.toString() }
ParameterNode() { this = TParameterNode(n) }

/** Gets the parameter in the AST that this node corresponds to. */
Param getParameter() { result = parameter.getParam() }
Param getParameter() { result = n.getParam() }
}

final class ArgumentNode = NaNode;
Expand Down Expand Up @@ -281,6 +289,11 @@
pragma[nomagic]
predicate localFlowStepCommon(Node nodeFrom, Node nodeTo) {
nodeFrom.getCfgNode() = getALastEvalNode(nodeTo.getCfgNode())
or
exists(LetStmt s |
nodeFrom.getCfgNode().getAstNode() = s.getInitializer() and
nodeTo.getCfgNode().getAstNode() = s.getPat()
)
Comment on lines +293 to +296
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not take CFG splitting into account, but that changes once #17918 lands.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I can also adapt this after #17918 (if you don't want to do it all yourself).

}
}

Expand Down Expand Up @@ -485,6 +498,7 @@
newtype TNode =
TExprNode(ExprCfgNode n) or
TParameterNode(ParamCfgNode p) or
TPatNode(PatCfgNode p) or
TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node)

cached
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ uniqueNodeLocation
| file://:0:0:0:0 | BlockExpr | Node should have one location but has 0. |
| file://:0:0:0:0 | Param | Node should have one location but has 0. |
| file://:0:0:0:0 | path | Node should have one location but has 0. |
| file://:0:0:0:0 | path | Node should have one location but has 0. |
missingLocation
| Nodes without location: 5 |
| Nodes without location: 6 |
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
| main.rs:2:9:2:9 | [SSA] s | main.rs:3:33:3:33 | s |
| main.rs:2:13:2:19 | "Hello" | main.rs:2:9:2:9 | s |
| main.rs:6:18:6:21 | [SSA] cond | main.rs:9:16:9:19 | cond |
| main.rs:7:9:7:9 | [SSA] a | main.rs:10:9:10:9 | a |
| main.rs:7:13:7:13 | 1 | main.rs:7:9:7:9 | a |
| main.rs:8:9:8:9 | [SSA] b | main.rs:12:9:12:9 | b |
| main.rs:8:13:8:13 | 2 | main.rs:8:9:8:9 | b |
| main.rs:9:9:9:9 | [SSA] c | main.rs:14:5:14:5 | c |
| main.rs:9:13:13:5 | IfExpr | main.rs:9:9:9:9 | c |
| main.rs:9:21:11:5 | BlockExpr | main.rs:9:13:13:5 | IfExpr |
| main.rs:10:9:10:9 | a | main.rs:9:21:11:5 | BlockExpr |
| main.rs:11:12:13:5 | BlockExpr | main.rs:9:13:13:5 | IfExpr |
| main.rs:12:9:12:9 | b | main.rs:11:12:13:5 | BlockExpr |
| main.rs:14:5:14:5 | c | main.rs:6:37:15:1 | BlockExpr |
| main.rs:18:9:18:9 | [SSA] a | main.rs:20:15:20:15 | a |
| main.rs:18:13:18:13 | 1 | main.rs:18:9:18:9 | a |
| main.rs:19:9:19:9 | [SSA] b | main.rs:22:5:22:5 | b |
| main.rs:19:13:21:5 | LoopExpr | main.rs:19:9:19:9 | b |
| main.rs:20:9:20:15 | BreakExpr | main.rs:19:13:21:5 | LoopExpr |
| main.rs:20:15:20:15 | a | main.rs:20:9:20:15 | BreakExpr |
| main.rs:22:5:22:5 | b | main.rs:17:29:23:1 | BlockExpr |
| main.rs:26:17:26:17 | 1 | main.rs:26:9:26:13 | i |
| main.rs:27:5:27:5 | [SSA] i | main.rs:28:5:28:5 | i |
| main.rs:27:5:27:5 | i | main.rs:27:5:27:5 | [SSA] i |
| main.rs:28:5:28:5 | i | main.rs:25:24:29:1 | BlockExpr |
Expand Down