Merge pull request #13147 from michaelnebel/csharp/entityframeworkrefactor

C#: Use synthetic global in the EntityFramework code instead of jump steps.

Author: michaelnebel

csharp/ql/lib/semmle/code/csharp/dataflow/FlowSummary.qll

@@ -72,6 +72,10 @@ module SummaryComponent {
           jrk.getTargetReturnKind() instanceof DataFlowDispatch::NormalReturnKind
         ))
   }
+
+  predicate syntheticGlobal = SummaryComponentInternal::syntheticGlobal/1;
+
+  class SyntheticGlobal = SummaryComponentInternal::SyntheticGlobal;
 }
 
 class SummaryComponentStack = Impl::Public::SummaryComponentStack;
@@ -112,6 +116,14 @@ module SummaryComponentStack {
 
   /** Gets a singleton stack representing a jump to `c`. */
   SummaryComponentStack jump(Callable c) { result = singleton(SummaryComponent::jump(c)) }
+
+  /** Gets a singleton stack representing a synthetic global with name `name`. */
+  SummaryComponentStack syntheticGlobal(string synthetic) {
+    result = singleton(SummaryComponent::syntheticGlobal(synthetic))
+  }
+
+  /** Gets a textual representation of this stack used for flow summaries. */
+  string getComponentStack(SummaryComponentStack s) { result = Impl::Public::getComponentStack(s) }
 }
 
 class SummarizedCallable = Impl::Public::SummarizedCallable;

csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll

@@ -180,6 +180,11 @@ module Public {
       result = "Argument[" + getParameterPosition(pos) + "]"
     )
     or
+    exists(string synthetic |
+      sc = TSyntheticGlobalSummaryComponent(synthetic) and
+      result = "SyntheticGlobal[" + synthetic + "]"
+    )
+    or
     sc = TReturnSummaryComponent(getReturnValueKind()) and result = "ReturnValue"
   }
 

csharp/ql/lib/semmle/code/csharp/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -19,6 +19,13 @@ class SummarizedCallableBase extends Callable {
   SummarizedCallableBase() { this.isUnboundDeclaration() }
 }
 
+/**
+ * A module for importing frameworks that define synthetic globals.
+ */
+private module SyntheticGlobals {
+  private import semmle.code.csharp.frameworks.EntityFramework
+}
+
 DataFlowCallable inject(SummarizedCallable c) { result.asSummarizedCallable() = c }
 
 /** Gets the parameter position of the instance parameter. */

csharp/ql/lib/semmle/code/csharp/frameworks/EntityFramework.qll

@@ -4,6 +4,7 @@
 
 import csharp
 private import DataFlow
+private import semmle.code.csharp.commons.QualifiedName
 private import semmle.code.csharp.frameworks.System
 private import semmle.code.csharp.frameworks.system.data.Entity
 private import semmle.code.csharp.frameworks.system.collections.Generic
