diff --git a/analyzers/rspec/cs/S2445_c#.html b/analyzers/rspec/cs/S2445_c#.html index a4cc24ea031..0119f6dfac7 100644 --- a/analyzers/rspec/cs/S2445_c#.html +++ b/analyzers/rspec/cs/S2445_c#.html @@ -1,8 +1,12 @@
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.
+Locking on a readonly
field of a class which is not private
allows external code to lock the field, potentially
+interfering with synchronization by methods of that class.
Locking on a local variable or on a new instance undermines the synchronization: two different threads running the method in parallel would lock on +two different object instances.
+Locking on a string literal is even more dangerous: depending on whether the string is interned or not, different threads may or may not +synchronize on the same object instance.
private string color = "red"; diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs deleted file mode 100644 index 5929b808723..00000000000 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldPrivateReadonly.cs +++ /dev/null @@ -1,61 +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 LockedFieldPrivateReadonly : SonarDiagnosticAnalyzer -{ - private const string DiagnosticId = "S2445"; - - 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 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/LockedFieldShouldBePrivateAndReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs new file mode 100644 index 00000000000..324e02c2a2c --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs @@ -0,0 +1,73 @@ +/* + * 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 LockedFieldShouldBePrivateAndReadonly : 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?.RemoveParentheses(); + if (expression is ObjectCreationExpressionSyntax + or AnonymousObjectCreationExpressionSyntax + or ArrayCreationExpressionSyntax + or ImplicitArrayCreationExpressionSyntax + or QueryExpressionSyntax) + { + ReportIssue("Locking on a new instance is a no-op."); + } + else + { + var type = c.SemanticModel.GetTypeInfo(expression).Type; + + if (expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression) + || (type is not null && type.Is(KnownType.System_String))) + { + ReportIssue("Strings can be interned, and should not be used for locking."); + } + else if (expression is IdentifierNameSyntax + && c.SemanticModel.GetSymbolInfo(expression).Symbol is ILocalSymbol lockedSymbol) + { + ReportIssue($"'{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)) + { + ReportIssue(lockedField.ContainingType is { } lockedFieldType + && c.ContainingSymbol?.ContainingType is { } containingType + && !lockedFieldType.Equals(containingType) + ? $"Use field from '{containingType.ToMinimalDisplayString(c.SemanticModel, expression.SpanStart)}' for locking." + : $"'{lockedField.Name}' is not 'private readonly', and should not be used for locking."); + } + } + + void ReportIssue(string message) => c.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), message)); + }, + SyntaxKind.LockStatement); +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs similarity index 61% rename from analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs rename to analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs index 61cf3ea09f0..ed6a414f847 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldPrivateReadonlyTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs @@ -23,25 +23,32 @@ namespace SonarAnalyzer.UnitTest.Rules; [TestClass] -public class LockedFieldPrivateReadonlyTest +public class LockedFieldShouldBePrivateAndReadonlyTest { - private readonly VerifierBuilder builder = new VerifierBuilder (); + private readonly VerifierBuilder builder = new VerifierBuilder (); [TestMethod] - public void LockedFieldPrivateReadonly_CS() => - builder.AddPaths("LockedFieldPrivateReadonly.cs").Verify(); + public void LockedFieldShouldBePrivateAndReadonly_CS() => + builder.AddPaths("LockedFieldShouldBePrivateAndReadonly.cs").Verify(); [TestMethod] - public void LockedFieldPrivateReadonly_CSharp8() => + public void LockedFieldShouldBePrivateAndReadonly_CSharp8() => builder - .AddPaths("LockedFieldPrivateReadonly.CSharp8.cs") + .AddPaths("LockedFieldShouldBePrivateAndReadonly.CSharp8.cs") .WithOptions(ParseOptionsHelper.FromCSharp8) .Verify(); [TestMethod] - public void LockedFieldPrivateReadonly_CSharp11() => + public void LockedFieldShouldBePrivateAndReadonly_CSharp9() => builder - .AddPaths("LockedFieldPrivateReadonly.CSharp11.cs") + .AddPaths("LockedFieldShouldBePrivateAndReadonly.CSharp9.cs") + .WithOptions(ParseOptionsHelper.FromCSharp9) + .Verify(); + + [TestMethod] + public void LockedFieldShouldBePrivateAndReadonly_CSharp11() => + builder + .AddPaths("LockedFieldShouldBePrivateAndReadonly.CSharp11.cs") .WithOptions(ParseOptionsHelper.FromCSharp11) .Verify(); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp11.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp11.cs similarity index 100% rename from analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp11.cs rename to analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp11.cs diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp8.cs similarity index 80% rename from analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs rename to analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp8.cs index 2bad6133323..ffb6bbd01fe 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.CSharp8.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp8.cs @@ -1,6 +1,4 @@ -using System; - -class Test +class Test { static readonly object staticReadonlyField = null; static object staticReadWriteField = null; @@ -32,13 +30,18 @@ void ReadonlyReferences() void OnANewInstanceOnStack() { - lock (stackalloc int[] { }) { } // Error [CS0185] - lock (stackalloc [] { 1 }) { } // Error [CS0185] + lock (stackalloc int[] { }) { } // Error [CS0185] + lock (stackalloc [] { 1 }) { } // Error [CS0185] } void CoalescingAssignment(object oPar) { - lock (oPar ??= readonlyField) { } // FN, null conditional assignment not supported + 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/LockedFieldShouldBePrivateAndReadonly.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.cs new file mode 100644 index 00000000000..f90b56d9fb0 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.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/LockedFieldPrivateReadonly.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs similarity index 80% rename from analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.cs rename to analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs index 4fb8a46f7d4..8675c562d26 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldPrivateReadonly.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs @@ -24,18 +24,21 @@ void OnAStaticField() 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.}} + lock (AnotherClass.staticReadonlyField) { } // Noncompliant {{Use field from 'Test' for locking.}} + lock (AnotherClass.staticReadWriteField) { } // Noncompliant {{Use field from 'Test' for locking.}} // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } void OnAFieldOfSameInstance() { lock (readonlyField) { } - lock (readonlyStringField) { } + lock ((readonlyField)) { } + lock (readonlyStringField) { } // Noncompliant {{Strings can be interned, and should not be used for locking.}} lock (readWriteField) { } // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}} + 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.}} + lock ((this.readWriteField)) { } // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}} } void OnAFieldOfDifferentInstance() @@ -44,6 +47,7 @@ void OnAFieldOfDifferentInstance() lock (anotherInstance.readonlyField) { } lock (anotherInstance.readWriteField) { } // Noncompliant {{'readWriteField' is not 'private readonly', and should not be used for locking.}} lock (anotherInstance.readonlyField) { } + lock (anotherInstance?.readWriteField) { } // FN: ?. not supported } void OnALocalVariable() @@ -60,13 +64,11 @@ 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 Test[] { }) { } // Noncompliant + lock (new[] { readonlyField }) { } // Noncompliant lock (new Tuple