C#: Fix CFG position of property setter calls.

Author: aschackmull

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

@@ -270,15 +270,17 @@ private module CallGraph {
    *
    * the constructor call `new Lazy<int>(M2)` includes `M2` as a target.
    */
-  Callable getARuntimeTarget(Call c, boolean libraryDelegateCall) {
+  Callable getARuntimeTarget(Call c, ControlFlowNode n, boolean libraryDelegateCall) {
     // Non-delegate call: use dispatch library
     exists(DispatchCall dc | dc.getCall() = c |
+      n = dc.getControlFlowNode() and
       result = dc.getADynamicTarget().getUnboundDeclaration() and
       libraryDelegateCall = false
     )
     or
     // Delegate call: use simple analysis
-    result = SimpleDelegateAnalysis::getARuntimeDelegateTarget(c, libraryDelegateCall)
+    result = SimpleDelegateAnalysis::getARuntimeDelegateTarget(c, libraryDelegateCall) and
+    n = c.getControlFlowNode()
   }
 
   private module SimpleDelegateAnalysis {
@@ -465,7 +467,7 @@ private module CallGraph {
 
   /** Holds if `(c1,c2)` is an edge in the call graph. */
   predicate callEdge(Callable c1, Callable c2) {
-    exists(Call c | c.getEnclosingCallable() = c1 and c2 = getARuntimeTarget(c, _))
+    exists(Call c | c.getEnclosingCallable() = c1 and c2 = getARuntimeTarget(c, _, _))
   }
 }
 
@@ -599,7 +601,7 @@ private module FieldOrPropsImpl {
   private predicate intraInstanceCallEdge(Callable c1, InstanceCallable c2) {
     exists(Call c |
       c.getEnclosingCallable() = c1 and
-      c2 = getARuntimeTarget(c, _) and
+      c2 = getARuntimeTarget(c, _, _) and
       c.(QualifiableExpr).targetIsLocalInstance()
     )
   }
@@ -615,8 +617,7 @@ private module FieldOrPropsImpl {
 
   pragma[noinline]
   predicate callAt(BasicBlock bb, int i, Call call) {
-    bb.getNode(i) = call.getAControlFlowNode() and
-    getARuntimeTarget(call, _).hasBody()
+    getARuntimeTarget(call, bb.getNode(i), _).hasBody()
   }
 
   /**
@@ -635,7 +636,7 @@ private module FieldOrPropsImpl {
     Call call, FieldOrPropSourceVariable fps, FieldOrProp fp, Callable c, boolean fresh
   ) {
     updateCandidate(_, _, fps, call) and
-    c = getARuntimeTarget(call, _) and
+    c = getARuntimeTarget(call, _, _) and
     fp = fps.getAssignable() and
     if c instanceof Constructor then fresh = true else fresh = false
   }

csharp/ql/lib/semmle/code/csharp/dispatch/Dispatch.qll

@@ -20,6 +20,9 @@ class DispatchCall extends Internal::TDispatchCall {
   /** Gets the underlying expression of this call. */
   Expr getCall() { result = Internal::getCall(this) }
 
+  /** Gets the control flow node of this call. */
+  ControlFlowNode getControlFlowNode() { result = Internal::getControlFlowNode(this) }
+
   /** Gets the `i`th argument of this call. */
   Expr getArgument(int i) { result = Internal::getArgument(this, i) }
 
@@ -90,7 +93,12 @@ private module Internal {
         not mc.isLateBound() and
         not isExtensionAccessorCall(mc)
       } or
-      TDispatchAccessorCall(AccessorCall ac) or
+      TDispatchAccessorCall(AccessorCall ac, boolean isRead) {
+        // For compound assignments an AccessorCall can be both a read and a write
+        ac instanceof AssignableRead and isRead = true
+        or
+        ac instanceof AssignableWrite and isRead = false
+      } or
       TDispatchOperatorCall(OperatorCall oc) { not oc.isLateBound() } or
       TDispatchReflectionCall(MethodCall mc, string name, Expr object, Expr qualifier, int args) {
         isReflectionCall(mc, name, object, qualifier, args)
@@ -117,6 +125,11 @@ private module Internal {
     cached
     Expr getCall(DispatchCall dc) { result = dc.(DispatchCallImpl).getCall() }
 
+    cached
+    ControlFlowNode getControlFlowNode(DispatchCall dc) {
+      result = dc.(DispatchCallImpl).getControlFlowNode()
+    }
+
     cached
     Expr getArgument(DispatchCall dc, int i) { result = dc.(DispatchCallImpl).getArgument(i) }
 
@@ -224,6 +237,9 @@ private module Internal {
     /** Gets the underlying expression of this call. */
     abstract Expr getCall();
 
+    /** Gets the control flow node of this call. */
+    ControlFlowNode getControlFlowNode() { result = this.getCall().getControlFlowNode() }
+
     /** Gets the `i`th argument of this call. */
     abstract Expr getArgument(int i);
 
@@ -877,13 +893,28 @@ private module Internal {
    * into account.
    */
   private class DispatchAccessorCall extends DispatchOverridableCall, TDispatchAccessorCall {
-    override AccessorCall getCall() { this = TDispatchAccessorCall(result) }
+    private predicate isRead() { this = TDispatchAccessorCall(_, true) }
+
+    override ControlFlowNode getControlFlowNode() {
+      if this.isRead()
+      then result = this.getCall().getControlFlowNode()
+      else
+        exists(AssignableDefinition def |
+          def.getTargetAccess() = this.getCall() and result = def.getExpr().getControlFlowNode()
+        )
+    }
+
+    override AccessorCall getCall() { this = TDispatchAccessorCall(result, _) }
 
     override Expr getArgument(int i) { result = this.getCall().getArgument(i) }
 
     override Expr getQualifier() { result = this.getCall().(MemberAccess).getQualifier() }
 
-    override Accessor getAStaticTarget() { result = this.getCall().getTarget() }
+    override Accessor getAStaticTarget() {
+      if this.isRead()
+      then result = this.getCall().getReadTarget()
+      else result = this.getCall().getWriteTarget()
+    }
 
     override RuntimeAccessor getADynamicTarget() {
       result = DispatchOverridableCall.super.getADynamicTarget() and

csharp/ql/lib/semmle/code/csharp/exprs/Call.qll

@@ -633,7 +633,25 @@ class FunctionPointerCall extends DelegateLikeCall, @function_pointer_invocation
  * (`EventCall`).
  */
 class AccessorCall extends Call, QualifiableExpr, @call_access_expr {
-  override Accessor getTarget() { none() }
+  override Accessor getTarget() { result = this.getReadTarget() or result = this.getWriteTarget() }
+
+  /**
+   * Gets the static (compile-time) target of this call, assuming that this is
+   * an `AssignableRead`.
+   *
+   * Note that left-hand sides of compound assignments are both
+   * `AssignableRead`s and `AssignableWrite`s.
+   */
+  Accessor getReadTarget() { none() }
+
+  /**
+   * Gets the static (compile-time) target of this call, assuming that this is
+   * an `AssignableWrite`.
+   *
+   * Note that left-hand sides of compound assignments are both
+   * `AssignableRead`s and `AssignableWrite`s.
+   */
+  Accessor getWriteTarget() { none() }
 
   override Expr getArgument(int i) { none() }
 
@@ -655,12 +673,12 @@ class AccessorCall extends Call, QualifiableExpr, @call_access_expr {
  * ```
  */
 class PropertyCall extends AccessorCall, PropertyAccessExpr {
-  override Accessor getTarget() {
-    exists(PropertyAccess pa, Property p | pa = this and p = this.getProperty() |
-      pa instanceof AssignableRead and result = p.getGetter()
-      or
-      pa instanceof AssignableWrite and result = p.getSetter()
-    )
+  override Accessor getReadTarget() {
+    this instanceof AssignableRead and result = this.getProperty().getGetter()
+  }
+
+  override Accessor getWriteTarget() {
+    this instanceof AssignableWrite and result = this.getProperty().getSetter()
   }
 
   override Expr getArgument(int i) {
@@ -690,12 +708,12 @@ class PropertyCall extends AccessorCall, PropertyAccessExpr {
  * ```
  */
 class IndexerCall extends AccessorCall, IndexerAccessExpr {
-  override Accessor getTarget() {
-    exists(IndexerAccess ia, Indexer i | ia = this and i = this.getIndexer() |
-      ia instanceof AssignableRead and result = i.getGetter()
-      or
-      ia instanceof AssignableWrite and result = i.getSetter()
-    )
+  override Accessor getReadTarget() {
+    this instanceof AssignableRead and result = this.getIndexer().getGetter()
+  }
+
+  override Accessor getWriteTarget() {
+    this instanceof AssignableWrite and result = this.getIndexer().getSetter()
   }
 
   override Expr getArgument(int i) {
@@ -770,7 +788,7 @@ class ExtensionPropertyCall extends PropertyCall {
  * ```
  */
 class EventCall extends AccessorCall, EventAccessExpr {
-  override EventAccessor getTarget() {
+  override EventAccessor getWriteTarget() {
     exists(Event e, AddOrRemoveEventExpr aoree |
       e = this.getEvent() and
       aoree.getLValue() = this