Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions csharp/ql/consistency-queries/DataFlowConsistency.ql
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ private module Input implements InputSig<Location, CsharpDataFlow> {
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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* C# 14: Support for null-conditional assignments (such as `c?.Prop = p`). Furthermore, the `MaybeNullExpr` class now takes null-conditional access (such as `?.`) into account when modeling potential null values.
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,8 @@ module Expressions {
not this instanceof ObjectCreation and
not this instanceof ArrayCreation and
not this instanceof QualifiedWriteAccess and
not this instanceof AccessorWrite 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
Expand All @@ -448,6 +449,10 @@ 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).
Comment on lines 449 to 450
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This QL doc is now incorrect. Perhaps just remove the entire In a qualified write access, the access itself is not evaluated, only the qualifier and the indexer arguments (if any).

*
* Note that 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.
*/
private class QualifiedWriteAccess extends ControlFlowTree instanceof WriteAccess, QualifiableExpr
{
Expand All @@ -470,25 +475,112 @@ module Expressions {
final override predicate last(AstNode last, Completion c) {
// Skip the access in a qualified write access
last(getLastExprChild(this), last, c)
or
// Qualifier exits with a null completion
super.isConditional() and
last(super.getQualifier(), last, c) and
c.(NullnessCompletion).isNull()
}

final override predicate succ(AstNode pred, AstNode succ, Completion c) {
exists(int i |
last(getExprChild(this, i), pred, c) and
c instanceof NormalCompletion and
(i != 0 or not c.(NullnessCompletion).isNull()) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i = 0 then not c.(NullnessCompletion).isNull() else any() is slightly better as it avoids tuple duplication during evaluation (the case where i != 0 and c is not (NullnessCompletion).isNull() is currently valid for both disjuncts.

first(getExprChild(this, i + 1), succ)
)
}
}

private class StatOrDynAccessorCall_ =
@dynamic_member_access_expr or @dynamic_element_access_expr or @call_access_expr;
/**
* 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call M -> M(out x.Field)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not convinced that this is the CFG we want in this case. Consider this example:

    void MyTest()
    {
        if (Set(out this.IntField))
            return;
        Guarded();
    }

It gets the following CFG

flowchart TD
1["enter MyTest"]
10["return ...;"]
11["call to method Guarded"]
12["this access"]
13["...;"]
2["exit MyTest"]
3["exit MyTest (normal)"]
4["{...}"]
5["if (...) ..."]
6["call to method Set"]
7["this access"]
8["this access"]
9["access to field IntField"]

1 --> 4
3 --> 2
4 --> 5
5 --> 7
6 -- false, true --> 9
7 --> 8
8 --> 6
9 -- false --> 13
9 -- true --> 10
10 -- return --> 3
11 --> 3
12 --> 11
13 --> 12
Loading

Note how get the join at access to field IntField, which means that we can no longer tell that Guarded is guarded by Set(out this.IntField) evaluating to true. I think what we want is simply the same as if there was no out, i.e.

flowchart TD
1["enter MyTest"]
10["return ...;"]
11["call to method Guarded"]
12["this access"]
13["...;"]
2["exit MyTest"]
3["exit MyTest (normal)"]
4["{...}"]
5["if (...) ..."]
6["call to method Set"]
7["this access"]
8["this access"]
9["access to field IntField"]

1 --> 4
3 --> 2
4 --> 5
5 --> 7
9 --> 6
7 --> 8
8 --> 9
6 -- false --> 13
6 -- true --> 10
10 -- return --> 3
11 --> 3
12 --> 11
13 --> 12
Loading

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can do it like that. However, then it looks like a field read instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will also significantly simplify the implementation.

* ```
*/
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)
}

/** A normal or a (potential) dynamic call to an accessor. */
private class StatOrDynAccessorCall extends Expr, StatOrDynAccessorCall_ { }
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 call, for example `x.Prop = 0`,
* An expression that writes via an accessor, for example `x.Prop = 0`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessor means stuff like get and set, so that is no longer the right word to use as it also includes fields.

* where `Prop` is a property.
*
* Accessor writes need special attention, because we need to model the fact
Expand All @@ -498,24 +590,33 @@ module Expressions {
* ```csharp
* x -> 0 -> set_Prop -> x.Prop = 0
* ```
*
* For consistency, control flow is implemented this way for all accessor writes.
* For example, `x.Field = 0`, where `Field` is a field, we want a CFG that looks like
*
* ```csharp
* x -> 0 -> x.Field -> x.Field = 0
* ```
*/
class AccessorWrite extends PostOrderTree instanceof Expr {
private class QualifiedAccessorWrite extends PostOrderTree instanceof Expr {
AssignableDefinition def;

AccessorWrite() {
QualifiedAccessorWrite() {
def.getExpr() = this and
def.getTargetAccess().(WriteAccess) instanceof StatOrDynAccessorCall and
def.getTargetAccess().(WriteAccess) instanceof QualifiableExpr and
not def instanceof AssignableDefinitions::OutRefDefinition and
not this instanceof AssignOperationWithExpandedAssignment
}

/**
* Gets the `i`th accessor being called in this write. More than one call
* can happen in tuple assignments.
*/
StatOrDynAccessorCall getCall(int i) {
QualifiableExpr getAccess(int i) {
result =
rank[i + 1](AssignableDefinitions::TupleAssignmentDefinition tdef |
tdef.getExpr() = this and tdef.getTargetAccess() instanceof StatOrDynAccessorCall
tdef.getExpr() = this and
tdef.getTargetAccess() instanceof QualifiableExpr
|
tdef order by tdef.getEvaluationOrder()
).getTargetAccess()
Expand All @@ -528,7 +629,13 @@ module Expressions {
final override predicate propagatesAbnormal(AstNode child) {
child = getExprChild(this, _)
or
child = this.getCall(_)
child = this.getAccess(_)
}

final override predicate last(AstNode last, Completion c) {
PostOrderTree.super.last(last, c)
or
last(getExprChild(this, 0), last, c) and c.(NullnessCompletion).isNull()
}

final override predicate first(AstNode first) { first(getExprChild(this, 0), first) }
Expand All @@ -538,24 +645,25 @@ module Expressions {
exists(int i |
last(getExprChild(this, i), pred, c) and
c instanceof NormalCompletion and
(i != 0 or not c.(NullnessCompletion).isNull()) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment.

first(getExprChild(this, i + 1), succ)
)
or
// Flow from last element of last child to first accessor call
last(getLastExprChild(this), pred, c) and
succ = this.getCall(0) and
succ = this.getAccess(0) and
c instanceof NormalCompletion
or
// Flow from one call to the next
exists(int i | pred = this.getCall(i) |
succ = this.getCall(i + 1) and
exists(int i | pred = this.getAccess(i) |
succ = this.getAccess(i + 1) and
c.isValidFor(pred) and
c instanceof NormalCompletion
)
or
// Post-order: flow from last call to element itself
exists(int last | last = max(int i | exists(this.getCall(i))) |
pred = this.getCall(last) and
exists(int last | last = max(int i | exists(this.getAccess(i))) |
pred = this.getAccess(last) and
succ = this and
c.isValidFor(pred) and
c instanceof NormalCompletion
Expand Down Expand Up @@ -704,7 +812,9 @@ module Expressions {
private class ConditionallyQualifiedExpr extends PostOrderTree instanceof QualifiableExpr {
private Expr qualifier;

ConditionallyQualifiedExpr() { this.isConditional() and qualifier = getExprChild(this, 0) }
ConditionallyQualifiedExpr() {
this.isConditional() and qualifier = getExprChild(this, 0) and not this instanceof WriteAccess
}

final override predicate propagatesAbnormal(AstNode child) { child = qualifier }

Expand Down
6 changes: 6 additions & 0 deletions csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ private Expr maybeNullExpr(Expr reason) {
)
or
result.(NullCoalescingExpr).getRightOperand() = maybeNullExpr(reason)
or
result =
any(QualifiableExpr qe |
qe.isConditional() and
qe.getQualifier() = maybeNullExpr(reason)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually needed? I would think that an expression like x?.M() can always potentially be null, regardless of what we know about x.

)
}

/** An expression that may be `null`. */
Expand Down
22 changes: 22 additions & 0 deletions csharp/ql/test/library-tests/controlflow/graph/Assignments.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,26 @@ void M()

delegate void EventHandler(object sender, object e);
event EventHandler Event;
int IntField;
string StringField;

void SetParamSingle(out int x)
{
x = 42;
}

void SetParamMulti(out int x, object o, out string y)
{
x = 42;
y = "Hello";
}

void M2()
{
int x1;
SetParamSingle(out x1);
SetParamSingle(out IntField);
SetParamMulti(out var y, null, out StringField);
SetParamMulti(out IntField, null, out StringField);
}
}
Loading