CSHARP.CONCURRENCY.UG.PARAM : Unguarded Parameter (C#)

Summary

A parameter of a method or constructor is accessed without the expected lock being held.

C# uses statements and methods to guarantee thatdata is accessed in a sequential way and avoid race conditions in multithreaded applications. This is achieved with the lock statements, or by surrounding a block between calls to Monitor.Enter(obj) and Monitor.Exit(obj). Incorrect uses of synchronization result in unexpected behaviors and subtle bugs, very hard to identify and reproduce. This checker helps the programmer by identifying or checking the locks that are held when a field is accessed or a method is called. This information can be inferred, and reported in a jaif file, or verified, if the programmer provides source code attributes on the locks that must be held for specific fields and methods. Moreover, this checker signals situations where a synchronization seems missing on a field.

The [GuardedBy] attribute for fields and parameters and the [Holding] attribute for methods and constructors accepts a string argument, according to the following syntax

• [GuardedBy/Holding(new string[] { "this" })]: the lock on the receiver of a non-static field or method must be held
• [GuardedBy(new string[] {"itself")]: the lock on the same parameter or field must be held
• [GuardedBy/Holding(new string[] { "field-name" })]: the lock on the named field of the receiver of a non-static field or method must be held
• [GuardedBy/Holding(new string[] { "Class.field-name" })]: the lock on the named static field of the named class must be held
• [GuardedBy/Holding(new string[] { "Class.class" })]: the lock of the unique class object representing the class named Class must be held
• [GuardedBy/Holding(new string[] { "foo()" })]: the lock on the return value of the named instance method called on the receiver of a non-static field or method must be held. Method foo must return a reference type
• [GuardedBy/Holding(new string[] { "Class.foo()" })]: the lock on the return value of the named static method of the named class must be held. Method foo must return a reference type.

Properties

WARNING_FILTER += allow class="Unguarded Parameter (C#)"

Example

Suppose we are analyzing the following code, and have set CSHARP_ANALYSIS_ENTRY_POINTS_MODE=LIBRARY in the project configuration file.

using System;
using System.Runtime.CompilerServices;
using com.juliasoft.julia.checkers.guardedBy;

namespace GuardedByTest 
{
  public class GuardedByTest
    {
        [GuardedBy(new string[] { "lock1" })] private int counter;
        private object lock1 = new object();
        private long counter2;
        private static int counter3;
        private static object lock4 = new object();
        private static int counter4 = 13;
        private readonly object[] locks = new object[10];
        private int next;
        private readonly object lock5 = new object();
        private static readonly object lock6 = new object();
        private int counter5;
        public GuardedByTest()
        {
            counter = 1;
            counter5 = 17;
            counter5 += counter;
        }
        [MethodImpl(MethodImplOptions.Synchronized)]
        public void IncrementCounter()
        {
            counter++;
        }
        public void DecrementCounter()
        {
            lock (this)
            {
                counter--;
            }
        }
        public bool CheckCounterIs0()
        {
            object copy = this;
            lock (copy)
            {
                return counter == 0;
            }
        }
        public void IncrementOtherCounter(GuardedByTest other)
        {
            GuardedByTest copy = other;
            lock (copy)
            {
                IncrementCounterAux(other);
            }
        }
        private void IncrementCounterAux([GuardedBy(new string[] { "this" })] GuardedByTest other) // "Unguarded Parameter (C#)" warning issued here
                                                                                                   // - [GuardedBy] annotation differs from that inferred, as discussed below
        {
            other.counter += 2;
        }
        public void IncrementCounter2()
        {
            lock (lock1)
            {
                counter2++;
            }
        }
        public void DecrementCounter2()
        {
            lock (lock1)
            {
                counter2--;
            }
        }
        public bool CheckCounter2Is0()
        {
            object copy = lock1;
            lock (copy)
            {
                return counter2 == 0;
            }
        }
        public void IncrementOtherCounter2(GuardedByTest other)
        {
            GuardedByTest copy = other;
            lock (copy.lock1)
            {
                IncrementCounter2Aux(other);
            }
        }
        private static void IncrementCounter2Aux(GuardedByTest other)
        {
            other.counter2 += 2;
        }
        public void IncrementCounter3()
        {
            lock (typeof(GuardedByTest))
            {
                counter3++;
            }
        }
        [Holding(new string[] { "GuardedByTest.class" })]
        [MethodImpl(MethodImplOptions.Synchronized)]
        public static void DecrementCounter3()
        { // UnguardedMethodOrConstructorWarning
            counter3--;
        }
        public void IncrementCounter4()
        {
            lock (lock4)
            {
                counter4++;
            }
        }
        public static void DecrementCounter4()
        {
            lock (lock4)
            {
                DecrementCounter4Aux();
            }
        }
        [Holding(new string[] { "GuardedByTest.lock4" })]
        private static void DecrementCounter4Aux()
        {
            counter4--;
        }
        private object GetLock()
        {
            return locks[next % locks.Length];
        }
        public void IncrementCounter5()
        {
            lock (GetLock())
            {
                counter5++;
                next++;
            }
        }
        public void DecrementCounter5()
        {
            lock (GetLock())
            {
                DecrementCounter5Aux();
            }
        }
        private void DecrementCounter5Aux()
        {
            counter5--;
        }
        public void AccessLock5()
        {
            lock (lock5)
            {
                lock5.GetHashCode();
            }
        }
        public static void AccessLock6()
        {
            lock (lock6)
            {
                lock6.GetHashCode();
            }
        }
        [MethodImpl(MethodImplOptions.Synchronized)]
        public void ParameterTest1(object o1, string o2, string o3)
        {
            Console.WriteLine(o1);
            lock (lock1)
            {
                Console.WriteLine(o2);
            }
            lock (o3)
            {
                Console.WriteLine(o3 + " ");
            }
        }
    }
}

CodeSonar will infer the following @GuardedBy and @Holding annotations for class GuardedBy.

    field counter:
      type: @GuardedBy("this")
    field counter2:
      type: @GuardedBy("lock")
    field counter3:
      type: @GuardedBy("GuardedByTest.class")
    field counter4:
      type: @GuardedBy("GuardedByTest.lock4")
    field counter5:
      type: @GuardedBy("getLock()")
    field lock5:
      type: @GuardedBy("itself")
    field lock6:
      type: @GuardedBy("itself")
    method DecrementCounter()V:
      receiver: @GuardedBy("itself")
    method DecrementCounter3()V:
    method DecrementCounter4Aux()V: @Holding("GuardedByTest.lock4")
    method DecrementCounter5Aux()V: @Holding("getLock()")
      receiver: @GuardedBy("getLock()")
    method IncrementCounter()V:
      receiver: @GuardedBy("itself")
    method IncrementCounterAux(LGuardedByTest;)V:
      parameter #0:
        type: @GuardedBy("itself")
    method ParameterTest1(LSystem.Object;LSystem.String;LSystem.String;)V:
      receiver: @GuardedBy("itself")
      parameter #0:
        type: @GuardedBy("this")
      parameter #1:
        type: @GuardedBy("lock") @GuardedBy("this")
      parameter #2:
        type: @GuardedBy("itself") @GuardedBy("this")

• Field counter is inferred to be accessed only when its receiver is locked.
  • This does not include the accesses in the constructor, as long as its receiver has not escaped before accessing counter.
  • The analyzer considers definite aliasing in method CheckCounterIs0, since by locking copy one actually locks this as well.
  • The receiver is not always a synonym for the this object of the method. For instance, consider method IncrementCounterAux, where the lock check is performed on other.
• Field counter2 is inferred to be accessed only when the lock field of its receiver is locked. Similar considerations as above for counter apply.
• Field counter3 is inferred to be accessed only when GuardedByTest.class (a System.Type object) is locked. This can occur by explicit synchronization on that object (as in method IncrementCounter3) or through implicit synchronization at call-time (as in the static method DecrementCounter3).
• Field counter4 is static and is inferred to be accessed only when the static field GuardedByTest.lock4 is held. Note that a static field can only be guarded by another static field or a class tag or a static method call.
• Field counter5 is inferred to be locked by the return value of a call to method GetLock() on the same receiver. The analyzer checks that that GetLock() is deterministic (that is, it yields the same value when run from the same initial context) and that its execution does not modify the context that it reads.

  of IncrementCounter5() is reproduced below.

  public void IncrementCounter5()
  {
      lock (GetLock())
      {
          counter5++;
          next++;
      }
  }
  

  This method modifies the value of counter5, then next; the latter of which affects the return value of GetLock(). If the order of these statements were reversed, so that next is modified before the access to counter5, CodeSonar would not infer that counter5 is guarded by the return value of GetLock().

• Fields lock5 and lock6 are always accessed after getting a lock to themselves. Hence they are both guarded by itself, which can be seen here as a shorthand for a field notation.
• Methods DecrementCounter4Aux and DecrementCounter5Aux are also inferred to be guarded by specific locks: that is, those locks are held when the methods are called. It is interesting to observe that DecrementCounter5Aux is guarded because it is private, and hence there cannot be any other call to that method beyond the one in the body of DecrementCounter4(). If the method is made public, the analyzer does not infer it as guarded anymore. Note that guard annotations for methods are reported as @Holding.
• The parameter other of method IncrementCounterAux() is always locked when it is accessed.

  Note that this is different from what the programmer apparently expected: they have explicitly annotated it with [GuardedBy(new string[] { "this" })]. This discrepancy triggers an "Unguarded Parameter (C#)" warning at the IncrementCounterAux() method header.

Resolution

Verify if the missing synchronization should actually be there. Annotate fields and methods with the lock that must be held when they are accessed or called, by using the @GuardedBy and @Holding annotations. If this checker does not accept those annotations, it is likely the case that your program has a synchronization problem.

Relevant Configuration File Parameters

The following configuration file parameters affect checks for this warning class.

• CSHARP_ANALYSIS_CONCURRENCY_CALLS
• CSHARP_ANALYSIS_CONCURRENCY_GUARDS_MODE
• CSHARP_ANALYSIS_DEEP_NULLNESS_CONSERVATIVE_CHECK
• CSHARP_ANALYSIS_FAST_DEEP_CHECK
• CSHARP_ANALYSIS_FIELD_SENSITIVE
• CSHARP_ANALYSIS_INITIALIZATION_CHECK
• CSHARP_ANALYSIS_MERGE_CREATION_POINTS
• REPORT_SIMILAR_WARNINGS
• WARNING_FILTER