New rule S6507: Blocks should not be synchronized on local variables (#…

…6854)

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

@@ -1,7 +1,7 @@
 {
 "issues":  [
 {
-"id":  "S2445",
+"id":  "S6507",
 "message":  "Do not lock on local variable 'typeMap', use a readonly field instead.",
 "location":  {
 "uri":  "sources\Automapper\src\AutoMapper\Configuration\MapperConfiguration.cs",

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

@@ -1,7 +1,7 @@
 {
 "issues":  [
 {
-"id":  "S2445",
+"id":  "S6507",
 "message":  "Do not lock on local variable 'typeMap', use a readonly field instead.",
 "location":  {
 "uri":  "sources\Automapper\src\AutoMapper\Configuration\MapperConfiguration.cs",

analyzers/rspec/cs/S2445_c#.html

@@ -3,8 +3,8 @@
 <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 new instance of an object undermines synchronization because two different threads running the same method in parallel will 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>

analyzers/rspec/cs/S6507_c#.html

@@ -0,0 +1,35 @@
+<p>Locking on a local variable 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.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+private void DoSomething()
+{
+  object local = new object();
+  // Code potentially modifying the local variable ...
+
+  lock (local) // Noncompliant
+  {
+    // ...
+  }
+}
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+private readonly object lockObj = new object();
+
+private void DoSomething()
+{
+  lock (lockObj)
+  {
+    //...
+  }
+}
+</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://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/S6507_c#.json

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

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

@@ -23,11 +23,14 @@ namespace SonarAnalyzer.Rules.CSharp;
 [DiagnosticAnalyzer(LanguageNames.CSharp)]
 public sealed class LockedFieldShouldBeReadonly : SonarDiagnosticAnalyzer
 {
-    private const string DiagnosticId = "S2445";
+    private const string LockedFieldDiagnosticId = "S2445";
+    private const string LocalVariableDiagnosticId = "S6507";
+    private const string MessageFormat = "Do not lock on {0}, use a readonly field instead.";
 
-    private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, "Do not lock on {0}, use a readonly field instead.");
+    private static readonly DiagnosticDescriptor LockedFieldRule = DescriptorFactory.Create(LockedFieldDiagnosticId, MessageFormat);
+    private static readonly DiagnosticDescriptor LocalVariableRule = DescriptorFactory.Create(LocalVariableDiagnosticId, MessageFormat);
 
-    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(LockedFieldRule, LocalVariableRule);
 
     protected override void Initialize(SonarAnalysisContext context) =>
         context.RegisterNodeAction(CheckLockStatement, SyntaxKind.LockStatement);
@@ -37,27 +40,24 @@ 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");
+            context.ReportIssue(Diagnostic.Create(LockedFieldRule, expression.GetLocation(), "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");
+                context.ReportIssue(Diagnostic.Create(LockedFieldRule, expression.GetLocation(), "strings as they can be interned"));
             }
             else if (expression is IdentifierNameSyntax && lazySymbol.Value is ILocalSymbol localSymbol)
             {
-                ReportIssue($"local variable '{localSymbol.Name}'");
+                context.ReportIssue(Diagnostic.Create(LocalVariableRule, expression.GetLocation(), $"local variable '{localSymbol.Name}'"));
             }
             else if (FieldWritable(expression, lazySymbol) is { } field)
             {
-                ReportIssue($"writable field '{field.Name}'");
+                context.ReportIssue(Diagnostic.Create(LockedFieldRule, expression.GetLocation(), $"writable field '{field.Name}'"));
             }
         }
-
-        void ReportIssue(string message) =>
-            context.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), message));
     }
 
     private static bool IsCreation(ExpressionSyntax expression) =>

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

@@ -6431,7 +6431,7 @@ internal static class RuleTypeMappingCS
         // ["S6504"],
         // ["S6505"],
         // ["S6506"],
-        // ["S6507"],
+        ["S6507"] = "BUG",
         // ["S6508"],
         // ["S6509"],
         // ["S6510"],