C#: Adopt shared data flow implementation #1306

hvitved · 2019-05-06T13:35:45Z

Finally, the PR that brings C# in line with Java and C/C++ on the data flow implementation. The PR is structured into several commits, which are best reviewed one-by-one:

26debb8: A simple toString() change.
a6fa6df: Copies the shared data flow files from Java/C++. All changes required for C# were done on Generalize data-flow library in preparation for C# adoption #1226, so DataFlowImpl(Common).qll need not be reviewed.
c6a471e: This is the commit where all the work is done. Highlights:
- General refactoring to fit with the shared data flow implementation.
- Move CFG splitting logic into ControlFlowReachability.qll.
- Replace isAdditionalFlowStepIntoCall() with TaintedParameterNode.
- Redefine ReturnNode to be the actual values that are returned, which should yield better path information.
7b7a1ec: A simple file move.

Here are some performance numbers (full report here):

Project              23faa942ced@639d715d0  23faa942ced@949b3601d
pythonnet            101                    115                    1.13861
OneOf                773                    908                    1.17464
corefx               2195                   2621                   1.19408
nhibernate-core      2841                   3451                   1.21471
roslyn               2653                   3282                   1.23709
monodevelop          3487                   4388                   1.25839
PowerShell           838                    1094                   1.30549
OrchardCore          1000                   1307                   1.307
EntityFrameworkCore  2023                   2745                   1.3569
coreclr              287                    393                    1.36934
ql                   285                    417                    1.46316
runuo                670                    1040                   1.55224
mono                 1893                   3584                   1.89329

I already have a few performance improvements ready for the shared implementation, which should bring performance back at par, but I would rather do those in a follow-up PR.

- General refactoring to fit with the shared data flow implementation. - Move CFG splitting logic into `ControlFlowReachability.qll`. - Replace `isAdditionalFlowStepIntoCall()` with `TaintedParameterNode`. - Redefine `ReturnNode` to be the actual values that are returned, which should yield better path information. - No longer consider overrides in CIL calls.

hvitved · 2019-05-09T17:56:34Z

I also ran a performance test on a huge customer snapshot; here performance went from 17695s to 84240s, but I have verified that my upcoming improvements roughly restores performance (19518s).

calumgrant

This is great stuff! You have done a lot of work.

calumgrant · 2019-05-08T08:43:42Z

csharp/ql/src/semmle/code/csharp/dataflow/CallContext.qll

