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/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 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/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] 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/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() 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) } } 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 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()