@@ -236,7 +237,7 @@ module EntityFramework {
      * }
      * ```
      */
-    private Property getADbSetProperty(Class elementType) {
+    Property getADbSetProperty(Class elementType) {
       exists(ConstructedClass c |
         result.getType() = c and
         c.getUnboundDeclaration() instanceof DbSet and
@@ -351,67 +352,137 @@ module EntityFramework {
 
     /** Holds if component stack `head :: tail` is required for the output specification. */
     predicate requiresComponentStackOut(
-      Content head, Type headType, SummaryComponentStack tail, int dist
+      Content head, Type headType, SummaryComponentStack tail, int dist,
+      DbContextClassSetProperty dbSetProp
     ) {
-      exists(Property dbSetProp, PropertyContent c1 |
+      exists(PropertyContent c1 |
         dbSetProp = this.getADbSetProperty(headType) and
         this.stepRev(c1, _, head, headType, 0) and
         c1.getProperty() = dbSetProp and
-        tail = SummaryComponentStack::jump(dbSetProp.getGetter()) and
+        tail = SummaryComponentStack::return() and
         dist = 0
       )
       or
       exists(Content tailHead, SummaryComponentStack tailTail, Type tailType |
-        this.requiresComponentStackOut(tailHead, tailType, tailTail, dist - 1) and
+        this.requiresComponentStackOut(tailHead, tailType, tailTail, dist - 1, dbSetProp) and
         tail = SummaryComponentStack::push(SummaryComponent::content(tailHead), tailTail) and
         this.stepRev(tailHead, tailType, head, headType, dist)
       )
     }
-  }
-
-  private class DbContextSaveChanges extends EFSummarizedCallable {
-    private DbContextClass c;
-
-    DbContextSaveChanges() { this = c.getASaveChanges() }
 
+    /**
+     * Holds if `input` is a valid summary component stack for property `mapped` for this.
+     */
     pragma[noinline]
-    private predicate input(SummaryComponentStack input, Property mapped) {
+    predicate input(SummaryComponentStack input, Property mapped) {
       exists(PropertyContent head, SummaryComponentStack tail |
-        c.requiresComponentStackIn(head, _, tail, _) and
+        this.requiresComponentStackIn(head, _, tail, _) and
         head.getProperty() = mapped and
-        mapped = c.getAColumnProperty(_) and
+        mapped = this.getAColumnProperty(_) and
         input = SummaryComponentStack::push(SummaryComponent::content(head), tail)
       )
     }
 
+    /**
+     * Holds if `output` is a valid summary component stack for the getter of `dbSet`
+     * for property `mapped` for this.
+     */
     pragma[noinline]
-    private predicate output(SummaryComponentStack output, Property mapped) {
+    private predicate output(
+      SummaryComponentStack output, Property mapped, DbContextClassSetProperty dbSet
+    ) {
       exists(PropertyContent head, SummaryComponentStack tail |
-        c.requiresComponentStackOut(head, _, tail, _) and
+        this.requiresComponentStackOut(head, _, tail, _, dbSet) and
         head.getProperty() = mapped and
-        mapped = c.getAColumnProperty(_) and
+        mapped = this.getAColumnProperty(_) and
         output = SummaryComponentStack::push(SummaryComponent::content(head), tail)
       )
     }
 
+    /**
+     * Gets the synthetic name for the getter of `dbSet` for property `mapped` for this,
+     * where `output` is a valid summary component stack for the getter of `dbSet`
+     * for the property `mapped`.
+     */
+    pragma[nomagic]
+    string getSyntheticName(
+      SummaryComponentStack output, Property mapped, DbContextClassSetProperty dbSet
+    ) {
+      this = dbSet.getDbContextClass() and
+      this.output(output, mapped, dbSet) and
+      result = dbSet.getFullName() + "#" + SummaryComponentStack::getComponentStack(output)
+    }
+  }
+
+  private class DbContextClassSetProperty extends Property {
+    private DbContextClass c;
+
+    DbContextClassSetProperty() { this = c.getADbSetProperty(_) }
+
+    /**
+     * Gets the fully qualified name for this.
+     */
+    string getFullName() {
+      exists(string qualifier, string type, string name |
+        this.hasQualifiedName(qualifier, type, name)
+      |
+        result = getQualifiedName(qualifier, type, name)
+      )
+    }
+
+    /**
+     * Gets the context class where this is a DbSet property.
+     */
+    DbContextClass getDbContextClass() { result = c }
+  }
+
+  private class DbContextClassSetPropertySynthetic extends EFSummarizedCallable {
+    private DbContextClassSetProperty p;
+
+    DbContextClassSetPropertySynthetic() { this = p.getGetter() }
+
     override predicate propagatesFlow(
       SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
     ) {
-      exists(Property mapped |
+      exists(string name, DbContextClass c |
         preservesValue = true and
-        this.input(input, mapped) and
-        this.output(output, mapped)
+        name = c.getSyntheticName(output, _, p) and
+        input = SummaryComponentStack::syntheticGlobal(name)
       )
     }
   }
 
+  private class DbContextSaveChanges extends EFSummarizedCallable {
+    private DbContextClass c;
+
+    DbContextSaveChanges() { this = c.getASaveChanges() }
+
+    override predicate propagatesFlow(
+      SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
+    ) {
+      exists(string name, Property mapped |
+        preservesValue = true and
+        c.input(input, mapped) and
+        name = c.getSyntheticName(_, mapped, _) and
+        output = SummaryComponentStack::syntheticGlobal(name)
+      )
+    }
+  }
+
+  /**
+   * Add all possible synthetic global names.
+   */
+  private class EFSummarizedCallableSyntheticGlobal extends SummaryComponent::SyntheticGlobal {
+    EFSummarizedCallableSyntheticGlobal() { this = any(DbContextClass c).getSyntheticName(_, _, _) }
+  }
+
   private class DbContextSaveChangesRequiredSummaryComponentStack extends RequiredSummaryComponentStack
   {
     override predicate required(SummaryComponent head, SummaryComponentStack tail) {
       exists(Content c | head = SummaryComponent::content(c) |
         any(DbContextClass cls).requiresComponentStackIn(c, _, tail, _)
         or
-        any(DbContextClass cls).requiresComponentStackOut(c, _, tail, _)
+        any(DbContextClass cls).requiresComponentStackOut(c, _, tail, _, _)
       )
     }
   }

csharp/ql/test/library-tests/frameworks/EntityFramework/EntityFramework.cs

@@ -5,7 +5,7 @@
 
 namespace EFTests
 {
-    class Person
+    public class Person
     {
         public virtual int Id { get; set; }
         public virtual string Name { get; set; }
@@ -17,13 +17,13 @@ class Person
         public ICollection<Address> Addresses { get; set; }
     }
 
-    class Address
+    public class Address
     {
         public int Id { get; set; }
         public string Street { get; set; }
     }
 
-    class PersonAddressMap
+    public class PersonAddressMap
     {
         public int Id { get; set; }
         public int PersonId { get; set; }
@@ -34,7 +34,7 @@ class PersonAddressMap
         public Address Address { get; set; }
     }
 
-    class MyContext : DbContext
+    public class MyContext : DbContext
     {
         public virtual DbSet<Person> Persons { get; set; }
         public virtual DbSet<Address> Addresses { get; set; }

csharp/ql/test/library-tests/frameworks/EntityFramework/EntityFrameworkCore.cs

@@ -6,7 +6,7 @@
 
 namespace EFCoreTests
 {
-    class Person
+    public class Person
     {
         public virtual int Id { get; set; }
         public virtual string Name { get; set; }
@@ -18,13 +18,13 @@ class Person
         public ICollection<Address> Addresses { get; set; }
     }
 
-    class Address
+    public class Address
     {
         public int Id { get; set; }
         public string Street { get; set; }
     }
 
-    class PersonAddressMap
+    public class PersonAddressMap
     {
         public int Id { get; set; }
         public int PersonId { get; set; }
@@ -35,7 +35,7 @@ class PersonAddressMap
         public Address Address { get; set; }
     }
 
-    class MyContext : DbContext
+    public class MyContext : DbContext
     {
         public virtual DbSet<Person> Persons { get; set; }
         public virtual DbSet<Address> Addresses { get; set; }