New Rule S2445: Blocks should be synchronized on read-only fields (#6750

)

analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json

@@ -0,0 +1,17 @@
+{
+"issues":  [
+{
+"id":  "S2445",
+"message":  "Do not lock on local variable 'typeMap', use a readonly field instead.",
+"location":  {
+"uri":  "sources\Automapper\src\AutoMapper\Configuration\MapperConfiguration.cs",
+"region":  {
+"startLine":  256,
+"startColumn":  27,
+"endLine":  256,
+"endColumn":  34
+}
+}
+}
+]
+}

analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json

@@ -0,0 +1,17 @@
+{
+"issues":  [
+{
+"id":  "S2445",
+"message":  "Do not lock on local variable 'typeMap', use a readonly field instead.",
+"location":  {
+"uri":  "sources\Automapper\src\AutoMapper\Configuration\MapperConfiguration.cs",
+"region":  {
+"startLine":  256,
+"startColumn":  27,
+"endLine":  256,
+"endColumn":  34
+}
+}
+}
+]
+}

analyzers/its/expected/Nancy/Nancy--net452-S2445.json

@@ -0,0 +1,17 @@
+{
+"issues":  [
+{
+"id":  "S2445",
+"message":  "Do not lock on writable field 'entitiesLock', use a readonly field instead.",
+"location":  {
+"uri":  "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs",
+"region":  {
+"startLine":  62,
+"startColumn":  23,
+"endLine":  62,
+"endColumn":  35
+}
+}
+}
+]
+}

analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2445.json

@@ -0,0 +1,17 @@
+{
+"issues":  [
+{
+"id":  "S2445",
+"message":  "Do not lock on writable field 'entitiesLock', use a readonly field instead.",
+"location":  {
+"uri":  "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs",
+"region":  {
+"startLine":  62,
+"startColumn":  23,
+"endLine":  62,
+"endColumn":  35
+}
+}
+}
+]
+}

analyzers/rspec/cs/S2445_c#.html

@@ -0,0 +1,58 @@
+<p>Locking on a class field synchronizes not on the field itself, but on the object assigned to it. Thus, there are some good practices to follow to
+avoid bugs related to thread synchronization.</p>
+<ol>
+  <li> Locking on a non-<code>readonly</code> field makes it possible for the field’s value to change while a thread is in the code block locked on
+  the old value. This allows another thread to lock on the new value and access the same block concurrently. </li>
+  <li> Locking on a local variable or a new instance of an object can undermine synchronization because two different threads running the same method
+  in parallel will potentially lock on different instances of the same object, allowing them to access the synchronized block at the same time. </li>
+  <li> Locking on a string literal is also dangerous since, depending on whether the string is interned or not, different threads may or may not
+  synchronize on the same object instance. </li>
+</ol>
+<h2>Noncompliant Code Example</h2>
+<pre>
+private Color color = new Color("red");
+private readonly string colorString = "red";
+
+private void DoSomething()
+{
+  // Synchronizing access via "color"
+  lock (color) // Noncompliant; lock is actually on object instance "red" referred to by the "color" field
+  {
+    //...
+    color = new Color("green"); // other threads now allowed into this block
+    // ...
+  }
+  lock (new object()) // Noncompliant; this is a no-op
+  {
+    // ...
+  }
+  lock (colorString)  // Noncompliant; strings can be interned
+  {
+    // ...
+  }
+}
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+private Color color = new Color("red");
+private readonly object lockObj = new object();
+
+private void DoSomething()
+{
+  lock (lockObj)
+  {
+    //...
+    color = new Color("green");
+    // ...
+  }
+}
+</pre>
+<h2>See</h2>
+<ul>
+  <li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock">Lock Statement</a> - lock statement - ensure
+  exclusive access to a shared resource </li>
+  <li> <a href="https://learn.microsoft.com/en-us/dotnet/api/system.string.intern">String.Intern</a> - <code>String.Intern(String)</code> Method </li>
+  <li> <a href="https://cwe.mitre.org/data/definitions/412">MITRE, CWE-412</a> - Unrestricted Externally Accessible Lock </li>
+  <li> <a href="https://cwe.mitre.org/data/definitions/413">MITRE, CWE-413</a> - Improper Resource Locking </li>
+</ul>
+

