diff --git a/analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json b/analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json new file mode 100644 index 00000000000..c9f27b7abe2 --- /dev/null +++ b/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 +} +} +} +] +} diff --git a/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json b/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json new file mode 100644 index 00000000000..c9f27b7abe2 --- /dev/null +++ b/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 +} +} +} +] +} diff --git a/analyzers/its/expected/Nancy/Nancy--net452-S2445.json b/analyzers/its/expected/Nancy/Nancy--net452-S2445.json new file mode 100644 index 00000000000..59c67779f40 --- /dev/null +++ b/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 +} +} +} +] +} diff --git a/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2445.json b/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2445.json new file mode 100644 index 00000000000..59c67779f40 --- /dev/null +++ b/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 +} +} +} +] +} diff --git a/analyzers/rspec/cs/S2445_c#.html b/analyzers/rspec/cs/S2445_c#.html new file mode 100644 index 00000000000..21ef51420e3 --- /dev/null +++ b/analyzers/rspec/cs/S2445_c#.html @@ -0,0 +1,58 @@ +

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.

+
    +
  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 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.
    4. +
+

Noncompliant Code Example

+
+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
+  {
+    // ...
+  }
+}
+
+

Compliant Solution

+
+private Color color = new Color("red");
+private readonly object lockObj = new object();
+
+private void DoSomething()
+{
+  lock (lockObj)
+  {
+    //...
+    color = new Color("green");
+    // ...
+  }
+}
+
+

See