@@ -32,6 +33,8 @@ class CallContext extends TCallContext {
 /** An empty call context. */
 class EmptyCallContext extends CallContext, TEmptyCallContext {
  override string toString() { result = "<empty>" }
+
+  override Location getLocation() { result instanceof EmptyLocation }


override EmptyLocation getLocation() { any() }
?

calumgrant · 2019-05-13T08:47:14Z

csharp/ql/src/semmle/code/csharp/dataflow/internal/ControlFlowReachability.qll

+private import DataFlowPrivate
+private import DataFlowPublic
+
+private ControlFlowElement getAScope(boolean exactScope) {


This feels like there could be a class ControlFlowScope here instead

calumgrant · 2019-05-14T15:43:32Z

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

+    }
+
+  cached
+  DotNet::Callable getEnclosingCallable(Node node) {


Why can't we use classes here? There seems to be a lot of synthetic dispatch here that would be easier to maintain using QL classes.

It is simply to make sure that the cached predicates are evaluated in the same stage (in the cached module DataFlowPrivateCached).

calumgrant · 2019-05-14T16:11:58Z

csharp/ql/src/semmle/code/csharp/exprs/Creation.qll

@@ -378,7 +379,9 @@ class ArrayCreation extends Expr, @array_creation_expr {
 class AnonymousFunctionExpr extends Expr, Callable, @anonymous_function_expr {
  override string getName() { result = "<anonymous>" }

-  override Type getReturnType() { result = getType().(DelegateType).getReturnType() }
+  override Type getReturnType() {
+    result = getType().(SystemLinqExpressions::DelegateExtType).getDelegateType().getReturnType()


calumgrant · 2019-05-14T16:21:59Z

...tests/Security Features/CWE-201/ExposureInTransmittedData/ExposureInTransmittedData.expected

@@ -1,21 +1,13 @@
 edges
+| ExposureInTransmittedData.cs:16:32:16:39 | access to local variable password | ExposureInTransmittedData.cs:16:32:16:39 | access to local variable password |
+| ExposureInTransmittedData.cs:20:32:20:44 | call to method ToString | ExposureInTransmittedData.cs:20:32:20:44 | call to method ToString |
+| ExposureInTransmittedData.cs:24:32:24:41 | access to property Message | ExposureInTransmittedData.cs:24:32:24:41 | access to property Message |


Where does all of this self-flow come from? It does not seem to be helpful.

This is because the shared implementation represents paths of length 0 as paths of length 1 by adding a synthetic sink node. This is also the reason why the nodes predicate is no longer needed. (ping @aschackmull)

(Note that these are therefore only additions to the edges query predicate, not the select predicate.)

True. Although we might need to add a nodes predicate in order to save the PathNode.toString as labels, but that's a separate concern.

calumgrant · 2019-05-15T09:40:23Z

csharp/ql/test/query-tests/Security Features/CWE-798/HardcodedCredentials.expected

 #select
 | HardcodedCredentials.cs:17:25:17:36 | "myPa55word" | HardcodedCredentials.cs:17:25:17:36 | "myPa55word" | HardcodedCredentials.cs:17:25:17:36 | "myPa55word" | The hard-coded value "myPa55word" flows to $@ which is compared against $@. | HardcodedCredentials.cs:17:25:17:36 | "myPa55word" | "myPa55word" | HardcodedCredentials.cs:17:13:17:20 | access to local variable password | access to local variable password |
+| HardcodedCredentials.cs:17:25:17:36 | "myPa55word" | HardcodedCredentials.cs:17:25:17:36 | "myPa55word" | HardcodedCredentials.cs:17:25:17:36 | "myPa55word" | The hard-coded value "myPa55word" flows to $@ which is compared against $@. | HardcodedCredentials.cs:17:25:17:36 | "myPa55word" | "myPa55word" | HardcodedCredentials.cs:17:13:17:20 | access to local variable password | access to local variable password |


There seems to be some unnecessary duplication of the results. I think these duplicated results must be removed (also in other queries).

Good catch, this is actually because of a bug in the query; it uses hasFlow, but should have used hasFlowPath.

calumgrant · 2019-05-15T09:51:20Z

csharp/ql/src/semmle/code/csharp/dataflow/CallContext.qll

@@ -46,25 +45,22 @@ abstract class ArgumentCallContext extends CallContext {
   * Holds if this call context represents the argument at position `i` of the
   * call expression `call`.
   */
-  abstract predicate isArgument(DotNet::Expr call, int i);
+  abstract predicate isArgument(Expr call, int i);


I guess we don't bother with CIL::Expr because we don't analyse virtual dispatch in CIL anyway. So it makes sense to remove these from the call context for now.

Actually, this is because these call contexts are now only used when resolving delegate targets in DelegateDataFlow.qll, and there flow is restricted to C# code. The shared DataFlowImplCommon.qll has its own notion of a call context.

hvitved added 4 commits May 6, 2019 14:54

C#: Change ImplicitCapturedArgumentNode::toString()

26debb8

C#: Add shared data flow files

a6fa6df

C#: Move DelegateDataFlow.qll into internal folder

7b7a1ec

hvitved added the C# label May 6, 2019

hvitved requested a review from calumgrant May 6, 2019 13:35

hvitved requested a review from a team as a code owner May 6, 2019 13:35

C#: Simplify DataFlow::Node::getType()

a89505b

C#: Simplify CallContext.qll

89e60dc

calumgrant reviewed May 15, 2019

View reviewed changes

C#: Address review comments

949b360

calumgrant approved these changes May 15, 2019

View reviewed changes

calumgrant merged commit 6a198ff into github:master May 15, 2019

hvitved deleted the csharp/dataflow/shared-implementation branch May 16, 2019 07:39

hvitved mentioned this pull request May 16, 2019

Data flow: performance improvements #1329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

C#: Adopt shared data flow implementation #1306

C#: Adopt shared data flow implementation #1306

Uh oh!

hvitved commented May 6, 2019 •

edited

Loading

Uh oh!

hvitved commented May 9, 2019

Uh oh!

calumgrant left a comment

Uh oh!

calumgrant May 8, 2019

Uh oh!

calumgrant May 13, 2019

Uh oh!

calumgrant May 14, 2019

Uh oh!

hvitved May 15, 2019

Uh oh!

calumgrant May 14, 2019

Uh oh!

calumgrant May 14, 2019

Uh oh!

hvitved May 15, 2019

Uh oh!

hvitved May 15, 2019

Uh oh!

aschackmull May 15, 2019

Uh oh!

calumgrant May 15, 2019

Uh oh!

hvitved May 15, 2019

Uh oh!

calumgrant May 15, 2019

Uh oh!

hvitved May 15, 2019

Uh oh!

Uh oh!

C#: Adopt shared data flow implementation #1306

C#: Adopt shared data flow implementation #1306

Uh oh!

Conversation

hvitved commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvitved commented May 9, 2019

Uh oh!

calumgrant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hvitved commented May 6, 2019 •

edited

Loading