analyzers/rspec/cs/S2445_c#.json

@@ -0,0 +1,24 @@
+{
+  "title": "Blocks should be synchronized on read-only fields",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "15min"
+  },
+  "tags": [
+    "cwe",
+    "multi-threading"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-2445",
+  "sqKey": "S2445",
+  "scope": "All",
+  "securityStandards": {
+    "CWE": [
+      412,
+      413
+    ]
+  },
+  "quickfix": "unknown"
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -99,6 +99,7 @@
     "S2386",
     "S2436",
     "S2437",
+    "S2445",
     "S2479",
     "S2486",
     "S2551",

analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs

@@ -0,0 +1,79 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2023 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+namespace SonarAnalyzer.Rules.CSharp;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class LockedFieldShouldBeReadonly : SonarDiagnosticAnalyzer
+{
+    private const string DiagnosticId = "S2445";
+
+    private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, "Do not lock on {0}, use a readonly field instead.");
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+
+    protected override void Initialize(SonarAnalysisContext context) =>
+        context.RegisterNodeAction(CheckLockStatement, SyntaxKind.LockStatement);
+
+    private static void CheckLockStatement(SonarSyntaxNodeReportingContext context)
+    {
+        var expression = ((LockStatementSyntax)context.Node).Expression?.RemoveParentheses();
+        if (IsCreation(expression))
+        {
+            ReportIssue("a new instance because is a no-op");
+        }
+        else
+        {
+            var lazySymbol = new Lazy<ISymbol>(() => context.SemanticModel.GetSymbolInfo(expression).Symbol);
+            if (IsOfTypeString(expression, lazySymbol))
+            {
+                ReportIssue("strings as they can be interned");
+            }
+            else if (expression is IdentifierNameSyntax && lazySymbol.Value is ILocalSymbol localSymbol)
+            {
+                ReportIssue($"local variable '{localSymbol.Name}'");
+            }
+            else if (FieldWritable(expression, lazySymbol) is { } field)
+            {
+                ReportIssue($"writable field '{field.Name}'");
+            }
+        }
+
+        void ReportIssue(string message) =>
+            context.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), message));
+    }
+
+    private static bool IsCreation(ExpressionSyntax expression) =>
+        expression.IsAnyKind(
+            SyntaxKind.ObjectCreationExpression,
+            SyntaxKind.AnonymousObjectCreationExpression,
+            SyntaxKind.ArrayCreationExpression,
+            SyntaxKind.ImplicitArrayCreationExpression,
+            SyntaxKind.QueryExpression);
+
+    private static bool IsOfTypeString(ExpressionSyntax expression, Lazy<ISymbol> lazySymbol) =>
+        expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression)
+        || lazySymbol.Value.GetSymbolType().Is(KnownType.System_String);
+
+    private static IFieldSymbol FieldWritable(ExpressionSyntax expression, Lazy<ISymbol> lazySymbol) =>
+        expression.IsAnyKind(SyntaxKind.IdentifierName, SyntaxKind.SimpleMemberAccessExpression) && lazySymbol.Value is IFieldSymbol lockedField && !lockedField.IsReadOnly
+            ? lockedField
+            : null;
+}

analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs

@@ -2369,7 +2369,7 @@ internal static class RuleTypeMappingCS
         // ["S2442"],
         // ["S2443"],
         // ["S2444"],
-        // ["S2445"],
+        ["S2445"] = "BUG",
         // ["S2446"],
         // ["S2447"],
         // ["S2448"],

analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBeReadonlyTest.cs

