Sitelet https://web.archive.org/web/20260530034019/https://github.com/github/codeql/pull/4646
Skip to content

C#: Represent all expressions in post-order in the CFG#4646

Merged
hvitved merged 3 commits into
github:mainfrom
hvitved:csharp/cfg/post-order-exprs
Nov 17, 2020
Merged

C#: Represent all expressions in post-order in the CFG#4646
hvitved merged 3 commits into
github:mainfrom
hvitved:csharp/cfg/post-order-exprs

Conversation

@hvitved
Copy link
Copy Markdown
Contributor

@hvitved hvitved commented Nov 10, 2020 •
edited
Loading

This PR changes the CFG construction for expressions so they are all visited in post order. The motivation for doing this is consistency, as well as to eliminate false positives (see https://github.com/github/codeql-csharp-team/issues/87).

https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/618/ (excluding last commit).
https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/621/ (including last commit).

Example 1 
void M1(bool b1, bool b2)
{
    if (b1 && b2)
        return;
}

Before

Screenshot 2020-11-12 at 20 23 54

After

Screenshot 2020-11-12 at 20 18 32
Example 2 
void M2(bool b)
{
    var s = b ? "taint source" : "not tainted";
    if (b)
        Sink(s);
    else
        Sink(s);
}

Before

Screenshot 2020-11-12 at 20 22 22

After

Screenshot 2020-11-12 at 20 20 13

@github-actions github-actions Bot added the C# label Nov 10, 2020
@hvitved hvitved force-pushed the csharp/cfg/post-order-exprs branch 4 times, most recently from 9cc2cb7 to 967a834 Compare November 11, 2020 16:35
@hvitved hvitved force-pushed the csharp/cfg/post-order-exprs branch from 967a834 to 94deed3 Compare November 12, 2020 19:05
@hvitved hvitved marked this pull request as ready for review November 13, 2020 09:34
@hvitved hvitved requested a review from a team as a code owner November 13, 2020 09:34
tamasvajk
tamasvajk approved these changes Nov 16, 2020
View reviewed changes
Comment thread csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql
@@ -35,7 +35,23 @@ class ConstantBooleanCondition extends ConstantCondition {

override predicate isWhiteListed() {
Copy link
Copy Markdown
Contributor

@tamasvajk tamasvajk Nov 16, 2020

Choose a reason for hiding this comment

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

Why do we need white listing of some constant conditions?

Also, we might want to change the name of the predicate to something more inclusive.

Copy link
Copy Markdown
Contributor Author

@hvitved hvitved Nov 17, 2020

Choose a reason for hiding this comment

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

For example, if x is constantly false, then we do not also want to report that x && y is constantly false.

Comment thread csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowGraph.qll
)
// Operand exits abnormally
result = cfe.(LogicalNotExpr).getOperand() and
c = TRec(TLastRecAbnormalCompletion())
Copy link
Copy Markdown
Contributor

@tamasvajk tamasvajk Nov 16, 2020

Choose a reason for hiding this comment

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

Why do we need to list this many cases for the abnormal completion? Isn't it the case that any child (of tyoe Expr) can exit abnormally?

Copy link
Copy Markdown
Contributor Author

@hvitved hvitved Nov 17, 2020

Choose a reason for hiding this comment

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

This should be better in an upcoming refactoring.

Comment thread csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowGraph.qll
result = this.(JumpStmt).getChild(0) or
result = this.(ThrowExpr).getExpr() or
result = getObjectCreationArgument(this, 0)
result = this.(StandardExpr).getFirstChildElement()
Copy link
Copy Markdown
Contributor

@tamasvajk tamasvajk Nov 16, 2020

Choose a reason for hiding this comment

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

I had the feeling so far that we're building the child/parent relationships in the AST in the order of execution. Why do we need to handle individually these types?

Also, JumpStmt feels odd to be mixed with expressions.

Copy link
Copy Markdown
Contributor Author

@hvitved hvitved Nov 17, 2020

Choose a reason for hiding this comment

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

Again, should be better in the follow-up refactoring.

@hvitved hvitved merged commit 7f0ad2d into github:main Nov 17, 2020
@hvitved hvitved deleted the csharp/cfg/post-order-exprs branch November 17, 2020 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tamasvajk tamasvajk tamasvajk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tamasvajk tamasvajk

Labels

C#

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hvitved @tamasvajk