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/LockedFieldPrivateReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs
new file mode 100644
index 00000000000..5929b808723
--- /dev/null
+++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs
@@ -0,0 +1,61 @@
+/*
+ * 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 LockedFieldPrivateReadonly : SonarDiagnosticAnalyzer
+{
+ private const string DiagnosticId = "S2445";
+
+ private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, "{0}");
+
+ public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule);
+
+ protected override void Initialize(SonarAnalysisContext context) =>
+ context.RegisterNodeAction(c =>
+ {
+ var expression = ((LockStatementSyntax)c.Node).Expression;
+ if (expression is ObjectCreationExpressionSyntax
+ or AnonymousObjectCreationExpressionSyntax
+ or ArrayCreationExpressionSyntax
+ or ImplicitArrayCreationExpressionSyntax
+ or QueryExpressionSyntax)
+ {
+ 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 || lockedField.GetEffectiveAccessibility() != Accessibility.Private))
+ {
+ c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), $"'{lockedField.Name}' is not 'private readonly', and should not be used for locking."));
+ }
+ },
+ SyntaxKind.LockStatement);
+}
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/SynchronizedFieldAssignment.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/SynchronizedFieldAssignment.cs
deleted file mode 100644
index dd150f9be38..00000000000
--- a/analyzers/src/SonarAnalyzer.CSharp/Rules/SynchronizedFieldAssignment.cs
+++ /dev/null
@@ -1,43 +0,0 @@
-/*
- * 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 SynchronizedFieldAssignment : SonarDiagnosticAnalyzer
-{
- private const string DiagnosticId = "S2445";
- private const string MessageFormat = "FIXME";
-
- private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
-
- public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule);
-
- protected override void Initialize(SonarAnalysisContext context) =>
- context.RegisterNodeAction(c =>
- {
- var node = c.Node;
- if (true)
- {
- c.ReportIssue(Diagnostic.Create(Rule, node.GetLocation()));
- }
- },
- SyntaxKind.InvocationExpression);
-}
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/SynchronizedFieldAssignmentTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs
similarity index 60%
rename from analyzers/tests/SonarAnalyzer.UnitTest/Rules/SynchronizedFieldAssignmentTest.cs
rename to analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs
index a58f1221e33..61cf3ea09f0 100644
--- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/SynchronizedFieldAssignmentTest.cs
+++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs
@@ -23,11 +23,25 @@
namespace SonarAnalyzer.UnitTest.Rules;
[TestClass]
-public class SynchronizedFieldAssignmentTest
+public class LockedFieldPrivateReadonlyTest
{
- private readonly VerifierBuilder builder = new VerifierBuilder();
+ private readonly VerifierBuilder builder = new VerifierBuilder();
[TestMethod]
- public void SynchronizedFieldAssignment_CS() =>
- builder.AddPaths("SynchronizedFieldAssignment.cs").Verify();
+ public void LockedFieldPrivateReadonly_CS() =>
+ builder.AddPaths("LockedFieldPrivateReadonly.cs").Verify();
+
+ [TestMethod]
+ public void LockedFieldPrivateReadonly_CSharp8() =>
+ builder
+ .AddPaths("LockedFieldPrivateReadonly.CSharp8.cs")
+ .WithOptions(ParseOptionsHelper.FromCSharp8)
+ .Verify();
+
+ [TestMethod]
+ public void LockedFieldPrivateReadonly_CSharp11() =>
+ builder
+ .AddPaths("LockedFieldPrivateReadonly.CSharp11.cs")
+ .WithOptions(ParseOptionsHelper.FromCSharp11)
+ .Verify();
}
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp11.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp11.cs
new file mode 100644
index 00000000000..395faf70089
--- /dev/null
+++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.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/LockedFieldPrivateReadonly.CSharp8.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs
new file mode 100644
index 00000000000..2bad6133323
--- /dev/null
+++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs
@@ -0,0 +1,56 @@
+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
+ }
+}
+
+class NonPrivateAccessibily
+{
+ private protected readonly object privateProtectedField = null;
+
+ private object PrivateProtectedProperty => null;
+
+ void Test()
+ {
+ lock (privateProtectedField) { } // Noncompliant
+ lock (PrivateProtectedProperty) { } // Compliant, not a field
+ }
+}
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.cs
new file mode 100644
index 00000000000..4fb8a46f7d4
--- /dev/null
+++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.cs
@@ -0,0 +1,180 @@
+using System;
+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 {{'staticReadWriteField' is not 'private readonly', and should not be used for locking.}}
+ // ^^^^^^^^^^^^^^^^^^^^
+ lock (Test.staticReadonlyField) { }
+ lock (Test.staticReadWriteField) { } // Noncompliant {{'staticReadWriteField' is not 'private readonly', and should not be used for locking.}}
+ // ^^^^^^^^^^^^^^^^^^^^^^^^^
+ lock (AnotherClass.staticReadonlyField) { } // Noncompliant {{'staticReadonlyField' is not 'private readonly', and should not be used for locking.}}
+ lock (AnotherClass.staticReadWriteField) { } // Noncompliant {{'staticReadWriteField' is not 'private readonly', and should not be used for locking.}}
+ // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+ }
+
+ void OnAFieldOfSameInstance()
+ {
+ lock (readonlyField) { }
+ lock (readonlyStringField) { }
+ lock (readWriteField) { } // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}}
+ lock (this.readonlyField) { }
+ lock (this.readWriteField) { } // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}}
+ }
+
+ void OnAFieldOfDifferentInstance()
+ {
+ var anotherInstance = new Test();
+ lock (anotherInstance.readonlyField) { }
+ lock (anotherInstance.readWriteField) { } // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}}
+ lock (anotherInstance.readonlyField) { }
+ }
+
+ void OnALocalVariable()
+ {
+ object localVarNull = null;
+ lock (localVarNull) { } // Noncompliant {{'localVarNull' is a local variable, and should not be used for locking.}}
+ 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 {{Locking on a new instance is a no-op.}}
+ lock (new ANamespace.AClass()) { } // Noncompliant
+ lock (new Test[] { }) // Noncompliant
+ lock (new[] { readonlyField}) { } // Noncompliant
+ lock (new Tuple