From beb7750c214191265e19db15c4aa5227527da85a Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 19 Jan 2026 10:06:14 +0100 Subject: [PATCH] C#: Address review comments. --- .../DataFlowConsistency.ql | 6 - .../internal/ControlFlowGraphImpl.qll | 124 +++--------------- 2 files changed, 20 insertions(+), 110 deletions(-) diff --git a/csharp/ql/consistency-queries/DataFlowConsistency.ql b/csharp/ql/consistency-queries/DataFlowConsistency.ql index f9049068a7f..638bace3892 100644 --- a/csharp/ql/consistency-queries/DataFlowConsistency.ql +++ b/csharp/ql/consistency-queries/DataFlowConsistency.ql @@ -60,12 +60,6 @@ private module Input implements InputSig { qe.isConditional() and qe.getQualifier() = arg.asExpr() ) - or - // TODO: Remove once underlying issue is fixed - exists(AssignableDefinitions::OutRefDefinition def | - def.getTargetAccess().(QualifiableExpr) = arg.asExpr() and - def.getTargetAccess().isOutArgument() - ) } predicate multipleArgumentCallExclude(ArgumentNode arg, DataFlowCall call) { 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 77a88fb8c6a..6be79a17be2 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll @@ -430,7 +430,6 @@ module Expressions { not this instanceof ArrayCreation and not this instanceof QualifiedWriteAccess and not this instanceof QualifiedAccessorWrite and - not this instanceof QualifiedAccessorWriteOutParam and not this instanceof NoNodeExpr and not this instanceof SwitchExpr and not this instanceof SwitchCaseExpr and @@ -447,25 +446,29 @@ module Expressions { } /** - * A qualified write access. In a qualified write access, the access itself is - * not evaluated, only the qualifier and the indexer arguments (if any). + * A qualified write access. * - * Note that the successor declaration in `QualifiedAccessorWrite` ensures that the access itself + * The successor declaration in `QualifiedAccessorWrite` ensures that the access itself * is evaluated after the qualifier and the indexer arguments (if any) * and the right hand side of the assignment. + * + * When a qualified write access is used as an `out/ref` argument, the access itself is evaluated immediately. */ private class QualifiedWriteAccess extends ControlFlowTree instanceof WriteAccess, QualifiableExpr { QualifiedWriteAccess() { - this.hasQualifier() - or - // Member initializers like - // ```csharp - // new Dictionary() { [0] = "Zero", [1] = "One", [2] = "Two" } - // ``` - // need special treatment, because the accesses `[0]`, `[1]`, and `[2]` - // have no qualifier. - this = any(MemberInitializer mi).getLValue() + ( + this.hasQualifier() + or + // Member initializers like + // ```csharp + // new Dictionary() { [0] = "Zero", [1] = "One", [2] = "Two" } + // ``` + // need special treatment, because the accesses `[0]`, `[1]`, and `[2]` + // have no qualifier. + this = any(MemberInitializer mi).getLValue() + ) and + not exists(AssignableDefinitions::OutRefDefinition def | def.getTargetAccess() = this) } final override predicate propagatesAbnormal(AstNode child) { child = getExprChild(this, _) } @@ -486,101 +489,14 @@ module Expressions { exists(int i | last(getExprChild(this, i), pred, c) and c instanceof NormalCompletion and - (i != 0 or not c.(NullnessCompletion).isNull()) and + (if i = 0 then not c.(NullnessCompletion).isNull() else any()) and first(getExprChild(this, i + 1), succ) ) } } /** - * An expression that writes via an accessor in an `out` parameter, for example `s = M(out x.Field)`, - * where `Field` is a field. - * - * Note that `ref` parameters are not included here as they are considered reads before the call. - * Ideally, we would model `ref` parameters as both reads and writes, but that is not currently supported. - * - * Accessor writes need special attention, because we need to model that the - * access is written after the method call. - * - * In the example above, this means we want a CFG that looks like - * - * ```csharp - * x -> call M -> x.Field -> s = M(out x.Field) - * ``` - */ - private class QualifiedAccessorWriteOutParam extends PostOrderTree instanceof Expr { - QualifiedAccessorWriteOutParam() { - exists(AssignableDefinitions::OutRefDefinition def | - def.getExpr() = this and - def.getTargetAccess() instanceof QualifiableExpr and - def.getTargetAccess().isOutArgument() - ) - } - - private QualifiableExpr getOutAccess(int i) { - result = - rank[i + 1](AssignableDefinitions::OutRefDefinition def | - def.getExpr() = this and - def.getTargetAccess() instanceof QualifiableExpr and - def.getTargetAccess().isOutArgument() - | - def order by def.getIndex() - ).getTargetAccess() - } - - private QualifiableExpr getLastOutAccess() { - exists(int last | - result = this.getOutAccess(last) and - not exists(this.getOutAccess(last + 1)) - ) - } - - final override predicate propagatesAbnormal(AstNode child) { child = getExprChild(this, _) } - - final override predicate first(AstNode first) { - first(getExprChild(this, 0), first) - or - not exists(getExprChild(this, 0)) and - first = this - } - - final override predicate last(AstNode last, Completion c) { - // The last ast node is the last out writeaccess. - // Completion from the call itself is propagated (required for eg. conditions). - last = this.getLastOutAccess() and - c.isValidFor(this) - } - - final override predicate succ(AstNode pred, AstNode succ, Completion c) { - exists(int i | - last(getExprChild(this, i), pred, c) and - c instanceof NormalCompletion - | - // Post-order: flow from last element of last child to element itself - i = max(int j | exists(getExprChild(this, j))) and - succ = this - or - // Standard left-to-right evaluation - first(getExprChild(this, i + 1), succ) - ) - or - // Flow from this element to the first write access. - pred = this and - succ = this.getOutAccess(0) and - c.isValidFor(pred) and - c instanceof NormalCompletion - or - // Flow from one access to the next - exists(int i | pred = this.getOutAccess(i) | - succ = this.getOutAccess(i + 1) and - c.isValidFor(pred) and - c instanceof NormalCompletion - ) - } - } - - /** - * An expression that writes via an accessor, for example `x.Prop = 0`, + * An expression that writes via a qualifiable expression, for example `x.Prop = 0`, * where `Prop` is a property. * * Accessor writes need special attention, because we need to model the fact @@ -591,7 +507,7 @@ module Expressions { * x -> 0 -> set_Prop -> x.Prop = 0 * ``` * - * For consistency, control flow is implemented this way for all accessor writes. + * For consistency, control flow is implemented the same way for other qualified writes. * For example, `x.Field = 0`, where `Field` is a field, we want a CFG that looks like * * ```csharp @@ -645,7 +561,7 @@ module Expressions { exists(int i | last(getExprChild(this, i), pred, c) and c instanceof NormalCompletion and - (i != 0 or not c.(NullnessCompletion).isNull()) and + (if i = 0 then not c.(NullnessCompletion).isNull() else any()) and first(getExprChild(this, i + 1), succ) ) or