diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll index 414cfc2d50ae..40ec3722edd2 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll @@ -142,6 +142,7 @@ private module GuardsInput implements } } + pragma[nomagic] predicate equalityTest(Expr eqtest, Expr left, Expr right, boolean polarity) { exists(ComparisonTest ct | ct.getExpr() = eqtest and @@ -410,6 +411,22 @@ private predicate typePattern(PatternMatch pm, TypePatternExpr tpe, Type t) { t = pm.getExpr().getType() } +pragma[nomagic] +private predicate dereferenceableExpr(Expr e, boolean isNullableType) { + exists(Type t | t = e.getType() | + t instanceof NullableType and + isNullableType = true + or + t instanceof RefType and + isNullableType = false + ) + or + exists(Expr parent | + dereferenceableExpr(parent, isNullableType) and + e = getNullEquivParent(parent) + ) +} + /** * An expression that evaluates to a value that can be dereferenced. That is, * an expression that may evaluate to `null`. @@ -418,21 +435,12 @@ class DereferenceableExpr extends Expr { private boolean isNullableType; DereferenceableExpr() { - exists(Expr e, Type t | - // There is currently a bug in the extractor: the type of `x?.Length` is - // incorrectly `int`, while it should have been `int?`. We apply - // `getNullEquivParent()` as a workaround - this = getNullEquivParent*(e) and - t = e.getType() and - not this instanceof SwitchCaseExpr and - not this instanceof PatternExpr - | - t instanceof NullableType and - isNullableType = true - or - t instanceof RefType and - isNullableType = false - ) + // There is currently a bug in the extractor: the type of `x?.Length` is + // incorrectly `int`, while it should have been `int?`. We apply + // `getNullEquivParent()` as a workaround + dereferenceableExpr(this, isNullableType) and + not this instanceof SwitchCaseExpr and + not this instanceof PatternExpr } /** Holds if this expression has a nullable type `T?`. */ diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll index 6be79a17be25..576d0734ef6d 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll @@ -94,9 +94,19 @@ private Element getAChild(Element p) { result = p.(AssignOperation).getExpandedAssignment() } +pragma[nomagic] +private predicate astNode(Element e) { + e = any(@top_level_exprorstmt_parent p | not p instanceof Attribute) + or + exists(Element parent | + astNode(parent) and + e = getAChild(parent) + ) +} + /** An AST node. */ class AstNode extends Element, TAstNode { - AstNode() { this = getAChild*(any(@top_level_exprorstmt_parent p | not p instanceof Attribute)) } + AstNode() { astNode(this) } int getId() { idOf(this, result) } } diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ControlFlowReachability.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ControlFlowReachability.qll index 8ab7bb0fba43..706893c64ea0 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ControlFlowReachability.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ControlFlowReachability.qll @@ -15,16 +15,47 @@ private class ControlFlowScope extends ControlFlowElement { predicate isNonExact() { exactScope = false } } -private ControlFlowElement getANonExactScopeChild(ControlFlowScope scope) { - scope.isNonExact() and - result = scope +private newtype TControlFlowElementOrBasicBlock = + TControlFlowElement(ControlFlowElement cfe) or + TBasicBlock(ControlFlow::BasicBlock bb) + +class ControlFlowElementOrBasicBlock extends TControlFlowElementOrBasicBlock { + ControlFlowElement asControlFlowElement() { this = TControlFlowElement(result) } + + ControlFlow::BasicBlock asBasicBlock() { this = TBasicBlock(result) } + + string toString() { + result = this.asControlFlowElement().toString() + or + result = this.asBasicBlock().toString() + } + + Location getLocation() { + result = this.asControlFlowElement().getLocation() + or + result = this.asBasicBlock().getLocation() + } +} + +private predicate isBasicBlock(ControlFlowElementOrBasicBlock c) { c instanceof TBasicBlock } + +private predicate isNonExactScope(ControlFlowElementOrBasicBlock c) { + c.asControlFlowElement().(ControlFlowScope).isNonExact() +} + +private predicate step(ControlFlowElementOrBasicBlock pred, ControlFlowElementOrBasicBlock succ) { + pred.asBasicBlock().getANode().getAstNode() = succ.asControlFlowElement() or - result = getANonExactScopeChild(scope).getAChild() + pred.asControlFlowElement() = succ.asControlFlowElement().getAChild() } +private predicate basicBlockInNonExactScope( + ControlFlowElementOrBasicBlock bb, ControlFlowElementOrBasicBlock scope +) = doublyBoundedFastTC(step/2, isBasicBlock/1, isNonExactScope/1)(bb, scope) + pragma[noinline] private ControlFlow::BasicBlock getABasicBlockInScope(ControlFlowScope scope, boolean exactScope) { - result.getANode().getAstNode() = getANonExactScopeChild(scope) and + basicBlockInNonExactScope(TBasicBlock(result), TControlFlowElement(scope)) and exactScope = false or scope.isExact() and diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll index 1b3de63495f0..efed08460538 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll @@ -7,6 +7,7 @@ private import FlowSummaryImpl as FlowSummaryImpl private import semmle.code.csharp.dataflow.FlowSummary as FlowSummary private import semmle.code.csharp.dataflow.internal.ExternalFlow private import semmle.code.csharp.Conversion +private import semmle.code.csharp.exprs.internal.Expr private import semmle.code.csharp.dataflow.internal.SsaImpl as SsaImpl private import semmle.code.csharp.ExprOrStmtParent private import semmle.code.csharp.Unification @@ -2377,6 +2378,16 @@ predicate storeStep(Node node1, ContentSet c, Node node2) { storeStepDelegateCall(node1, c, node2) } +pragma[nomagic] +private predicate isAssignExprLValueDescendant(Expr e) { + e = any(AssignExpr ae).getLValue() + or + exists(Expr parent | + isAssignExprLValueDescendant(parent) and + e = parent.getAChildExpr() + ) +} + private class ReadStepConfiguration extends ControlFlowReachabilityConfiguration { ReadStepConfiguration() { this = "ReadStepConfiguration" } @@ -2432,7 +2443,7 @@ private class ReadStepConfiguration extends ControlFlowReachabilityConfiguration scope = any(AssignExpr ae | ae = defTo.(AssignableDefinitions::TupleAssignmentDefinition).getAssignment() and - e = ae.getLValue().getAChildExpr*().(TupleExpr) and + isAssignExprLValueDescendant(e.(TupleExpr)) and exactScope = false and isSuccessor = true ) @@ -2488,7 +2499,7 @@ private predicate readContentStep(Node node1, Content c, Node node2) { ) or // item = variable in node1 = (..., variable, ...) in a case/is var (..., ...) - te = any(PatternExpr pe).getAChildExpr*() and + isPatternExprDescendant(te) and exists(AssignableDefinitions::LocalVariableDefinition lvd | node2.(AssignableDefinitionNode).getDefinition() = lvd and lvd.getDeclaration() = item and diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ControlFlowReachability.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ControlFlowReachability.qll index 8ab7bb0fba43..706893c64ea0 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ControlFlowReachability.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/ControlFlowReachability.qll @@ -15,16 +15,47 @@ private class ControlFlowScope extends ControlFlowElement { predicate isNonExact() { exactScope = false } } -private ControlFlowElement getANonExactScopeChild(ControlFlowScope scope) { - scope.isNonExact() and - result = scope +private newtype TControlFlowElementOrBasicBlock = + TControlFlowElement(ControlFlowElement cfe) or + TBasicBlock(ControlFlow::BasicBlock bb) + +class ControlFlowElementOrBasicBlock extends TControlFlowElementOrBasicBlock { + ControlFlowElement asControlFlowElement() { this = TControlFlowElement(result) } + + ControlFlow::BasicBlock asBasicBlock() { this = TBasicBlock(result) } + + string toString() { + result = this.asControlFlowElement().toString() + or + result = this.asBasicBlock().toString() + } + + Location getLocation() { + result = this.asControlFlowElement().getLocation() + or + result = this.asBasicBlock().getLocation() + } +} + +private predicate isBasicBlock(ControlFlowElementOrBasicBlock c) { c instanceof TBasicBlock } + +private predicate isNonExactScope(ControlFlowElementOrBasicBlock c) { + c.asControlFlowElement().(ControlFlowScope).isNonExact() +} + +private predicate step(ControlFlowElementOrBasicBlock pred, ControlFlowElementOrBasicBlock succ) { + pred.asBasicBlock().getANode().getAstNode() = succ.asControlFlowElement() or - result = getANonExactScopeChild(scope).getAChild() + pred.asControlFlowElement() = succ.asControlFlowElement().getAChild() } +private predicate basicBlockInNonExactScope( + ControlFlowElementOrBasicBlock bb, ControlFlowElementOrBasicBlock scope +) = doublyBoundedFastTC(step/2, isBasicBlock/1, isNonExactScope/1)(bb, scope) + pragma[noinline] private ControlFlow::BasicBlock getABasicBlockInScope(ControlFlowScope scope, boolean exactScope) { - result.getANode().getAstNode() = getANonExactScopeChild(scope) and + basicBlockInNonExactScope(TBasicBlock(result), TControlFlowElement(scope)) and exactScope = false or scope.isExact() and diff --git a/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll b/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll index ada010652588..42d9320bfa81 100644 --- a/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll +++ b/csharp/ql/lib/semmle/code/csharp/exprs/Expr.qll @@ -21,6 +21,7 @@ import semmle.code.csharp.Type private import semmle.code.csharp.ExprOrStmtParent private import semmle.code.csharp.frameworks.System private import semmle.code.csharp.TypeRef +private import internal.Expr /** * An expression. Either an access (`Access`), a call (`Call`), an object or @@ -64,14 +65,24 @@ class Expr extends ControlFlowElement, @expr { /** Gets the enclosing callable of this expression, if any. */ override Callable getEnclosingCallable() { enclosingCallable(this, result) } + pragma[nomagic] + private predicate isExpandedAssignmentRValueDescendant() { + this = + any(AssignOperation op).getExpandedAssignment().getRValue().getChildExpr(0).getAChildExpr() + or + exists(Expr parent | + parent.isExpandedAssignmentRValueDescendant() and + this = parent.getAChildExpr() + ) + } + /** * Holds if this expression is generated by the compiler and does not appear * explicitly in the source code. */ final predicate isImplicit() { compiler_generated(this) or - this = - any(AssignOperation op).getExpandedAssignment().getRValue().getChildExpr(0).getAChildExpr+() + this.isExpandedAssignmentRValueDescendant() } /** @@ -1133,7 +1144,7 @@ class TupleExpr extends Expr, @tuple_expr { /** Holds if this expression is a tuple construction. */ predicate isConstruction() { not this = getAnAssignOrForeachChild() and - not this = any(PatternExpr pe).getAChildExpr*() + not isPatternExprDescendant(this) } override string getAPrimaryQlClass() { result = "TupleExpr" } diff --git a/csharp/ql/lib/semmle/code/csharp/exprs/internal/Expr.qll b/csharp/ql/lib/semmle/code/csharp/exprs/internal/Expr.qll new file mode 100644 index 000000000000..94dfac721932 --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/exprs/internal/Expr.qll @@ -0,0 +1,11 @@ +private import csharp + +pragma[nomagic] +predicate isPatternExprDescendant(Expr e) { + e instanceof PatternExpr + or + exists(Expr parent | + isPatternExprDescendant(parent) and + e = parent.getAChildExpr() + ) +} diff --git a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql b/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql index d40a28450fca..b980bfba1aea 100644 --- a/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql +++ b/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql @@ -15,23 +15,6 @@ import csharp -/** An expression containing a qualified member access, a method call, or an array access. */ -class DangerousExpression extends Expr { - DangerousExpression() { - exists(Expr e | this = e.getParent*() | - exists(Expr q | q = e.(MemberAccess).getQualifier() | - not q instanceof ThisAccess and - not q instanceof BaseAccess - ) - or - e instanceof MethodCall - or - e instanceof ArrayAccess - ) and - not exists(Expr e | this = e.getParent*() | e.(Call).getTarget().getAParameter().isOutOrRef()) - } -} - /** A use of `&` or `|` on operands of type boolean. */ class NonShortCircuit extends BinaryBitwiseOperation { NonShortCircuit() { @@ -42,10 +25,40 @@ class NonShortCircuit extends BinaryBitwiseOperation { ) and not exists(AssignBitwiseOperation abo | abo.getExpandedAssignment().getRValue() = this) and this.getLeftOperand().getType() instanceof BoolType and - this.getRightOperand().getType() instanceof BoolType and - this.getRightOperand() instanceof DangerousExpression + this.getRightOperand().getType() instanceof BoolType + } + + pragma[nomagic] + private predicate hasRightOperandDescendant(Expr e) { + e = this.getRightOperand() + or + exists(Expr parent | + this.hasRightOperandDescendant(parent) and + e.getParent() = parent + ) + } + + /** + * Holds if this non-short-circuit expression contains a qualified member access, + * a method call, or an array access inside the right operand. + */ + predicate isDangerous() { + exists(Expr e | this.hasRightOperandDescendant(e) | + exists(Expr q | q = e.(MemberAccess).getQualifier() | + not q instanceof ThisAccess and + not q instanceof BaseAccess + ) + or + e instanceof MethodCall + or + e instanceof ArrayAccess + ) and + not exists(Expr e | this.hasRightOperandDescendant(e) | + e.(Call).getTarget().getAParameter().isOutOrRef() + ) } } from NonShortCircuit e +where e.isDangerous() select e, "Potentially dangerous use of non-short circuit logic."