From 2361476966d467a0fe282bafe315e715b8cc8cf5 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 20 May 2021 10:51:26 +0200 Subject: [PATCH 1/6] C#: Improve join-order in `SplitImpl::hasSuccessor` Joining on `succ` first gets rid of bad join-orders like ``` Tuple counts for Splitting::SplitImpl::hasSuccessor_dispred#ffff/4@i4#f49ebw: 59306 ~2% {3} r1 = JOIN Splitting::SplitImpl::appliesTo#ff#prev_delta WITH Splitting::Cached::TAssertionSplit#ffff_30#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.0 'this', Lhs.1 'pred' 454395 ~0% {3} r2 = JOIN r1 WITH Splitting::AssertionSplitting::getAnAssertionDescendant#ff ON FIRST 1 OUTPUT Lhs.2 'pred', Rhs.1 'succ', Lhs.1 'this' 12157 ~0% {4} r3 = JOIN r2 WITH ControlFlowGraphImpl::succ#fff ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.0 'pred', Lhs.1 'succ', Rhs.2 'c' 0 ~0% {4} r4 = JOIN Splitting::LoopSplitting::LoopUnrollingSplitImpl::appliesToPredecessor_dispred#fff#prev_delta WITH Splitting::Cached::TLoopSplit#ff_10#join_rhs ON FIRST 1 OUTPUT Lhs.1 'pred', Lhs.2 'c', Rhs.1, Rhs.0 0 ~0% {5} r5 = JOIN r4 WITH ControlFlowGraphImpl::succ#fff_021#join_rhs ON FIRST 2 OUTPUT Lhs.2, Lhs.3 'this', Lhs.0 'pred', Lhs.1 'c', Rhs.2 'succ' 0 ~0% {5} r6 = r5 AND NOT Splitting::SplitImpl::hasSuccessor_dispred#ffff#antijoin_rhs#2(Lhs.2 'pred', Lhs.4 'succ', Lhs.3 'c', Lhs.0) 0 ~0% {4} r7 = SCAN r6 OUTPUT In.1 'this', In.2 'pred', In.4 'succ', In.3 'c' 12157 ~0% {4} r8 = r3 UNION r7 0 ~0% {3} r9 = SCAN Splitting::BooleanSplitting::BooleanSplitImpl::appliesToBlock_dispred#fff#prev_delta OUTPUT In.1, In.0 'this', In.2 0 ~0% {4} r10 = JOIN r9 WITH project#PreBasicBlocks::PreBasicBlock::getElement_dispred#fff ON FIRST 1 OUTPUT Rhs.1 'pred', Lhs.1 'this', Lhs.0, Lhs.2 0 ~0% {6} r11 = JOIN r10 WITH ControlFlowGraphImpl::succ#fff ON FIRST 1 OUTPUT Lhs.1 'this', Lhs.2, Lhs.3, Lhs.0 'pred', Rhs.1, Rhs.2 'c' 0 ~0% {6} r12 = r11 AND NOT PreBasicBlocks::PreBasicBlock::getLastElement_dispred#ff(Lhs.1, Lhs.3 'pred') 0 ~0% {4} r13 = SCAN r12 OUTPUT In.0 'this', In.3 'pred', In.4 'succ', In.5 'c' 35244 ~1% {3} r14 = JOIN Splitting::SplitImpl::appliesTo#ff#prev_delta WITH Splitting::Cached::TInitializerSplit#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.0 'this', Lhs.1 'pred' 24640675 ~6% {3} r15 = JOIN r14 WITH Splitting::InitializerSplitting::constructorInitializes#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this', Lhs.2 'pred' 24640675 ~21147% {3} r16 = JOIN r15 WITH Splitting::InitializerSplitting::InitializedInstanceMember::getInitializer_dispred#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this', Lhs.2 'pred' 470227342920 ~481608% {3} r17 = JOIN r16 WITH Splitting::InitializerSplitting::InitializedInstanceMember::getAnInitializerDescendant#ff ON FIRST 1 OUTPUT Lhs.2 'pred', Rhs.1 'succ', Lhs.1 'this' 24560447 ~66468% {4} r18 = JOIN r17 WITH ControlFlowGraphImpl::succ#fff ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.0 'pred', Lhs.1 'succ', Rhs.2 'c' 24560447 ~66468% {4} r19 = r13 UNION r18 24572604 ~48162% {4} r20 = r8 UNION r19 0 ~0% {3} r21 = JOIN r9 WITH project#PreBasicBlocks::PreBasicBlock::getElement_dispred#fff ON FIRST 1 OUTPUT Rhs.1 'pred', Lhs.2, Lhs.1 'this' 0 ~0% {4} r22 = JOIN r21 WITH ControlFlowGraphImpl::succ#fff_021#join_rhs ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.0 'pred', Rhs.2 'succ', Rhs.1 0 ~0% {5} r23 = JOIN r22 WITH Splitting::Cached::TBooleanSplit#fff_20#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.2 'succ', Lhs.0 'this', Lhs.1 'pred', Lhs.3 'c' 0 ~0% {4} r24 = JOIN r23 WITH Splitting::BooleanSplitting::SsaBooleanSplitSubKind::canReachCorrelatedCondition#ff ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.3 'pred', Lhs.1 'succ', Lhs.4 'c' 0 ~0% {2} r25 = SCAN Splitting::FinallySplitting::FinallySplitImpl::appliesToPredecessor_dispred#ff#prev_delta OUTPUT In.1 'pred', In.0 'this' 0 ~0% {4} r26 = JOIN r25 WITH ControlFlowGraphImpl::succ#fff ON FIRST 1 OUTPUT Lhs.1 'this', Lhs.0 'pred', Rhs.1, Rhs.2 'c' 0 ~0% {4} r27 = r26 AND NOT Splitting::FinallySplitting::FinallyControlFlowElement::isEntryNode_dispred#f(Lhs.2 'succ') 0 ~0% {5} r28 = JOIN r27 WITH Splitting::Cached::TFinallySplit#fff_21#join_rhs ON FIRST 1 OUTPUT Lhs.2 'succ', Lhs.0 'this', Lhs.1 'pred', Lhs.3 'c', Rhs.1 0 ~0% {6} r29 = JOIN r28 WITH ControlFlowGraphImpl::Statements::TryStmtTree::getAFinallyDescendant#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this', Lhs.2 'pred', Lhs.0 'succ', Lhs.3 'c', Lhs.4 0 ~0% {7} r30 = JOIN r29 WITH ControlFlowGraphImpl::Statements::TryStmtTree::nestLevel_dispred#ff ON FIRST 1 OUTPUT Lhs.1 'this', Lhs.2 'pred', Lhs.3 'succ', Lhs.4 'c', Lhs.5, Lhs.0, Rhs.1 0 ~0% {7} r31 = SELECT r30 ON In.6 >= In.4 0 ~0% {4} r32 = SCAN r31 OUTPUT In.0 'this', In.1 'pred', In.2 'succ', In.3 'c' 0 ~0% {4} r33 = r24 UNION r32 0 ~0% {4} r34 = JOIN r25 WITH ControlFlowGraphImpl::succ#fff ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this', Lhs.0 'pred', Rhs.2 'c' 0 ~0% {4} r35 = JOIN r34 WITH Splitting::FinallySplitting::FinallyControlFlowElement::isEntryNode_dispred#f ON FIRST 1 OUTPUT Lhs.1 'this', Lhs.2 'pred', Lhs.0 'succ', Lhs.3 'c' 0 ~0% {5} r36 = JOIN r35 WITH Splitting::Cached::TFinallySplit#fff_21#join_rhs ON FIRST 1 OUTPUT Lhs.2 'succ', Lhs.0 'this', Lhs.1 'pred', Lhs.3 'c', Rhs.1 0 ~0% {6} r37 = JOIN r36 WITH ControlFlowGraphImpl::Statements::TryStmtTree::getAFinallyDescendant#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this', Lhs.2 'pred', Lhs.0 'succ', Lhs.3 'c', Lhs.4 0 ~0% {7} r38 = JOIN r37 WITH ControlFlowGraphImpl::Statements::TryStmtTree::nestLevel_dispred#ff ON FIRST 1 OUTPUT Lhs.1 'this', Lhs.2 'pred', Lhs.3 'succ', Lhs.4 'c', Lhs.5, Lhs.0, Rhs.1 0 ~0% {7} r39 = SELECT r38 ON In.6 > In.4 0 ~0% {4} r40 = SCAN r39 OUTPUT In.0 'this', In.1 'pred', In.2 'succ', In.3 'c' 0 ~0% {3} r41 = SCAN Splitting::ExceptionHandlerSplitting::ExceptionHandlerSplitImpl::appliesToPredecessor_dispred#fff#prev_delta OUTPUT In.1 'pred', In.2 'c', In.0 'this' 0 ~0% {4} r42 = JOIN r41 WITH ControlFlowGraphImpl::last#fff_120#join_rhs ON FIRST 2 OUTPUT Rhs.2, Lhs.2 'this', Lhs.0 'pred', Lhs.1 'c' 0 ~0% {5} r43 = JOIN r42 WITH Stmt::TryStmt::getCatchClause_dispred#fff_201#join_rhs ON FIRST 1 OUTPUT Rhs.1, Rhs.2, Lhs.1 'this', Lhs.2 'pred', Lhs.3 'c' 0 ~0% {4} r44 = JOIN r43 WITH Stmt::TryStmt::getCatchClause_dispred#fff ON FIRST 2 OUTPUT Rhs.2, Lhs.2 'this', Lhs.3 'pred', Lhs.4 'c' 0 ~0% {5} r45 = JOIN r44 WITH Stmt::CatchClause::isLast_dispred#f ON FIRST 1 OUTPUT Lhs.0, 1, Lhs.1 'this', Lhs.2 'pred', Lhs.3 'c' 0 ~0% {3} r46 = JOIN r45 WITH catch_type_02#join_rhs ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.3 'pred', Lhs.4 'c' 0 {3} r47 = MATERIALIZE r46 AS unknown 0 ~0% {3} r48 = Splitting::ExceptionHandlerSplitting::ExceptionHandlerSplitImpl::appliesToPredecessor_dispred#fff#prev_delta AND NOT r47(Lhs.0 'this', Lhs.1 'pred', Lhs.2 'c') 0 ~0% {3} r49 = SCAN r48 OUTPUT In.1 'pred', In.2 'c', In.0 'this' 0 ~0% {4} r50 = JOIN r49 WITH ControlFlowGraphImpl::succ#fff_021#join_rhs ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.0 'pred', Lhs.1 'c', Rhs.2 'succ' 0 ~0% {4} r51 = JOIN r49 WITH ControlFlowGraphImpl::succ#fff_021#join_rhs ON FIRST 2 OUTPUT Rhs.2 'succ', Lhs.2 'this', Lhs.0 'pred', Lhs.1 'c' 0 ~0% {5} r52 = JOIN r51 WITH ControlFlowGraphImpl::ControlFlowTree::first_dispred#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, Lhs.1 'this', Lhs.2 'pred', Lhs.3 'c', Lhs.0 'succ' 0 ~0% {6} r53 = JOIN r52 WITH Stmt::CatchClause::getBlock_dispred#ff_10#join_rhs ON FIRST 1 OUTPUT Rhs.1, 1, Lhs.1 'this', Lhs.2 'pred', Lhs.3 'c', Lhs.4 'succ' 0 ~0% {4} r54 = JOIN r53 WITH catch_type_02#join_rhs ON FIRST 2 OUTPUT Lhs.2 'this', Lhs.3 'pred', Lhs.4 'c', Lhs.5 'succ' 0 {4} r55 = MATERIALIZE r54 AS unknown 0 ~0% {4} r56 = r50 AND NOT r55(Lhs.0 'this', Lhs.1 'pred', Lhs.2 'c', Lhs.3 'succ') 0 ~0% {4} r57 = r56 AND NOT ControlFlowGraphImpl::Statements::StandardStmt::getChildElement0_dispred#fff#antijoin_rhs#2(Lhs.3 'succ') 0 ~0% {4} r58 = SCAN r57 OUTPUT In.0 'this', In.1 'pred', In.3 'succ', In.2 'c' 0 ~0% {4} r59 = r40 UNION r58 0 ~0% {4} r60 = r33 UNION r59 24572604 ~48162% {4} r61 = r20 UNION r60 24572604 ~48162% {4} r62 = r61 AND NOT Splitting::SplitImpl::hasSuccessor_dispred#ffff#prev(Lhs.0 'this', Lhs.1 'pred', Lhs.2 'succ', Lhs.3 'c') return r62 ``` --- .../csharp/controlflow/internal/Splitting.qll | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll index 8826ee7c1be7..8ca77a4775e2 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll @@ -209,6 +209,13 @@ abstract class SplitImpl extends Split { or exists(ControlFlowElement pred | this.appliesTo(pred) | this.hasSuccessor(pred, cfe, _)) } + + /** The `succ` relation restricted to predecessors `pred` that this split applies to. */ + pragma[noinline] + final predicate appliesSucc(ControlFlowElement pred, ControlFlowElement succ, Completion c) { + this.appliesTo(pred) and + succ(pred, succ, c) + } } module InitializerSplitting { @@ -382,8 +389,7 @@ module InitializerSplitting { } override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) { - this.appliesTo(pred) and - succ(pred, succ, c) and + this.appliesSucc(pred, succ, c) and succ = any(InitializedInstanceMember m | constructorInitializes(this.getConstructor(), m.getInitializer()) @@ -620,8 +626,7 @@ module AssertionSplitting { } override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) { - this.appliesTo(pred) and - succ(pred, succ, c) and + this.appliesSucc(pred, succ, c) and succ = getAnAssertionDescendant(a) } } @@ -858,8 +863,7 @@ module FinallySplitting { } override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) { - this.appliesToPredecessor(pred) and - succ(pred, succ, c) and + this.appliesSucc(pred, succ, c) and succ = any(FinallyControlFlowElement fcfe | if fcfe.isEntryNode() @@ -1037,7 +1041,7 @@ module ExceptionHandlerSplitting { override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) { this.appliesToPredecessor(pred, c) and - succ(pred, succ, c) and + this.appliesSucc(pred, succ, c) and not first(any(SpecificCatchClause scc).getBlock(), succ) and not succ instanceof GeneralCatchClause and not exists(TryStmt ts, SpecificCatchClause scc, int last | @@ -1298,7 +1302,7 @@ module BooleanSplitting { override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) { exists(PreBasicBlock bb, Completion c0 | this.appliesToBlock(bb, c0) | pred = bb.getAnElement() and - succ(pred, succ, c) and + this.appliesSucc(pred, succ, c) and ( pred = bb.getLastElement() implies @@ -1489,7 +1493,7 @@ module LoopSplitting { override predicate hasSuccessor(ControlFlowElement pred, ControlFlowElement succ, Completion c) { this.appliesToPredecessor(pred, c) and - succ(pred, succ, c) and + this.appliesSucc(pred, succ, c) and not loop.stop(pred, succ, c) } } From 5102fcd5f3f22827a0a166fd8811f2af8aae3175 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Fri, 21 May 2021 09:44:21 +0200 Subject: [PATCH 2/6] C#: Rewrite predicates from using `forall` to using `unique` This avoids generation of expensive anti-join predicates with Cartesian products. --- .../semmle/code/csharp/controlflow/Guards.qll | 30 ++++++++++------ .../internal/pressa/SsaImplSpecific.qll | 36 +++++++++++++++---- 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll index 8a721da5293d..5a402717401e 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll @@ -1466,8 +1466,10 @@ module Internal { PreSsa::Definition def, AssignableRead read ) { read = def.getAFirstRead() and - not exists(AssignableRead other | PreSsa::adjacentReadPairSameVar(other, read) | - other != read + ( + not PreSsa::adjacentReadPairSameVar(_, read) + or + read = unique(AssignableRead read0 | PreSsa::adjacentReadPairSameVar(read0, read)) ) } @@ -1651,10 +1653,14 @@ module Internal { AssignableRead read1, AssignableRead read2 ) { PreSsa::adjacentReadPairSameVar(read1, read2) and - not exists(AssignableRead other | - PreSsa::adjacentReadPairSameVar(other, read2) and - other != read1 and - other != read2 + ( + read1 = read2 and + read1 = unique(AssignableRead other | PreSsa::adjacentReadPairSameVar(other, read2)) + or + read1 = + unique(AssignableRead other | + PreSsa::adjacentReadPairSameVar(other, read2) and other != read2 + ) ) } @@ -1887,10 +1893,14 @@ module Internal { Ssa::Definition def, ControlFlow::Node cfn1, ControlFlow::Node cfn2 ) { SsaImpl::adjacentReadPairSameVar(def, cfn1, cfn2) and - not exists(ControlFlow::Node other | - SsaImpl::adjacentReadPairSameVar(def, other, cfn2) and - other != cfn1 and - other != cfn2 + ( + cfn1 = cfn2 and + cfn1 = unique(ControlFlow::Node other | SsaImpl::adjacentReadPairSameVar(def, other, cfn2)) + or + cfn1 = + unique(ControlFlow::Node other | + SsaImpl::adjacentReadPairSameVar(def, other, cfn2) and other != cfn2 + ) ) } diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/internal/pressa/SsaImplSpecific.qll b/csharp/ql/src/semmle/code/csharp/controlflow/internal/pressa/SsaImplSpecific.qll index 610e808a6403..ad64c38973ac 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/pressa/SsaImplSpecific.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/pressa/SsaImplSpecific.qll @@ -15,13 +15,37 @@ class ExitBasicBlock extends BasicBlock { ExitBasicBlock() { scopeLast(_, this.getLastElement(), _) } } -pragma[noinline] +/** Holds if `a` is assigned in non-constructor callable `c`. */ +pragma[nomagic] +private predicate assignableDefinition(Assignable a, Callable c) { + exists(AssignableDefinition def | def.getTarget() = a | + c = def.getEnclosingCallable() and + not c instanceof Constructor + ) +} + +/** Holds if `a` is accessed in callable `c`. */ +pragma[nomagic] +private predicate assignableAccess(Assignable a, Callable c) { + exists(AssignableAccess aa | aa.getTarget() = a | c = aa.getEnclosingCallable()) +} + +pragma[nomagic] private predicate assignableNoCapturing(Assignable a, Callable c) { - exists(AssignableAccess aa | aa.getTarget() = a | c = aa.getEnclosingCallable()) and - forall(AssignableDefinition def | def.getTarget() = a | - c = def.getEnclosingCallable() + assignableAccess(a, c) and + /* + * The code below is equivalent to + * ```ql + * not exists(Callable other | assignableDefinition(a, other) | other != c) + * ``` + * but it avoids a Cartesian product in the compiler generated antijoin + * predicate. + */ + + ( + not assignableDefinition(a, _) or - def.getEnclosingCallable() instanceof Constructor + c = unique(Callable c0 | assignableDefinition(a, c0) | c0) ) } @@ -41,7 +65,7 @@ class SourceVariable extends Assignable { ( this instanceof LocalScopeVariable or - this instanceof Field + this = any(Field f | not f.isVolatile()) or this = any(TrivialProperty tp | not tp.isOverridableOrImplementable()) ) and From 3162e12082b731e913d1d11d933113014c822b84 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Thu, 20 May 2021 10:48:14 +0200 Subject: [PATCH 3/6] C#: Redefine `ControlFlowElement::getAssembly` --- .../semmle/code/csharp/controlflow/ControlFlowElement.qll | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll b/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll index dcae798813ae..7601b83f6b81 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowElement.qll @@ -2,6 +2,7 @@ import csharp private import semmle.code.csharp.ExprOrStmtParent +private import semmle.code.csharp.commons.Compilation private import ControlFlow private import ControlFlow::BasicBlocks private import SuccessorTypes @@ -22,7 +23,12 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element { Callable getEnclosingCallable() { none() } /** Gets the assembly that this element was compiled into. */ - Assembly getAssembly() { result = this.getEnclosingCallable().getDeclaringType().getALocation() } + Assembly getAssembly() { + exists(Compilation c | + c.getAFileCompiled() = this.getFile() and + result = c.getOutputAssembly() + ) + } /** * Gets a control flow node for this element. That is, a node in the From 088a1a970784626991bad38295b62e5a33f06336 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Fri, 21 May 2021 08:55:33 +0200 Subject: [PATCH 4/6] C#: Simplify `TriedControlFlowElement::getAThrownException()` --- .../controlflow/internal/Completion.qll | 49 +------------------ 1 file changed, 1 insertion(+), 48 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll index 089339ba1b8e..bda14e0b4aec 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/Completion.qll @@ -293,18 +293,6 @@ private class Overflowable extends UnaryOperation { } } -private class CoreLib extends Assembly { - CoreLib() { this = any(SystemExceptionClass c).getALocation() } -} - -/** - * Holds if assembly `a` was definitely compiled with core library `core`. - */ -pragma[noinline] -private predicate assemblyCompiledWithCoreLib(Assembly a, CoreLib core) { - a.getAnAttribute().getType().getBaseClass*().(SystemAttributeClass).getALocation() = core -} - /** A control flow element that is inside a `try` block. */ private class TriedControlFlowElement extends ControlFlowElement { TryStmt try; @@ -317,7 +305,7 @@ private class TriedControlFlowElement extends ControlFlowElement { /** * Gets an exception class that is potentially thrown by this element, if any. */ - private Class getAThrownException0() { + Class getAThrownException() { this instanceof Overflowable and result instanceof SystemOverflowExceptionClass or @@ -376,41 +364,6 @@ private class TriedControlFlowElement extends ControlFlowElement { this instanceof DynamicExpr and result instanceof SystemExceptionClass } - - private CoreLib getCoreLibFromACatchClause() { - exists(SpecificCatchClause scc | scc = try.getACatchClause() | - result = scc.getCaughtExceptionType().getBaseClass*().(SystemExceptionClass).getALocation() - ) - } - - private CoreLib getCoreLib() { - result = this.getCoreLibFromACatchClause() - or - not exists(this.getCoreLibFromACatchClause()) and - assemblyCompiledWithCoreLib(this.getAssembly(), result) - } - - pragma[noinline] - private Class getAThrownExceptionFromPlausibleCoreLib(string name) { - result = this.getAThrownException0() and - name = result.getQualifiedName() and - ( - not exists(this.getCoreLib()) - or - this.getCoreLib() = result.getALocation() - ) - } - - Class getAThrownException() { - exists(string name | result = this.getAThrownExceptionFromPlausibleCoreLib(name) | - result = - min(Class c | - c = this.getAThrownExceptionFromPlausibleCoreLib(name) - | - c order by c.getLocation().(Assembly).getFullName() - ) - ) - } } pragma[nomagic] From 0d14b9413de210cabd06932dcdc5073e9b9b9c41 Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Fri, 21 May 2021 09:44:41 +0200 Subject: [PATCH 5/6] C#: Avoid recomputing `ControlFlowTree::Range` outside the CFG construction stage --- .../ql/src/semmle/code/csharp/controlflow/BasicBlocks.qll | 2 +- .../csharp/controlflow/internal/ControlFlowGraphImpl.qll | 6 +++--- .../internal/rangeanalysis/ModulusAnalysisSpecific.qll | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/BasicBlocks.qll b/csharp/ql/src/semmle/code/csharp/controlflow/BasicBlocks.qll index 8b20ccb3c22f..b4448a713802 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/BasicBlocks.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/BasicBlocks.qll @@ -403,7 +403,7 @@ private module JoinBlockPredecessors { private import semmle.code.csharp.controlflow.internal.ControlFlowGraphImpl int getId(JoinBlockPredecessor jbp) { - exists(ControlFlowTree::Range t | ControlFlowTree::idOf(t, result) | + exists(ControlFlowTree::Range_ t | ControlFlowTree::idOf(t, result) | t = jbp.getFirstNode().getElement() or t = jbp.(EntryBasicBlock).getCallable() diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll b/csharp/ql/src/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll index 6a5944db3a2c..63f6943a29ca 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll @@ -77,7 +77,7 @@ class CfgScope extends Element, @top_level_exprorstmt_parent { } module ControlFlowTree { - private class Range_ = @callable or @control_flow_element; + class Range_ = @callable or @control_flow_element; class Range extends Element, Range_ { Range() { this = getAChild*(any(CfgScope scope)) } @@ -88,9 +88,9 @@ module ControlFlowTree { result = p.(AssignOperation).getExpandedAssignment() } - private predicate id(Range x, Range y) { x = y } + private predicate id(Range_ x, Range_ y) { x = y } - predicate idOf(Range x, int y) = equivalenceRelation(id/2)(x, y) + predicate idOf(Range_ x, int y) = equivalenceRelation(id/2)(x, y) } abstract private class ControlFlowTree extends ControlFlowTree::Range { diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll b/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll index f71b0d0ffcd2..030020ede632 100644 --- a/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll +++ b/csharp/ql/src/semmle/code/csharp/dataflow/internal/rangeanalysis/ModulusAnalysisSpecific.qll @@ -49,7 +49,7 @@ module Private { } int getId(PhiInputEdgeBlock bb) { - exists(CfgImpl::ControlFlowTree::Range t | CfgImpl::ControlFlowTree::idOf(t, result) | + exists(CfgImpl::ControlFlowTree::Range_ t | CfgImpl::ControlFlowTree::idOf(t, result) | t = bb.getFirstNode().getElement() or t = bb.(CS::ControlFlow::BasicBlocks::EntryBlock).getCallable() From b55bce46f87075802f76a38d1b926b7b5b2fefba Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 25 May 2021 10:41:58 +0200 Subject: [PATCH 6/6] C#: Restrict non-returning CIL analysis to methods not from source --- .../code/csharp/controlflow/internal/NonReturning.qll | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/internal/NonReturning.qll b/csharp/ql/src/semmle/code/csharp/controlflow/internal/NonReturning.qll index 6d075d031f5d..fe9f1148a6d8 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/internal/NonReturning.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/internal/NonReturning.qll @@ -39,8 +39,10 @@ private class ThrowingCall extends NonReturningCall { or this.(FailingAssertion).getAssertionFailure().isException(c.getExceptionClass()) or - exists(CIL::Method m, CIL::Type ex | - this.getTarget().matchesHandle(m) and + exists(Callable target, CIL::Method m, CIL::Type ex | + target = this.getTarget() and + not target.hasBody() and + target.matchesHandle(m) and alwaysThrowsException(m, ex) and c.getExceptionClass().matchesHandle(ex) and not m.isVirtual()