New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New rule S6507: Blocks should not be synchronized on local variables #6854
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
<p>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.</p> | ||
<h2>Noncompliant Code Example</h2> | ||
<pre> | ||
private void DoSomething() | ||
{ | ||
object local = new object(); | ||
// Code potentially modifying the local variable ... | ||
|
||
lock (local) // Noncompliant | ||
{ | ||
// ... | ||
} | ||
} | ||
</pre> | ||
<h2>Compliant Solution</h2> | ||
<pre> | ||
private readonly object lockObj = new object(); | ||
|
||
private void DoSomething() | ||
{ | ||
lock (lockObj) | ||
{ | ||
//... | ||
} | ||
} | ||
</pre> | ||
<h2>See</h2> | ||
<ul> | ||
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock">Lock Statement</a> - lock statement - ensure | ||
exclusive access to a shared resource </li> | ||
<li> <a href="https://cwe.mitre.org/data/definitions/412">MITRE, CWE-412</a> - Unrestricted Externally Accessible Lock </li> | ||
<li> <a href="https://cwe.mitre.org/data/definitions/413">MITRE, CWE-413</a> - Improper Resource Locking </li> | ||
</ul> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule); | ||
public override ImmutableArray<DiagnosticDescriptor> 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<ISymbol>(() => 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) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: I'd keep this method and add the Rule as a parameter because it's still shorter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. Otherwise |
||
context.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), message)); | ||
} | ||
|
||
private static bool IsCreation(ExpressionSyntax expression) => | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not relevant for this clone of the rule - can be fixed in RSPEC later, not in this release