+ + diff --git a/analyzers/rspec/cs/S2445_c#.json b/analyzers/rspec/cs/S2445_c#.json new file mode 100644 index 00000000000..afe23a65374 --- /dev/null +++ b/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" +} diff --git a/analyzers/rspec/cs/Sonar_way_profile.json b/analyzers/rspec/cs/Sonar_way_profile.json index d84e4e17b49..8bee12a45f8 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -99,6 +99,7 @@ "S2386", "S2436", "S2437", + "S2445", "S2479", "S2486", "S2551", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs new file mode 100644 index 00000000000..104b9592a54 --- /dev/null +++ b/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 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(() => 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 lazySymbol) => + expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression) + || lazySymbol.Value.GetSymbolType().Is(KnownType.System_String); + + private static IFieldSymbol FieldWritable(ExpressionSyntax expression, Lazy lazySymbol) => + expression.IsAnyKind(SyntaxKind.IdentifierName, SyntaxKind.SimpleMemberAccessExpression) && lazySymbol.Value is IFieldSymbol lockedField && !lockedField.IsReadOnly + ? lockedField + : null; +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs index 052bc77b511..7d53275d567 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs @@ -2369,7 +2369,7 @@ internal static class RuleTypeMappingCS // ["S2442"], // ["S2443"], // ["S2444"], - // ["S2445"], + ["S2445"] = "BUG", // ["S2446"], // ["S2447"], // ["S2448"], diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBeReadonlyTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBeReadonlyTest.cs new file mode 100644 index 00000000000..f5f3a4b7f5d --- /dev/null +++ b/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(); + + [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(); +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp11.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp11.cs new file mode 100644 index 00000000000..fb473e8e122 --- /dev/null +++ b/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) { } // Error [CS8754] + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp8.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp8.cs new file mode 100644 index 00000000000..21f4b14b44b --- /dev/null +++ b/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 + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.CSharp9.cs new file mode 100644 index 00000000000..f90b56d9fb0 --- /dev/null +++ b/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(); +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.cs new file mode 100644 index 00000000000..18dbd529ef6 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBeReadonly.cs @@ -0,0 +1,216 @@ +using System; +using System.Collections.Generic; +using System.Linq; + +class Test +{ + private static readonly object staticReadonlyField = null; + private static object staticReadWriteField = null; + + private readonly object readonlyField = null; + private readonly string readonlyStringField = "a string"; + private object readWriteField = null; + + private static object StaticReadonlyProperty => null; + private object ReadonlyProperty => null; + + private static object StaticReadWriteProperty { get; set; } + private object ReadWriteProperty { get; set; } + + void OnAStaticField() + { + lock (staticReadonlyField) { } + lock (staticReadWriteField) { } // Noncompliant {{Do not lock on writable field 'staticReadWriteField', use a readonly field instead.}} + // ^^^^^^^^^^^^^^^^^^^^ + lock (Test.staticReadonlyField) { } + lock (Test.staticReadWriteField) { } // Noncompliant {{Do not lock on writable field 'staticReadWriteField', use a readonly field instead.}} + // ^^^^^^^^^^^^^^^^^^^^^^^^^ + lock (AnotherClass.staticReadonlyField) { } + lock (AnotherClass.staticReadWriteField) { } // Noncompliant {{Do not lock on writable field 'staticReadWriteField', use a readonly field instead.}} + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + void OnAFieldOfSameInstance() + { + lock (readonlyField) { } + lock (readonlyStringField) { } // Noncompliant {{Do not lock on strings as they can be interned, use a readonly field instead.}} + lock (readWriteField) { } // Noncompliant {{Do not lock on writable field 'readWriteField', use a readonly field instead.}} + lock (this.readonlyField) { } + lock (this.readWriteField) { } // Noncompliant {{Do not lock on writable field 'readWriteField', use a readonly field instead.}} + } + + void OnParenthesizedExpressions() + { + lock ((readonlyField)) { } + lock ((readWriteField)) { } // Noncompliant {{Do not lock on writable field 'readWriteField', use a readonly field instead.}} + lock ((this.readWriteField)) { } // Noncompliant {{Do not lock on writable field 'readWriteField', use a readonly field instead.}} + } + + void OnAFieldOfDifferentInstance() + { + var anotherInstance = new Test(); + lock (anotherInstance.readonlyField) { } + lock (anotherInstance.readWriteField) { } // Noncompliant {{Do not lock on writable field 'readWriteField', use a readonly field instead.}} + lock (anotherInstance.readonlyField) { } + lock (anotherInstance?.readWriteField) { } // FN: ?. not supported + } + + void OnALocalVariable() + { + object localVarNull = null; + lock (localVarNull) { } // Noncompliant {{Do not lock on local variable 'localVarNull', use a readonly field instead.}} + object localVarReadonlyField = readonlyField; + lock (localVarReadonlyField) { } // Noncompliant, while the local variable references a readonly field, the local variable itself can mutate + object localVarReadWriteField = readWriteField; + lock (localVarReadWriteField) { } // Noncompliant + } + + void OnALocalOutVar(Dictionary lockObjs) + { + if (lockObjs.TryGetValue(42, out var lockObj)) + { + lock (lockObj) { } // Noncompliant, FP: the lock object is a local variable retrieved from a collection of locks + } + } + + void OnANewInstance() + { + lock (new object()) { } // Noncompliant {{Do not lock on a new instance because is a no-op, use a readonly field instead.}} + lock (new ANamespace.AClass()) { } // Noncompliant + lock (new Test[] { }) { } // Noncompliant + lock (new[] { readonlyField }) { } // Noncompliant + lock (new Tuple(readonlyField)) { } // Noncompliant + lock (new { }) { } // Noncompliant + + lock (1) { } // Error [CS0185] + lock ((a: readonlyField, b: readonlyField)) { } // Error [CS0185] + + lock (new ADelegate(x => x)) { } // Noncompliant + lock (new Func(x => x)) { } // Noncompliant + lock (x => x) { } // Error [CS0185] + lock ((int?)1) { } // Error [CS0185] + + lock (from x in new object[2] select x) { } // Noncompliant + } + + void OnAStringInstance() + { + lock ("a string") { } // Noncompliant {{Do not lock on strings as they can be interned, use a readonly field instead.}} + lock ($"an interpolated {"string"}") { } // Noncompliant + lock ("a" + "string") { } // Noncompliant + lock (MethodReturningString()) { } // Noncompliant + + string MethodReturningString() => "a string"; + } + + void OnAssignment() + { + object x; + lock (x = readonlyField) { } + lock (x = readWriteField) { } // FN, assignment not supported + } + + void OtherCases(object oPar, bool bPar, object[] arrayPar) + { + lock (null) { } + + lock (oPar) { } + + lock (this) { } + + lock (SomeMethod()) { } + lock (oPar.GetType()) { } + lock (typeof(Test)) { } + lock (default(Test)) { } + + object SomeMethod() => null; + + lock (StaticReadonlyProperty) { } + lock (ReadonlyProperty) { } + lock (StaticReadWriteProperty) { } + lock (ReadWriteProperty) { } + + lock (bPar ? readWriteField : readonlyField) { } + + lock (oPar ?? readonlyField) { } + lock (oPar = readonlyField) { } + + lock (arrayPar[0]) { } + } + + void ReadWriteReferences() + { + lock (RefReturnReadWriteField(this)) { } // FN, the method returns a readwrite ref to a member + lock (RefReturnStaticReadonlyField(this)) { } // FN, the method returns a readwrite ref to a member + + ref object RefReturnReadWriteField(Test instance) => ref instance.readWriteField; + ref object RefReturnStaticReadonlyField(Test instance) => ref Test.staticReadWriteField; + } + + void NoIdentifier() + { + lock () { } // Error + lock (()) { } // Error + } + + delegate object ADelegate(object oPar); +} + +class TestExplicitCast +{ + private readonly object readonlyField = null; + + void Test() + { + lock ((AnotherClass)readonlyField) { } // Compliant, the cast operator can run arbitrary code + } +} + +class AnotherClass +{ + public static readonly object staticReadonlyField = null; + public static object staticReadWriteField = null; + + public readonly object readonlyField = null; + public object readWriteField = null; + + public static explicit operator AnotherClass(Test o) => new AnotherClass(); +} + +class FieldAccessibily +{ + private readonly object privateField = null; + protected readonly object protectedField = null; + protected internal readonly object protectedInternalField = null; + internal readonly object internalField = null; + public readonly object publicField = null; + + private object PrivateProperty => null; + protected object ProtectedProperty => null; + protected internal object ProtectedInternalProperty => null; + internal object InternalProperty => null; + public object PublicProperty => null; + + void FieldAccessibilityDoesntMatter() + { + lock (privateField) { } + lock (protectedField) { } + lock (protectedInternalField) { } + lock (internalField) { } + lock (publicField) { } + } + + void RuleDoesntRaiseOnProperties() + { + lock (PrivateProperty) { } + lock (ProtectedProperty) { } + lock (ProtectedInternalProperty) { } + lock (InternalProperty) { } + lock (PublicProperty) { } + } +} + +namespace ANamespace +{ + class AClass { } +}