@@ -0,0 +1,54 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2023 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+using SonarAnalyzer.Rules.CSharp;
+
+namespace SonarAnalyzer.UnitTest.Rules;
+
+[TestClass]
+public class LockedFieldShouldBeReadonlyTest
+{
+    private readonly VerifierBuilder builder = new VerifierBuilder<LockedFieldShouldBeReadonly>();
+
+    [TestMethod]
+    public void LockedFieldShouldBeReadonly_CS() =>
+        builder.AddPaths("LockedFieldShouldBeReadonly.cs").Verify();
+
+    [TestMethod]
+    public void LockedFieldShouldBeReadonly_CSharp8() =>
+        builder
+            .AddPaths("LockedFieldShouldBeReadonly.CSharp8.cs")
+            .WithOptions(ParseOptionsHelper.FromCSharp8)
+            .Verify();
+
+    [TestMethod]
+    public void LockedFieldShouldBeReadonly_CSharp9() =>
+        builder
+            .AddPaths("LockedFieldShouldBeReadonly.CSharp9.cs")
+            .WithOptions(ParseOptionsHelper.FromCSharp9)
+            .Verify();
+
+    [TestMethod]
+    public void LockedFieldShouldBeReadonly_CSharp11() =>
+        builder
+            .AddPaths("LockedFieldShouldBeReadonly.CSharp11.cs")
+            .WithOptions(ParseOptionsHelper.FromCSharp11)
+            .Verify();
+}

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp11.cs

@@ -0,0 +1,15 @@
+using System;
+
+class Test
+{
+    void OnANewInstance()
+    {
+        lock ("""a raw string literal""") { }                    // Noncompliant
+        lock ($"""an interpolated {"raw string literal"}""") { } // Noncompliant
+    }
+
+    void TargetTypedObjectCreation()
+    {
+        lock (new() as Tuple<int>) { }                           // Error [CS8754]
+    }
+}

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp8.cs

@@ -0,0 +1,45 @@
+class Test
+{
+    static readonly object staticReadonlyField = null;
+
+    readonly object readonlyField = null;
+    object readWriteField = null;
+
+    Test()
+    {
+        ref object refToReadonlyField = ref readonlyField;
+        lock (refToReadonlyField) { }  // Noncompliant, while the reference is to a readonly field, the reference itself is a local variable and as of C# 7.3 can be ref reassigned
+
+        ref object refToReadWriteField = ref readWriteField;
+        lock (refToReadWriteField) { } // Noncompliant
+    }
+
+    void ReadonlyReferences()
+    {
+        lock (RefReturnReadonlyField(this)) { }
+        lock (RefReturnStaticReadonlyField()) { }
+        lock (StaticRefReturnReadonlyField(this)) { }
+        lock (StaticRefReturnStaticReadonlyField()) { }
+
+        ref readonly object RefReturnReadonlyField(Test instance) => ref instance.readonlyField;
+        ref readonly object RefReturnStaticReadonlyField() => ref Test.staticReadonlyField;
+        static ref readonly object StaticRefReturnReadonlyField(Test instance) => ref instance.readonlyField;
+        static ref readonly object StaticRefReturnStaticReadonlyField() => ref Test.staticReadonlyField;
+    }
+
+    void OnANewInstanceOnStack()
+    {
+        lock (stackalloc int[] { }) { }              // Error [CS0185]
+        lock (stackalloc [] { 1 }) { }               // Error [CS0185]
+    }
+
+    void CoalescingAssignment(object oPar)
+    {
+        lock (oPar ??= readonlyField) { }            // FN, null conditional assignment not supported
+    }
+
+    void SwitchExpression(object oPar)
+    {
+        lock (oPar switch { _ => new object() }) { } // FN, switch expression not supported
+    }
+}

analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp9.cs

@@ -0,0 +1,23 @@
+class Test
+{
+    readonly ARecord readonlyField = new();
+    ARecord readWriteField = new();
+
+    static readonly ARecord staticReadonlyField = new();
+    static ARecord staticReadWriteField = new();
+
+    void OnAFieldOfTypeRecord()
+    {
+        lock (readonlyField) { }
+        lock (readWriteField) { }       // Noncompliant
+        lock (staticReadonlyField) { }
+        lock (staticReadWriteField) { } // Noncompliant
+    }
+
+    void OnANewRecordInstance()
+    {
+        lock (new ARecord()) { }  // Noncompliant
+    }
+
+    record ARecord();
+}