From 8dab9119bbdafaa57e40bf98323c2a5575bd201d Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Mon, 6 Mar 2023 13:15:28 +0100 Subject: [PATCH] New rule S6507: Blocks should not be synchronized on local variables (#6854) --- ...445.json => AutoMapper--net461-S6507.json} | 2 +- ... => AutoMapper--netstandard2.0-S6507.json} | 2 +- analyzers/rspec/cs/S2445_c#.html | 4 +-- analyzers/rspec/cs/S6507_c#.html | 35 +++++++++++++++++++ analyzers/rspec/cs/S6507_c#.json | 24 +++++++++++++ .../Rules/LockedFieldShouldBeReadonly.cs | 20 +++++------ .../PackagingTests/RuleTypeMappingCS.cs | 2 +- 7 files changed, 74 insertions(+), 15 deletions(-) rename analyzers/its/expected/Automapper/{AutoMapper--net461-S2445.json => AutoMapper--net461-S6507.json} (94%) rename analyzers/its/expected/Automapper/{AutoMapper--netstandard2.0-S2445.json => AutoMapper--netstandard2.0-S6507.json} (94%) create mode 100644 analyzers/rspec/cs/S6507_c#.html create mode 100644 analyzers/rspec/cs/S6507_c#.json diff --git a/analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json b/analyzers/its/expected/Automapper/AutoMapper--net461-S6507.json similarity index 94% rename from analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json rename to analyzers/its/expected/Automapper/AutoMapper--net461-S6507.json index c9f27b7abe2..cd1d53d403f 100644 --- a/analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json +++ b/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", diff --git a/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json b/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S6507.json similarity index 94% rename from analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json rename to analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S6507.json index c9f27b7abe2..cd1d53d403f 100644 --- a/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json +++ b/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", diff --git a/analyzers/rspec/cs/S2445_c#.html b/analyzers/rspec/cs/S2445_c#.html index 21ef51420e3..36a28fa9354 100644 --- a/analyzers/rspec/cs/S2445_c#.html +++ b/analyzers/rspec/cs/S2445_c#.html @@ -3,8 +3,8 @@
  1. Locking on a non-readonly 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.
    2. -
  2. 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.
    3. +
  3. 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.
  4. 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.
diff --git a/analyzers/rspec/cs/S6507_c#.html b/analyzers/rspec/cs/S6507_c#.html new file mode 100644 index 00000000000..5b61f94376b --- /dev/null +++ b/analyzers/rspec/cs/S6507_c#.html @@ -0,0 +1,35 @@ +

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.

+

Noncompliant Code Example

+
+private void DoSomething()
+{
+  object local = new object();
+  // Code potentially modifying the local variable ...
+
+  lock (local) // Noncompliant
+  {
+    // ...
+  }
+}
+
+

Compliant Solution

+
+private readonly object lockObj = new object();
+
+private void DoSomething()
+{
+  lock (lockObj)
+  {
+    //...
+  }
+}
+
+

See

+ + diff --git a/analyzers/rspec/cs/S6507_c#.json b/analyzers/rspec/cs/S6507_c#.json new file mode 100644 index 00000000000..f01c758da85 --- /dev/null +++ b/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" +} diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs index 104b9592a54..524ac4eea34 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs +++ b/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 SupportedDiagnostics => ImmutableArray.Create(Rule); + public override ImmutableArray 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(() => 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) => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs index 919122b3e4a..a79296bd910 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs @@ -6431,7 +6431,7 @@ internal static class RuleTypeMappingCS // ["S6504"], // ["S6505"], // ["S6506"], - // ["S6507"], + ["S6507"] = "BUG", // ["S6508"], // ["S6509"], // ["S6510"],