From 76a8d598f3c7bcdc8b79704bee75ac55e459ccb2 Mon Sep 17 00:00:00 2001 From: antonioaversa Date: Fri, 3 Mar 2023 09:37:28 +0000 Subject: [PATCH 1/6] Create rule S6507 --- rules/S6507/csharp/metadata.json | 17 +++++++++++++++++ rules/S6507/csharp/rule.adoc | 22 ++++++++++++++++++++++ rules/S6507/metadata.json | 2 ++ 3 files changed, 41 insertions(+) create mode 100644 rules/S6507/csharp/metadata.json create mode 100644 rules/S6507/csharp/rule.adoc create mode 100644 rules/S6507/metadata.json diff --git a/rules/S6507/csharp/metadata.json b/rules/S6507/csharp/metadata.json new file mode 100644 index 00000000000..9142cad3464 --- /dev/null +++ b/rules/S6507/csharp/metadata.json @@ -0,0 +1,17 @@ +{ + "title": "FIXME", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6507", + "sqKey": "S6507", + "scope": "All", + "defaultQualityProfiles": ["Sonar way"], + "quickfix": "unknown" +} diff --git a/rules/S6507/csharp/rule.adoc b/rules/S6507/csharp/rule.adoc new file mode 100644 index 00000000000..52f820374d1 --- /dev/null +++ b/rules/S6507/csharp/rule.adoc @@ -0,0 +1,22 @@ +FIXME: add a description + +// If you want to factorize the description uncomment the following line and create the file. +//include::../description.adoc[] + +== Noncompliant Code Example + +[source,csharp] +---- +FIXME +---- + +== Compliant Solution + +[source,csharp] +---- +FIXME +---- + +== See + +FIXME: A list of links \ No newline at end of file diff --git a/rules/S6507/metadata.json b/rules/S6507/metadata.json new file mode 100644 index 00000000000..2c63c085104 --- /dev/null +++ b/rules/S6507/metadata.json @@ -0,0 +1,2 @@ +{ +} From 3f1211b57bfe47c1d769f968d42dbdec26d1b8ac Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Fri, 3 Mar 2023 11:16:40 +0100 Subject: [PATCH 2/6] Move "local variable" branch from S2445 to S6507 --- rules/S2445/comments-and-links.adoc | 2 ++ rules/S2445/csharp/message.adoc | 2 -- rules/S2445/csharp/rule.adoc | 2 +- rules/S6507/comments-and-links.adoc | 6 ++++ rules/S6507/csharp/highlighting.adoc | 4 +++ rules/S6507/csharp/message.adoc | 4 +++ rules/S6507/csharp/metadata.json | 16 +-------- rules/S6507/csharp/rule.adoc | 49 ++++++++++++++++++++++++---- rules/S6507/metadata.json | 31 ++++++++++++++++++ 9 files changed, 91 insertions(+), 25 deletions(-) create mode 100644 rules/S6507/comments-and-links.adoc create mode 100644 rules/S6507/csharp/highlighting.adoc create mode 100644 rules/S6507/csharp/message.adoc diff --git a/rules/S2445/comments-and-links.adoc b/rules/S2445/comments-and-links.adoc index 8716733989d..940d77d87fa 100644 --- a/rules/S2445/comments-and-links.adoc +++ b/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] diff --git a/rules/S2445/csharp/message.adoc b/rules/S2445/csharp/message.adoc index d1bbd4f3cfe..57bb9a21891 100644 --- a/rules/S2445/csharp/message.adoc +++ b/rules/S2445/csharp/message.adoc @@ -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. diff --git a/rules/S2445/csharp/rule.adoc b/rules/S2445/csharp/rule.adoc index f67ba05c23a..3bc3c328306 100644 --- a/rules/S2445/csharp/rule.adoc +++ b/rules/S2445/csharp/rule.adoc @@ -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. diff --git a/rules/S6507/comments-and-links.adoc b/rules/S6507/comments-and-links.adoc new file mode 100644 index 00000000000..3d537e6cda7 --- /dev/null +++ b/rules/S6507/comments-and-links.adoc @@ -0,0 +1,6 @@ +=== is related to: S2445 + +=== on 3 Mar 2022, 10:46:00 Antonio Aversa wrote: +Rule spawn from the C# version of S2445, due to this branch of the rule generating a lot of FPs. + + diff --git a/rules/S6507/csharp/highlighting.adoc b/rules/S6507/csharp/highlighting.adoc new file mode 100644 index 00000000000..8b4b9efcce8 --- /dev/null +++ b/rules/S6507/csharp/highlighting.adoc @@ -0,0 +1,4 @@ +=== Highlighting + +locked object in `lock (xxx)` statement + diff --git a/rules/S6507/csharp/message.adoc b/rules/S6507/csharp/message.adoc new file mode 100644 index 00000000000..b96de6b2d06 --- /dev/null +++ b/rules/S6507/csharp/message.adoc @@ -0,0 +1,4 @@ +=== Message + +Do not lock on local variable "xxx", use a readonly field instead. + diff --git a/rules/S6507/csharp/metadata.json b/rules/S6507/csharp/metadata.json index 9142cad3464..0db3279e44b 100644 --- a/rules/S6507/csharp/metadata.json +++ b/rules/S6507/csharp/metadata.json @@ -1,17 +1,3 @@ { - "title": "FIXME", - "type": "CODE_SMELL", - "status": "ready", - "remediation": { - "func": "Constant\/Issue", - "constantCost": "5min" - }, - "tags": [ - ], - "defaultSeverity": "Major", - "ruleSpecification": "RSPEC-6507", - "sqKey": "S6507", - "scope": "All", - "defaultQualityProfiles": ["Sonar way"], - "quickfix": "unknown" + } diff --git a/rules/S6507/csharp/rule.adoc b/rules/S6507/csharp/rule.adoc index 52f820374d1..907fe974b39 100644 --- a/rules/S6507/csharp/rule.adoc +++ b/rules/S6507/csharp/rule.adoc @@ -1,22 +1,57 @@ -FIXME: add a description - -// If you want to factorize the description uncomment the following line and create the file. -//include::../description.adoc[] +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] ---- -FIXME +private void DoSomething() +{ + object local; + // ... + + lock (local) // Noncompliant + { + // ... + } +} ---- + == Compliant Solution [source,csharp] ---- -FIXME +private readonly object lockObj = new object(); + +private void DoSomething() +{ + lock (lockObj) + { + //... + } +} ---- == See -FIXME: A list of links \ No newline at end of file +* https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock[Lock Statement] - lock statement - ensure exclusive access to a shared resource +* https://learn.microsoft.com/en-us/dotnet/api/system.string.intern[String.Intern] - `String.Intern(String)` Method +* 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[] diff --git a/rules/S6507/metadata.json b/rules/S6507/metadata.json index 2c63c085104..6f658a329d6 100644 --- a/rules/S6507/metadata.json +++ b/rules/S6507/metadata.json @@ -1,2 +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" } From 9dcb63cc4e0d0b8f8f4f92254757b87c852a4c2a Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Fri, 3 Mar 2023 16:01:09 +0100 Subject: [PATCH 3/6] Code review 1 --- rules/S6507/comments-and-links.adoc | 3 +-- rules/S6507/csharp/rule.adoc | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/rules/S6507/comments-and-links.adoc b/rules/S6507/comments-and-links.adoc index 3d537e6cda7..58c6d6e385c 100644 --- a/rules/S6507/comments-and-links.adoc +++ b/rules/S6507/comments-and-links.adoc @@ -1,6 +1,5 @@ === is related to: S2445 === on 3 Mar 2022, 10:46:00 Antonio Aversa wrote: -Rule spawn from the C# version of S2445, due to this branch of the rule generating a lot of FPs. - +Rule derived from the C# version of S2445, due to this branch of the rule generating a lot of FPs. diff --git a/rules/S6507/csharp/rule.adoc b/rules/S6507/csharp/rule.adoc index 907fe974b39..6f51906f95f 100644 --- a/rules/S6507/csharp/rule.adoc +++ b/rules/S6507/csharp/rule.adoc @@ -35,7 +35,6 @@ private void DoSomething() == 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://learn.microsoft.com/en-us/dotnet/api/system.string.intern[String.Intern] - `String.Intern(String)` Method * 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 From 5cb282b5a58150128bb039256b066db7cd79e4be Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Fri, 3 Mar 2023 16:56:20 +0100 Subject: [PATCH 4/6] Code review 2 --- rules/S6507/comments-and-links.adoc | 2 +- rules/S6507/csharp/rule.adoc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rules/S6507/comments-and-links.adoc b/rules/S6507/comments-and-links.adoc index 58c6d6e385c..244deb8cd49 100644 --- a/rules/S6507/comments-and-links.adoc +++ b/rules/S6507/comments-and-links.adoc @@ -1,5 +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. +Rule derived from the C# version of S2445, due to this branch of the rule generating a lot of FPs. Valid scenarios using local variables include retrieval of the object being locked from a collection or complex logic, to support a fine graned synchronization, renaming of a readonly field in the context of the current method or locking inside a loop, on the iteration variable. diff --git a/rules/S6507/csharp/rule.adoc b/rules/S6507/csharp/rule.adoc index 6f51906f95f..b8668c67a85 100644 --- a/rules/S6507/csharp/rule.adoc +++ b/rules/S6507/csharp/rule.adoc @@ -6,8 +6,8 @@ Locking on a local variable can undermine synchronization because two different ---- private void DoSomething() { - object local; - // ... + object local = new object(); + // Code potentially modifying the local variable ... lock (local) // Noncompliant { From 261d3224d345c329473118225c9f0d88c8ea94f8 Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Fri, 3 Mar 2023 17:06:07 +0100 Subject: [PATCH 5/6] Code review 2 --- rules/S6507/comments-and-links.adoc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rules/S6507/comments-and-links.adoc b/rules/S6507/comments-and-links.adoc index 244deb8cd49..1b585077cf7 100644 --- a/rules/S6507/comments-and-links.adoc +++ b/rules/S6507/comments-and-links.adoc @@ -1,5 +1,8 @@ === 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. Valid scenarios using local variables include retrieval of the object being locked from a collection or complex logic, to support a fine graned synchronization, renaming of a readonly field in the context of the current method or locking inside a loop, on the iteration variable. +Rule derived from the C# version of S2445, due to this branch of the rule generating a lot of FPs. +Valid scenarios using local variables include retrieval of the object being locked from a collection or complex logic, to support a fine graned synchronization, renaming of a readonly field in the context of the current method or locking inside a loop, on the iteration variable. + +The rule still makes sense, however, for all scenarios which don't require advanced synchronization, and prevents synchronization issues captured by S2445 to be circumvented via a local variable. For example via `var local = new object(); lock (local) { ... }`. From 6839e29779c5d8d0a2fb096bd440e2e7ee49dddd Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Mon, 6 Mar 2023 11:55:24 +0100 Subject: [PATCH 6/6] Empty commit to trigger CI