From 05e37a74656542bc93ca0a4b2f29ed9fab11724b Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 22 Dec 2021 10:02:31 +0100 Subject: [PATCH 1/5] C#: Promote existing ad-hoc consistency checks to consistency queries --- .../ql/consistency-queries/AstConsistency.qll | 19 ++++ .../GenericsConsistency.qll | 9 ++ .../ql/consistency-queries/SsaConsistency.ql | 16 +++ .../code/csharp/commons/ConsistencyChecks.qll | 102 ------------------ .../dataflow/internal/SsaImplCommon.qll | 11 ++ .../dataflow/ssa/SsaConsistency.expected | 0 .../dataflow/ssa/SsaConsistency.ql | 6 -- .../generics/ConsistencyChecks.expected | 0 .../generics/ConsistencyChecks.ql | 9 -- 9 files changed, 55 insertions(+), 117 deletions(-) create mode 100644 csharp/ql/consistency-queries/AstConsistency.qll create mode 100644 csharp/ql/consistency-queries/GenericsConsistency.qll delete mode 100644 csharp/ql/lib/semmle/code/csharp/commons/ConsistencyChecks.qll delete mode 100644 csharp/ql/test/library-tests/dataflow/ssa/SsaConsistency.expected delete mode 100644 csharp/ql/test/library-tests/dataflow/ssa/SsaConsistency.ql delete mode 100644 csharp/ql/test/library-tests/generics/ConsistencyChecks.expected delete mode 100644 csharp/ql/test/library-tests/generics/ConsistencyChecks.ql diff --git a/csharp/ql/consistency-queries/AstConsistency.qll b/csharp/ql/consistency-queries/AstConsistency.qll new file mode 100644 index 000000000000..5373b638db7b --- /dev/null +++ b/csharp/ql/consistency-queries/AstConsistency.qll @@ -0,0 +1,19 @@ +import csharp + +query predicate missingLocation(Element e) { + ( + e instanceof Declaration or + e instanceof Expr or + e instanceof Stmt + ) and + not e instanceof ImplicitAccessorParameter and + not e instanceof NullType and + not e instanceof Parameter and // Bug in Roslyn - params occasionally lack locations + not e.(Operator).getDeclaringType() instanceof IntType and // Roslyn quirk + not e instanceof Constructor and + not e instanceof ArrayType and + not e instanceof UnknownType and + not e instanceof ArglistType and + not exists(TupleType t | e = t or e = t.getAField()) and + not exists(e.getLocation()) +} diff --git a/csharp/ql/consistency-queries/GenericsConsistency.qll b/csharp/ql/consistency-queries/GenericsConsistency.qll new file mode 100644 index 000000000000..2f31ef8065fb --- /dev/null +++ b/csharp/ql/consistency-queries/GenericsConsistency.qll @@ -0,0 +1,9 @@ +import csharp + +query predicate missingUnbound(ConstructedGeneric g) { not exists(g.getUnboundGeneric()) } + +query predicate missingArgs(ConstructedGeneric g) { g.getNumberOfTypeArguments() = 0 } + +query predicate inconsistentArgCount(ConstructedGeneric g) { + g.getUnboundGeneric().getNumberOfTypeParameters() != g.getNumberOfTypeArguments() +} diff --git a/csharp/ql/consistency-queries/SsaConsistency.ql b/csharp/ql/consistency-queries/SsaConsistency.ql index 0e14eb75c869..ee2318282795 100644 --- a/csharp/ql/consistency-queries/SsaConsistency.ql +++ b/csharp/ql/consistency-queries/SsaConsistency.ql @@ -1,5 +1,6 @@ import csharp import semmle.code.csharp.dataflow.internal.SsaImplCommon::Consistency +import Ssa class MyRelevantDefinition extends RelevantDefinition, Ssa::Definition { override predicate hasLocationInfo( @@ -8,3 +9,18 @@ class MyRelevantDefinition extends RelevantDefinition, Ssa::Definition { this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) } } + +query predicate localDeclWithSsaDef(LocalVariableDeclExpr d) { + // Local variables in C# must be initialized before every use, so uninitialized + // local variables should not have an SSA definition, as that would imply that + // the declaration is live (can reach a use without passing through a definition) + exists(ExplicitDefinition def | + d = def.getADefinition().(AssignableDefinitions::LocalVariableDefinition).getDeclaration() + | + not d = any(ForeachStmt fs).getVariableDeclExpr() and + not d = any(SpecificCatchClause scc).getVariableDeclExpr() and + not d.getVariable().getType() instanceof Struct and + not d instanceof PatternExpr and + not d.getVariable().isCaptured() + ) +} diff --git a/csharp/ql/lib/semmle/code/csharp/commons/ConsistencyChecks.qll b/csharp/ql/lib/semmle/code/csharp/commons/ConsistencyChecks.qll deleted file mode 100644 index 79ff71071292..000000000000 --- a/csharp/ql/lib/semmle/code/csharp/commons/ConsistencyChecks.qll +++ /dev/null @@ -1,102 +0,0 @@ -/** - * INTERNAL: Do not use. - * - * Provides functionality for validating that the database and library are in a - * consistent state. - */ - -import csharp - -private predicate missingLocation(Element e, string m) { - ( - e instanceof Declaration or - e instanceof Expr or - e instanceof Stmt - ) and - not e instanceof ImplicitAccessorParameter and - not e instanceof NullType and - not e instanceof Parameter and // Bug in Roslyn - params occasionally lack locations - not e.(Operator).getDeclaringType() instanceof IntType and // Roslyn quirk - not e instanceof Constructor and - not e instanceof ArrayType and - not e instanceof UnknownType and - not e instanceof ArglistType and - not exists(TupleType t | e = t or e = t.getAField()) and - not exists(e.getLocation()) and - m = "Element does not have a location" -} - -private predicate inconsistentGeneric(ConstructedGeneric g, string m) { - not exists(g.getUnboundGeneric()) and - m = "Constructed generic does not have an unbound generic" - or - g.getNumberOfTypeArguments() = 0 and - m = "Constructed generic has no type arguments" - or - g.getUnboundGeneric().getNumberOfTypeParameters() != g.getNumberOfTypeArguments() and - m = "Inconsistent number of type arguments/parameters" -} - -module SsaChecks { - import Ssa - - predicate nonUniqueSsaDef(AssignableRead read, string m) { - exists(ControlFlow::Node cfn | strictcount(Definition def | def.getAReadAtNode(cfn) = read) > 1) and - m = "Read is associated with multiple SSA definitions" - } - - predicate notDominatedByDef(AssignableRead read, string m) { - exists( - Definition def, ControlFlow::BasicBlock bb, ControlFlow::Node rnode, ControlFlow::Node dnode, - int i - | - def.getAReadAtNode(rnode) = read - | - def.definesAt(_, bb, i) and - dnode = bb.getNode(max(int j | j = i or j = 0)) and - not dnode.dominates(rnode) - ) and - m = "Read is not dominated by SSA definition" - } - - predicate localDeclWithSsaDef(LocalVariableDeclExpr d, string m) { - // Local variables in C# must be initialized before every use, so uninitialized - // local variables should not have an SSA definition, as that would imply that - // the declaration is live (can reach a use without passing through a definition) - exists(ExplicitDefinition def | - d = def.getADefinition().(AssignableDefinitions::LocalVariableDefinition).getDeclaration() - | - not d = any(ForeachStmt fs).getVariableDeclExpr() and - not d = any(SpecificCatchClause scc).getVariableDeclExpr() and - not d.getVariable().getType() instanceof Struct and - not d = any(BindingPatternExpr bpe).getVariableDeclExpr() - ) and - m = "Local variable declaration has unexpected SSA definition" - } - - predicate ssaConsistencyFailure(Element e, string m) { - nonUniqueSsaDef(e, m) or - notDominatedByDef(e, m) or - localDeclWithSsaDef(e, m) - } -} - -module CfgChecks { - predicate multipleSuccessors(ControlFlowElement cfe, string m) { - exists(ControlFlow::Node cfn | cfn = cfe.getAControlFlowNode() | - strictcount(cfn.getASuccessorByType(any(ControlFlow::SuccessorTypes::NormalSuccessor e))) > 1 and - m = "Multiple (non-conditional/exceptional) successors" - ) - } -} - -/** - * Holds if element `e` has a consistency failure, as described by - * the message in `m`. - */ -predicate consistencyFailure(Element e, string m) { - missingLocation(e, m) or - inconsistentGeneric(e, m) or - SsaChecks::ssaConsistencyFailure(e, m) or - CfgChecks::multipleSuccessors(e, m) -} diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImplCommon.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImplCommon.qll index a7b1c571a7a8..19bf1d4f27de 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImplCommon.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImplCommon.qll @@ -659,4 +659,15 @@ module Consistency { not phiHasInputFromBlock(_, def, _) and not uncertainWriteDefinitionInput(_, def) } + + query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { + exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) | + ssaDefReachesReadWithinBlock(v, def, bb, i) and + (bb != bbDef or i < iDef) + or + ssaDefReachesRead(v, def, bb, i) and + not ssaDefReachesReadWithinBlock(v, def, bb, i) and + not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _) + ) + } } diff --git a/csharp/ql/test/library-tests/dataflow/ssa/SsaConsistency.expected b/csharp/ql/test/library-tests/dataflow/ssa/SsaConsistency.expected deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/csharp/ql/test/library-tests/dataflow/ssa/SsaConsistency.ql b/csharp/ql/test/library-tests/dataflow/ssa/SsaConsistency.ql deleted file mode 100644 index ea96a2923619..000000000000 --- a/csharp/ql/test/library-tests/dataflow/ssa/SsaConsistency.ql +++ /dev/null @@ -1,6 +0,0 @@ -import csharp -import semmle.code.csharp.commons.ConsistencyChecks - -from Element e, string m -where SsaChecks::ssaConsistencyFailure(e, m) -select e, m diff --git a/csharp/ql/test/library-tests/generics/ConsistencyChecks.expected b/csharp/ql/test/library-tests/generics/ConsistencyChecks.expected deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/csharp/ql/test/library-tests/generics/ConsistencyChecks.ql b/csharp/ql/test/library-tests/generics/ConsistencyChecks.ql deleted file mode 100644 index 7052c6f697a3..000000000000 --- a/csharp/ql/test/library-tests/generics/ConsistencyChecks.ql +++ /dev/null @@ -1,9 +0,0 @@ -/* - * @name Checks for model consistency - */ - -import semmle.code.csharp.commons.ConsistencyChecks - -from Element e, string m -where consistencyFailure(e, m) -select e, "Element class " + e.getPrimaryQlClasses() + " has consistency check failed: " + m From 915c0fdf9b9aa6e722c96aa928a0a4430c987729 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 22 Dec 2021 10:03:03 +0100 Subject: [PATCH 2/5] Shared SSA: Sync files --- .../code/cpp/ir/dataflow/internal/SsaImplCommon.qll | 11 +++++++++++ .../ql/lib/semmle/code/cil/internal/SsaImplCommon.qll | 11 +++++++++++ .../controlflow/internal/pressa/SsaImplCommon.qll | 11 +++++++++++ .../dataflow/internal/basessa/SsaImplCommon.qll | 11 +++++++++++ .../codeql/ruby/dataflow/internal/SsaImplCommon.qll | 11 +++++++++++ 5 files changed, 55 insertions(+) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImplCommon.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImplCommon.qll index a7b1c571a7a8..19bf1d4f27de 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImplCommon.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImplCommon.qll @@ -659,4 +659,15 @@ module Consistency { not phiHasInputFromBlock(_, def, _) and not uncertainWriteDefinitionInput(_, def) } + + query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { + exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) | + ssaDefReachesReadWithinBlock(v, def, bb, i) and + (bb != bbDef or i < iDef) + or + ssaDefReachesRead(v, def, bb, i) and + not ssaDefReachesReadWithinBlock(v, def, bb, i) and + not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _) + ) + } } diff --git a/csharp/ql/lib/semmle/code/cil/internal/SsaImplCommon.qll b/csharp/ql/lib/semmle/code/cil/internal/SsaImplCommon.qll index a7b1c571a7a8..19bf1d4f27de 100644 --- a/csharp/ql/lib/semmle/code/cil/internal/SsaImplCommon.qll +++ b/csharp/ql/lib/semmle/code/cil/internal/SsaImplCommon.qll @@ -659,4 +659,15 @@ module Consistency { not phiHasInputFromBlock(_, def, _) and not uncertainWriteDefinitionInput(_, def) } + + query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { + exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) | + ssaDefReachesReadWithinBlock(v, def, bb, i) and + (bb != bbDef or i < iDef) + or + ssaDefReachesRead(v, def, bb, i) and + not ssaDefReachesReadWithinBlock(v, def, bb, i) and + not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _) + ) + } } diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/pressa/SsaImplCommon.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/pressa/SsaImplCommon.qll index a7b1c571a7a8..19bf1d4f27de 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/pressa/SsaImplCommon.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/pressa/SsaImplCommon.qll @@ -659,4 +659,15 @@ module Consistency { not phiHasInputFromBlock(_, def, _) and not uncertainWriteDefinitionInput(_, def) } + + query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { + exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) | + ssaDefReachesReadWithinBlock(v, def, bb, i) and + (bb != bbDef or i < iDef) + or + ssaDefReachesRead(v, def, bb, i) and + not ssaDefReachesReadWithinBlock(v, def, bb, i) and + not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _) + ) + } } diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/basessa/SsaImplCommon.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/basessa/SsaImplCommon.qll index a7b1c571a7a8..19bf1d4f27de 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/basessa/SsaImplCommon.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/basessa/SsaImplCommon.qll @@ -659,4 +659,15 @@ module Consistency { not phiHasInputFromBlock(_, def, _) and not uncertainWriteDefinitionInput(_, def) } + + query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { + exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) | + ssaDefReachesReadWithinBlock(v, def, bb, i) and + (bb != bbDef or i < iDef) + or + ssaDefReachesRead(v, def, bb, i) and + not ssaDefReachesReadWithinBlock(v, def, bb, i) and + not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _) + ) + } } diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImplCommon.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImplCommon.qll index a7b1c571a7a8..19bf1d4f27de 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImplCommon.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImplCommon.qll @@ -659,4 +659,15 @@ module Consistency { not phiHasInputFromBlock(_, def, _) and not uncertainWriteDefinitionInput(_, def) } + + query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) { + exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) | + ssaDefReachesReadWithinBlock(v, def, bb, i) and + (bb != bbDef or i < iDef) + or + ssaDefReachesRead(v, def, bb, i) and + not ssaDefReachesReadWithinBlock(v, def, bb, i) and + not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _) + ) + } } From a3b1fb603a565963d99b86857fcd764bb164bb81 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 22 Dec 2021 10:03:43 +0100 Subject: [PATCH 3/5] C#: Add missing tuple declarations to `PatternExpr` `x` and `y` in `pair is var (x, y) ? x : null` are now correctly part of `PatternExpr`. --- .../ql/lib/semmle/code/csharp/exprs/Expr.qll | 15 +++++++----- .../library-tests/csharp8/PrintAst.expected | 8 +++---- .../dataflow/tuples/DataFlowStep.expected | 2 ++ .../dataflow/tuples/PrintAst.expected | 24 +++++++++---------- 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll b/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll index 7623b3d51b7f..8c53c7ea0c5b 100644 --- a/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll +++ b/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll @@ -316,6 +316,12 @@ private predicate hasChildPattern(ControlFlowElement pm, Expr child) { child = mid.getChildExpr(0) or child = mid.getChildExpr(1) ) + or + exists(Expr mid | + hasChildPattern(pm, mid) and + mid instanceof @tuple_expr and + child = mid.getAChildExpr() + ) } /** @@ -420,13 +426,10 @@ class TypeAccessPatternExpr extends TypePatternExpr, TypeAccess { override string getAPrimaryQlClass() { result = "TypeAccessPatternExpr" } } -/** A pattern that may bind a variable, for example `string s` in `x is string s`. */ -class BindingPatternExpr extends PatternExpr { - BindingPatternExpr() { - this instanceof LocalVariableDeclExpr or - this instanceof @recursive_pattern_expr - } +private class TBindingPatternExpr = @local_var_decl_expr or @recursive_pattern_expr; +/** A pattern that may bind a variable, for example `string s` in `x is string s`. */ +class BindingPatternExpr extends PatternExpr, TBindingPatternExpr { /** * Gets the local variable declaration of this pattern, if any. For example, * `string s` in `string { Length: 5 } s`. diff --git a/csharp/ql/test/library-tests/csharp8/PrintAst.expected b/csharp/ql/test/library-tests/csharp8/PrintAst.expected index 1be0350a2549..a9552216b59f 100644 --- a/csharp/ql/test/library-tests/csharp8/PrintAst.expected +++ b/csharp/ql/test/library-tests/csharp8/PrintAst.expected @@ -932,8 +932,8 @@ patterns.cs: # 58| 10: [BreakStmt] break; # 59| 11: [CaseStmt] case ...: # 59| 0: [TupleExpr] (..., ...) -# 59| 0: [LocalVariableDeclExpr] Int32 x -# 59| 1: [LocalVariableDeclExpr] Int32 y +# 59| 0: [VariablePatternExpr] Int32 x +# 59| 1: [VariablePatternExpr] Int32 y # 60| 12: [BreakStmt] break; # 61| 13: [DefaultCase] default: # 62| 14: [BreakStmt] break; @@ -1159,8 +1159,8 @@ patterns.cs: # 130| 2: [IntLiteral] 2 # 131| 3: [SwitchCaseExpr] ... => ... # 131| 0: [TupleExpr] (..., ...) -# 131| 0: [LocalVariableDeclExpr] Int32 x -# 131| 1: [DiscardExpr] _ +# 131| 0: [VariablePatternExpr] Int32 x +# 131| 1: [DiscardPatternExpr] _ # 131| 2: [IntLiteral] 3 # 134| 2: [TryStmt] try {...} ... # 135| 0: [BlockStmt] {...} diff --git a/csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.expected b/csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.expected index 99eef2875592..2c457546eb3e 100644 --- a/csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.expected +++ b/csharp/ql/test/library-tests/dataflow/tuples/DataFlowStep.expected @@ -57,6 +57,7 @@ | Tuples.cs:50:17:50:56 | (..., ...) | Tuples.cs:50:13:50:56 | SSA def(x) | | Tuples.cs:51:17:51:17 | access to local variable x | Tuples.cs:53:18:53:57 | SSA def(t) | | Tuples.cs:51:17:51:17 | access to local variable x | Tuples.cs:58:18:58:35 | (..., ...) | +| Tuples.cs:51:17:51:17 | access to local variable x | Tuples.cs:58:18:58:35 | (..., ...) | | Tuples.cs:51:17:51:17 | access to local variable x | Tuples.cs:77:13:77:13 | access to local variable x | | Tuples.cs:53:18:53:57 | SSA def(t) | Tuples.cs:53:64:53:64 | access to local variable t | | Tuples.cs:53:18:53:57 | SSA qualifier def(t.Item1) | Tuples.cs:54:22:54:28 | access to field Item1 | @@ -92,6 +93,7 @@ | Tuples.cs:70:22:70:28 | [post] access to field Item2 | Tuples.cs:72:22:72:28 | access to field Item2 | | Tuples.cs:70:22:70:28 | access to field Item2 | Tuples.cs:72:22:72:28 | access to field Item2 | | Tuples.cs:77:13:77:13 | access to local variable x | Tuples.cs:77:18:77:35 | (..., ...) | +| Tuples.cs:77:13:77:13 | access to local variable x | Tuples.cs:77:18:77:35 | (..., ...) | | Tuples.cs:77:23:77:23 | SSA def(p) | Tuples.cs:79:18:79:18 | access to local variable p | | Tuples.cs:77:27:77:27 | SSA def(q) | Tuples.cs:81:18:81:18 | access to local variable q | | Tuples.cs:77:30:77:30 | SSA def(r) | Tuples.cs:80:18:80:18 | access to local variable r | diff --git a/csharp/ql/test/library-tests/dataflow/tuples/PrintAst.expected b/csharp/ql/test/library-tests/dataflow/tuples/PrintAst.expected index bf10c1273b2d..bfec83782201 100644 --- a/csharp/ql/test/library-tests/dataflow/tuples/PrintAst.expected +++ b/csharp/ql/test/library-tests/dataflow/tuples/PrintAst.expected @@ -206,11 +206,11 @@ Tuples.cs: # 57| 4: [BreakStmt] break; # 58| 5: [CaseStmt] case ...: # 58| 0: [TupleExpr] (..., ...) -# 58| 0: [LocalVariableDeclExpr] String a +# 58| 0: [VariablePatternExpr] String a # 58| 1: [TupleExpr] (..., ...) -# 58| 0: [LocalVariableDeclExpr] Int32 b -# 58| 1: [LocalVariableDeclExpr] String c -# 58| 2: [DiscardExpr] _ +# 58| 0: [VariablePatternExpr] Int32 b +# 58| 1: [VariablePatternExpr] String c +# 58| 2: [DiscardPatternExpr] _ # 59| 6: [ExprStmt] ...; # 59| 0: [MethodCall] call to method Sink # 59| 0: [LocalVariableAccess] access to local variable a @@ -238,8 +238,8 @@ Tuples.cs: # 68| 2: [PositionalPatternExpr] ( ... ) # 68| 0: [ConstantPatternExpr,StringLiteral] "taint source" # 68| 1: [TupleExpr] (..., ...) -# 68| 0: [LocalVariableDeclExpr] Int32 b -# 68| 1: [LocalVariableDeclExpr] String c +# 68| 0: [VariablePatternExpr] Int32 b +# 68| 1: [VariablePatternExpr] String c # 68| 2: [DiscardPatternExpr] _ # 69| 1: [ExprStmt] ...; # 69| 0: [MethodCall] call to method Sink @@ -266,11 +266,11 @@ Tuples.cs: # 77| 0: [IsExpr] ... is ... # 77| 0: [LocalVariableAccess] access to local variable x # 77| 1: [TupleExpr] (..., ...) -# 77| 0: [LocalVariableDeclExpr] String p +# 77| 0: [VariablePatternExpr] String p # 77| 1: [TupleExpr] (..., ...) -# 77| 0: [LocalVariableDeclExpr] Int32 q -# 77| 1: [LocalVariableDeclExpr] String r -# 77| 2: [DiscardExpr] _ +# 77| 0: [VariablePatternExpr] Int32 q +# 77| 1: [VariablePatternExpr] String r +# 77| 2: [DiscardPatternExpr] _ # 78| 1: [BlockStmt] {...} # 79| 0: [ExprStmt] ...; # 79| 0: [MethodCall] call to method Sink @@ -339,8 +339,8 @@ Tuples.cs: # 96| 0: [LocalVariableAccess] access to local variable r # 98| 0: [CaseStmt] case ...: # 98| 0: [TupleExpr] (..., ...) -# 98| 0: [LocalVariableDeclExpr] String x -# 98| 1: [LocalVariableDeclExpr] Int32 y +# 98| 0: [VariablePatternExpr] String x +# 98| 1: [VariablePatternExpr] Int32 y # 99| 1: [ExprStmt] ...; # 99| 0: [MethodCall] call to method Sink # 99| 0: [LocalVariableAccess] access to local variable x From 8a62778e925eb09cc25f828371fe58e6248680e9 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 22 Dec 2021 13:05:31 +0100 Subject: [PATCH 4/5] C#: Extract `out/ref` information in `this(...)` constructor calls --- .../Semmle.Extraction.CSharp/Entities/Constructor.cs | 6 +----- csharp/ql/test/library-tests/csharp7.3/PrintAst.expected | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs index 525ba96164ac..272681dbbef3 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs @@ -113,11 +113,7 @@ protected override void ExtractInitializers(TextWriter trapFile) trapFile.expr_call(init, target); - var child = 0; - foreach (var arg in initializer.ArgumentList.Arguments) - { - Expression.Create(Context, arg.Expression, init, child++); - } + init.PopulateArguments(trapFile, initializer.ArgumentList, 0); } private ConstructorDeclarationSyntax? Syntax diff --git a/csharp/ql/test/library-tests/csharp7.3/PrintAst.expected b/csharp/ql/test/library-tests/csharp7.3/PrintAst.expected index 362dd350675b..75033e1818c8 100644 --- a/csharp/ql/test/library-tests/csharp7.3/PrintAst.expected +++ b/csharp/ql/test/library-tests/csharp7.3/PrintAst.expected @@ -102,7 +102,7 @@ csharp73.cs: # 46| 1: [IntLiteral] 5 # 49| 5: [InstanceConstructor] ExpressionVariables # 49| 3: [ConstructorInitializer] call to constructor ExpressionVariables -# 49| 0: [LocalVariableDeclExpr] Int32 x +# 49| 0: [LocalVariableAccess,LocalVariableDeclExpr] Int32 x # 50| 4: [BlockStmt] {...} # 51| 0: [ExprStmt] ...; # 51| 0: [MethodCall] call to method WriteLine From a1bbe585164646fa965af61ee094c7e163566900 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 4 Jan 2022 12:18:01 +0100 Subject: [PATCH 5/5] C#: More uses of `PopulateArguments` --- .../Semmle.Extraction.CSharp/Entities/Expression.cs | 8 +++++++- .../Entities/Expressions/Initializer.cs | 7 +------ .../Entities/Expressions/Tuple.cs | 6 +----- .../ql/test/library-tests/arguments/argumentType.expected | 8 ++++++++ 4 files changed, 17 insertions(+), 12 deletions(-) diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs index 123bc315bfa3..5c3a48c05aee 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs @@ -4,6 +4,7 @@ using Semmle.Extraction.CSharp.Entities.Expressions; using Semmle.Extraction.Kinds; using System; +using System.Collections.Generic; using System.IO; using System.Linq; @@ -324,7 +325,12 @@ public void MakeConditional(TextWriter trapFile) public void PopulateArguments(TextWriter trapFile, BaseArgumentListSyntax args, int child) { - foreach (var arg in args.Arguments) + PopulateArguments(trapFile, args.Arguments, child); + } + + public void PopulateArguments(TextWriter trapFile, IEnumerable args, int child) + { + foreach (var arg in args) PopulateArgument(trapFile, arg, child++); } diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Initializer.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Initializer.cs index 29afbce90167..7124929a37cf 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Initializer.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Initializer.cs @@ -105,12 +105,7 @@ protected override void PopulateExpression(TextWriter trapFile) if (assignment.Left is ImplicitElementAccessSyntax iea) { // An array/indexer initializer of the form `[...] = ...` - - var indexChild = 0; - foreach (var arg in iea.ArgumentList.Arguments) - { - Expression.Create(Context, arg.Expression, access, indexChild++); - } + access.PopulateArguments(trapFile, iea.ArgumentList.Arguments, 0); } } else diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Tuple.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Tuple.cs index 726f8f540d73..3e24cce632e5 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Tuple.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expressions/Tuple.cs @@ -15,11 +15,7 @@ private Tuple(ExpressionNodeInfo info) : base(info.SetKind(ExprKind.TUPLE)) protected override void PopulateExpression(TextWriter trapFile) { - var child = 0; - foreach (var argument in Syntax.Arguments.Select(a => a.Expression)) - { - Expression.Create(Context, argument, this, child++); - } + PopulateArguments(trapFile, Syntax.Arguments, 0); } } } diff --git a/csharp/ql/test/library-tests/arguments/argumentType.expected b/csharp/ql/test/library-tests/arguments/argumentType.expected index e2cd7fc4a514..aae04e46581c 100644 --- a/csharp/ql/test/library-tests/arguments/argumentType.expected +++ b/csharp/ql/test/library-tests/arguments/argumentType.expected @@ -24,13 +24,21 @@ | arguments.cs:45:35:45:38 | null | 0 | | arguments.cs:55:21:55:21 | 1 | 0 | | arguments.cs:55:24:55:24 | 2 | 0 | +| arguments.cs:56:10:56:13 | access to property Prop | 0 | +| arguments.cs:56:16:56:25 | access to indexer | 0 | | arguments.cs:56:21:56:21 | 3 | 0 | | arguments.cs:56:24:56:24 | 4 | 0 | +| arguments.cs:56:31:56:31 | 5 | 0 | +| arguments.cs:56:34:56:34 | 6 | 0 | | arguments.cs:59:14:59:14 | 8 | 0 | | arguments.cs:59:17:59:17 | 9 | 0 | | arguments.cs:60:14:60:15 | 10 | 0 | | arguments.cs:60:14:60:15 | 10 | 0 | | arguments.cs:60:18:60:19 | 11 | 0 | | arguments.cs:60:18:60:19 | 11 | 0 | +| arguments.cs:61:22:61:23 | 13 | 0 | +| arguments.cs:61:26:61:27 | 14 | 0 | +| arguments.cs:62:10:62:13 | access to property Prop | 0 | +| arguments.cs:62:16:62:27 | access to indexer | 0 | | arguments.cs:62:21:62:22 | 15 | 0 | | arguments.cs:62:25:62:26 | 16 | 0 |