diff --git a/analyzers/rspec/cs/S2445_c#.html b/analyzers/rspec/cs/S2445_c#.html index a5b4b409e90..a4cc24ea031 100644 --- a/analyzers/rspec/cs/S2445_c#.html +++ b/analyzers/rspec/cs/S2445_c#.html @@ -1,30 +1,35 @@ -
Synchronizing on a class field synchronizes not on the field itself, but on the object assigned to it. So synchronizing on a non-final
-field makes it possible for the field’s value to change while a thread is in a block synchronized on the old value. That would allow a second thread,
-synchronized on the new value, to enter the block at the same time.
The story is very similar for synchronizing on parameters; two different threads running the method in parallel could pass two different object -instances in to the method as parameters, completely undermining the synchronization.
+Locking on a class field synchronizes not on the field itself, but on the object assigned to it. So locking on a non-readonly
field
+makes it possible for the field’s value to change while a thread is in a block locked on the old value. That would allow a second thread, locked on
+the new value, to enter the block at the same time.
The story is very similar for locking on a local variable or on a new instance; two different threads running the method in parallel would lock on +two different object instances, completely undermining the synchronization.
-private String color = "red"; +private string color = "red"; -private void doSomething(){ - synchronized(color) { // Noncompliant; lock is actually on object instance "red" referred to by the color variable +private void DoSomething() +{ + lock (color) // Noncompliant; lock is actually on object instance "red" referred to by the color variable + { //... color = "green"; // other threads now allowed into this block // ... } - synchronized(new Object()) { // Noncompliant this is a no-op. + lock (new object()) // Noncompliant this is a no-op. + { // ... } }
-private String color = "red"; -private final Object lockObj = new Object(); +private string color = "red"; +private readonly object lockObj = new object(); -private void doSomething(){ - synchronized(lockObj) { +private void DoSomething() +{ + lock (lockObj) + { //... color = "green"; // ... diff --git a/analyzers/rspec/cs/S2445_c#.json b/analyzers/rspec/cs/S2445_c#.json index c3e5b80de4c..3bad3ab9135 100644 --- a/analyzers/rspec/cs/S2445_c#.json +++ b/analyzers/rspec/cs/S2445_c#.json @@ -1,5 +1,5 @@ { - "title": "Blocks should be synchronized on \"private final\" fields", + "title": "Blocks should be synchronized on \"private readonly\" fields", "type": "BUG", "status": "ready", "remediation": { diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/SynchronizedFieldAssignment.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/SynchronizedFieldAssignment.cs index dd150f9be38..5c82a2ad50f 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/SynchronizedFieldAssignment.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/SynchronizedFieldAssignment.cs @@ -24,20 +24,38 @@ namespace SonarAnalyzer.Rules.CSharp; public sealed class SynchronizedFieldAssignment : SonarDiagnosticAnalyzer { private const string DiagnosticId = "S2445"; - private const string MessageFormat = "FIXME"; - private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, "{0}"); public override ImmutableArraySupportedDiagnostics => ImmutableArray.Create(Rule); protected override void Initialize(SonarAnalysisContext context) => context.RegisterNodeAction(c => { - var node = c.Node; - if (true) + var expression = ((LockStatementSyntax)c.Node).Expression; + if (expression is ObjectCreationExpressionSyntax + or AnonymousObjectCreationExpressionSyntax + or ArrayCreationExpressionSyntax + or ImplicitArrayCreationExpressionSyntax + or QueryExpressionSyntax) { - c.ReportIssue(Diagnostic.Create(Rule, node.GetLocation())); + c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), "Locking on a new instance is a no-op.")); + } + else if (expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression)) + { + c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), "Strings can be interned, and should not be used for locking.")); + } + else if (expression is IdentifierNameSyntax + && c.SemanticModel.GetSymbolInfo(expression).Symbol is ILocalSymbol lockedSymbol) + { + c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), $"'{lockedSymbol.Name}' is a local variable, and should not be used for locking.")); + } + else if (expression is (IdentifierNameSyntax or MemberAccessExpressionSyntax) + && c.SemanticModel.GetSymbolInfo(expression).Symbol is IFieldSymbol lockedField + && !lockedField.IsReadOnly) + { + c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), $"'{lockedField.Name}' is not 'private readonly', and should not be used for locking.")); } }, - SyntaxKind.InvocationExpression); + SyntaxKind.LockStatement); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/SynchronizedFieldAssignmentTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/SynchronizedFieldAssignmentTest.cs index a58f1221e33..f9b43d38f1f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/SynchronizedFieldAssignmentTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/SynchronizedFieldAssignmentTest.cs @@ -30,4 +30,18 @@ public class SynchronizedFieldAssignmentTest [TestMethod] public void SynchronizedFieldAssignment_CS() => builder.AddPaths("SynchronizedFieldAssignment.cs").Verify(); + + [TestMethod] + public void SynchronizedFieldAssignment_CSharp8() => + builder + .AddPaths("SynchronizedFieldAssignment.CSharp8.cs") + .WithOptions(ParseOptionsHelper.FromCSharp8) + .Verify(); + + [TestMethod] + public void SynchronizedFieldAssignment_CSharp11() => + builder + .AddPaths("SynchronizedFieldAssignment.CSharp11.cs") + .WithOptions(ParseOptionsHelper.FromCSharp11) + .Verify(); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.CSharp11.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.CSharp11.cs new file mode 100644 index 00000000000..395faf70089 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.CSharp11.cs @@ -0,0 +1,21 @@ +using System; + +class Test +{ + static readonly object staticReadonlyField = null; + static object staticReadWriteField = null; + + readonly object readonlyField = null; + object readWriteField = null; + + 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/SynchronizedFieldAssignment.CSharp8.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.CSharp8.cs new file mode 100644 index 00000000000..eecdccaccd3 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.CSharp8.cs @@ -0,0 +1,43 @@ +using System; + +class Test +{ + static readonly object staticReadonlyField = null; + static object staticReadWriteField = 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 + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.cs index a790d6d1f92..39b82c8efa6 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/SynchronizedFieldAssignment.cs @@ -1,5 +1,151 @@ using System; +using System.Security.Cryptography.X509Certificates; +using System.Linq; -public class Program +class Test { + static readonly object staticReadonlyField = null; + static object staticReadWriteField = null; + + readonly object readonlyField = null; + readonly string readonlyStringField = "a string"; + object readWriteField = null; + + static object StaticReadonlyProperty => null; + object ReadonlyProperty => null; + + static object StaticReadWriteProperty { get; set; } + object ReadWriteProperty { get; set; } + + void OnAStaticField() + { + lock (staticReadonlyField) { } + lock (staticReadWriteField) { } // Noncompliant {{'staticReadWriteField' is not 'readonly', and should not be used for synchronization.}} + // ^^^^^^^^^^^^^^^^^^^^ + lock (Test.staticReadonlyField) { } + lock (Test.staticReadWriteField) { } // Noncompliant {{'staticReadWriteField' is not 'readonly', and should not be used for synchronization.}} + // ^^^^^^^^^^^^^^^^^^^^^^^^^ + lock (AnotherClass.staticReadonlyField) { } + lock (AnotherClass.staticReadWriteField) { } // Noncompliant {{'staticReadWriteField' is not 'readonly', and should not be used for synchronization.}} + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + void OnAFieldOfSameInstance() + { + lock (readonlyField) { } + lock (readonlyStringField) { } + lock (readWriteField) { } // Noncompliant {{'readWriteField' is not 'readonly', and should not be used for synchronization.}} + lock (this.readonlyField) { } + lock (this.readWriteField) { } // Noncompliant {{'readWriteField' is not 'readonly', and should not be used for synchronization.}} + } + + void OnAFieldOfDifferentInstance() + { + var anotherInstance = new Test(); + lock (anotherInstance.readonlyField) { } + lock (anotherInstance.readWriteField) { } // Noncompliant {{'readWriteField' is not 'readonly', and should not be used for synchronization.}} + lock (anotherInstance.readonlyField) { } + } + + void OnALocalVariable() + { + object localVarNull = null; + lock (localVarNull) { } // Noncompliant {{'localVarNull' is a local variable, and should not be used for synchronization.}} + 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 OnANewInstance() + { + lock (new object()) { } // Noncompliant {{Synchronizing on a new instance is a no-op.}} + lock (new ANamespace.AClass()) { } // Noncompliant + lock (new Test[] { }) // Noncompliant + lock (new[] { readonlyField}) { } // Noncompliant + lock (new Tuple