Skip to content
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

Create rule S6507 with "lock on local variable" check moved from S2445 #1608

Merged
merged 6 commits into from Mar 6, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions rules/S2445/comments-and-links.adoc
@@ -1,5 +1,7 @@
=== is related to: S1860

=== is related to: S6507

=== on 5 Feb 2016, 17:54:44 Ann Campbell wrote:
Scope expanded & may now apply to C# [~tamas.vajk]

Expand Down
2 changes: 0 additions & 2 deletions rules/S2445/csharp/message.adoc
Expand Up @@ -2,8 +2,6 @@

Do not lock on writable field "xxx", use a readonly field instead.

Do not lock on local variable "xxx", use a readonly field instead.

Do not lock on a new instance because is a no-op, use a readonly field instead.

Do not lock on strings as they can be interned, use a readonly field instead.
Expand Down
2 changes: 1 addition & 1 deletion rules/S2445/csharp/rule.adoc
Expand Up @@ -2,7 +2,7 @@ Locking on a class field synchronizes not on the field itself, but on the object

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. 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.
2. Locking on a new instance of an object undermines synchronization because two different threads running the same method in parallel will lock on different instances of the same object, allowing them to access the synchronized block at the same time.
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.
Expand Down
5 changes: 5 additions & 0 deletions rules/S6507/comments-and-links.adoc
@@ -0,0 +1,5 @@
=== is related to: S2445

=== on 3 Mar 2022, 10:46:00 Antonio Aversa wrote:
Rule derived from the C# version of S2445, due to this branch of the rule generating a lot of FPs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe explain a bit more why this decision was taken.
It's not only for the FPs - this branch of the rule is still useful if indeed a local variable var local = new Object() for example is used, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, added a couple of paragraphs in my comment, with more details.


4 changes: 4 additions & 0 deletions rules/S6507/csharp/highlighting.adoc
@@ -0,0 +1,4 @@
=== Highlighting

locked object in `lock (xxx)` statement

4 changes: 4 additions & 0 deletions rules/S6507/csharp/message.adoc
@@ -0,0 +1,4 @@
=== Message

Do not lock on local variable "xxx", use a readonly field instead.

3 changes: 3 additions & 0 deletions rules/S6507/csharp/metadata.json
@@ -0,0 +1,3 @@
{

}
56 changes: 56 additions & 0 deletions rules/S6507/csharp/rule.adoc
@@ -0,0 +1,56 @@
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.

== Noncompliant Code Example

[source,csharp]
----
private void DoSomething()
{
object local;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
object local;
object local = new object();

should not compile otherwise.

// ...

lock (local) // Noncompliant
{
// ...
}
}
----


== Compliant Solution

[source,csharp]
----
private readonly object lockObj = new object();

private void DoSomething()
{
lock (lockObj)
{
//...
}
}
----

== See

* https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock[Lock Statement] - lock statement - ensure exclusive access to a shared resource
* https://cwe.mitre.org/data/definitions/412[MITRE, CWE-412] - Unrestricted Externally Accessible Lock
* https://cwe.mitre.org/data/definitions/413[MITRE, CWE-413] - Improper Resource Locking

ifdef::env-github,rspecator-view[]

'''
== Implementation Specification
(visible only on this page)

include::message.adoc[]

include::highlighting.adoc[]

'''
== Comments And Links
(visible only on this page)

include::../comments-and-links.adoc[]
endif::env-github,rspecator-view[]
33 changes: 33 additions & 0 deletions rules/S6507/metadata.json
@@ -0,0 +1,33 @@
{
"title": "Blocks should not be synchronized on local variables",
"type": "BUG",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "15min"
},
"tags": [
"cwe",
"multi-threading"
],
"extra": {
"replacementRules": [

],
"legacyKeys": [

]
},
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-6507",
"sqKey": "S6507",
"scope": "All",
"securityStandards": {
"CWE": [
412,
413
]
},
"defaultQualityProfiles": [ ],
"quickfix": "unknown"
}