CSHARP.CONCURRENCY.SYNC.MSS : Missing synchronized Statement (C#)

Summary

A synchronized statement is needed to access a field.

Programmers often forget about the consistent use of synchronization statements, which results in fields being accessed without the required lock being held. This checker identifies such situations, even without any explicit annotation by the programmer.

The analyzer uses a statistical approach: if most accesses to a particular field are synchronized then it is likely that all accesses are intended to be synchronized, so a warning will be issued if there is an unsynchronized access to that field.

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="Missing synchronized Statement (C#)"

Example

using System;
using System.Collections.Generic;

namespace GuardedBy 
{
    public class GuardedBy
    {
        private readonly HashSet<string> users = new HashSet<string>();
        public void Register(string user)
        {
            lock (users)
            {
                users.Add(user);
            }
        }
        public void Unregister(string user)
        {
            lock (users)
            {
                users.Remove(user);
            }
        }
        public bool IsRegistered(string user)
        {
            return users.Contains(user); // "Missing synchronized Statement (C#)" warning issued here
                                         // - 'users' is locked for all other accesses of 'user', but not this one.
       }
        public int CountUsers()
        {
            lock (users)
            {
                return users.Count;
            }
        }
    }
}

In this case, the programmer should add a lock on the access to field users inside method IsRegistered() as well. We recall that hashsets are not thread-safe in C# and concurrent access to the same hashset might result in inconsistent behaviors or even deadlock. Another solution, here, would be to use a concurrent set, by exploiting the classes already available in the .NET library. In that case, no lock statement would be needed anymore.

We observe that an automatic static analyzer cannot decide if, in some special situation, a missing lock does not affect correctness. The analyzer can only use in this case a statistical approach: if most accesses are locked, then all accesses are likely to be locked.

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