From 863b7861f602deb372179c1bd0bdeb48ec7ad67b Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Wed, 1 Mar 2023 11:52:37 +0100 Subject: [PATCH] Modify rule S2445: Add C# description and samples (#1570) --- rules/S2445/csharp/highlighting.adoc | 4 ++ rules/S2445/csharp/message.adoc | 10 ++++ rules/S2445/csharp/metadata.json | 2 +- rules/S2445/csharp/rule.adoc | 66 ++++++++++++++++++++++-- rules/S2445/{ => java}/highlighting.adoc | 0 rules/S2445/{ => java}/message.adoc | 0 rules/S2445/java/metadata.json | 1 + rules/S2445/java/rule.adoc | 45 ++++++++++++++-- rules/S2445/metadata.json | 2 +- rules/S2445/rule-except-see.adoc | 42 --------------- rules/S2445/rule.adoc | 3 -- rules/S2445/see.adoc | 5 -- 12 files changed, 122 insertions(+), 58 deletions(-) create mode 100644 rules/S2445/csharp/highlighting.adoc create mode 100644 rules/S2445/csharp/message.adoc rename rules/S2445/{ => java}/highlighting.adoc (100%) rename rules/S2445/{ => java}/message.adoc (100%) delete mode 100644 rules/S2445/rule-except-see.adoc delete mode 100644 rules/S2445/rule.adoc delete mode 100644 rules/S2445/see.adoc diff --git a/rules/S2445/csharp/highlighting.adoc b/rules/S2445/csharp/highlighting.adoc new file mode 100644 index 00000000000..8b4b9efcce8 --- /dev/null +++ b/rules/S2445/csharp/highlighting.adoc @@ -0,0 +1,4 @@ +=== Highlighting + +locked object in `lock (xxx)` statement + diff --git a/rules/S2445/csharp/message.adoc b/rules/S2445/csharp/message.adoc new file mode 100644 index 00000000000..d1bbd4f3cfe --- /dev/null +++ b/rules/S2445/csharp/message.adoc @@ -0,0 +1,10 @@ +=== Message + +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/metadata.json b/rules/S2445/csharp/metadata.json index 17971333806..49d1a202ff1 100644 --- a/rules/S2445/csharp/metadata.json +++ b/rules/S2445/csharp/metadata.json @@ -1,3 +1,3 @@ { - + } diff --git a/rules/S2445/csharp/rule.adoc b/rules/S2445/csharp/rule.adoc index f5db9e87ac5..f67ba05c23a 100644 --- a/rules/S2445/csharp/rule.adoc +++ b/rules/S2445/csharp/rule.adoc @@ -1,4 +1,64 @@ -include::../rule.adoc[] +Locking on a class field synchronizes not on the field itself, but on the object assigned to it. Thus, there are some good practices to follow to avoid bugs related to thread synchronization. + +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. + +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. + + +== Noncompliant Code Example + +[source,csharp] +---- +private Color color = new Color("red"); +private readonly string colorString = "red"; + +private void DoSomething() +{ + // Synchronizing access via "color" + lock (color) // Noncompliant; lock is actually on object instance "red" referred to by the "color" field + { + //... + color = new Color("green"); // other threads now allowed into this block + // ... + } + lock (new object()) // Noncompliant; this is a no-op + { + // ... + } + lock (colorString) // Noncompliant; strings can be interned + { + // ... + } +} +---- + + +== Compliant Solution + +[source,csharp] +---- +private Color color = new Color("red"); +private readonly object lockObj = new object(); + +private void DoSomething() +{ + lock (lockObj) + { + //... + color = new Color("green"); + // ... + } +} +---- + +== 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 ifdef::env-github,rspecator-view[] @@ -6,9 +66,9 @@ ifdef::env-github,rspecator-view[] == Implementation Specification (visible only on this page) -include::../message.adoc[] +include::message.adoc[] -include::../highlighting.adoc[] +include::highlighting.adoc[] ''' == Comments And Links diff --git a/rules/S2445/highlighting.adoc b/rules/S2445/java/highlighting.adoc similarity index 100% rename from rules/S2445/highlighting.adoc rename to rules/S2445/java/highlighting.adoc diff --git a/rules/S2445/message.adoc b/rules/S2445/java/message.adoc similarity index 100% rename from rules/S2445/message.adoc rename to rules/S2445/java/message.adoc diff --git a/rules/S2445/java/metadata.json b/rules/S2445/java/metadata.json index 04bfbe5dd7e..86c7c0912a6 100644 --- a/rules/S2445/java/metadata.json +++ b/rules/S2445/java/metadata.json @@ -1,4 +1,5 @@ { + "title": "Blocks should be synchronized on \"private final\" fields", "tags": [ "cwe", "multi-threading", diff --git a/rules/S2445/java/rule.adoc b/rules/S2445/java/rule.adoc index 00a912506cd..a6088c8cf26 100644 --- a/rules/S2445/java/rule.adoc +++ b/rules/S2445/java/rule.adoc @@ -1,4 +1,43 @@ -include::../rule-except-see.adoc[] +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. + + +== Noncompliant Code Example + +[source,text] +---- +private String color = "red"; + +private void doSomething(){ + synchronized(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. + // ... + } +} +---- + + +== Compliant Solution + +[source,text] +---- +private String color = "red"; +private final Object lockObj = new Object(); + +private void doSomething(){ + synchronized(lockObj) { + //... + color = "green"; + // ... + } +} +---- include::see.adoc[] @@ -8,9 +47,9 @@ ifdef::env-github,rspecator-view[] == Implementation Specification (visible only on this page) -include::../message.adoc[] +include::message.adoc[] -include::../highlighting.adoc[] +include::highlighting.adoc[] ''' == Comments And Links diff --git a/rules/S2445/metadata.json b/rules/S2445/metadata.json index b8a30cd3bf3..9f9b77408fd 100644 --- a/rules/S2445/metadata.json +++ b/rules/S2445/metadata.json @@ -1,5 +1,5 @@ { - "title": "Blocks should be synchronized on \"private final\" fields", + "title": "Blocks should be synchronized on read-only fields", "type": "BUG", "status": "ready", "remediation": { diff --git a/rules/S2445/rule-except-see.adoc b/rules/S2445/rule-except-see.adoc deleted file mode 100644 index 719bdd5ed60..00000000000 --- a/rules/S2445/rule-except-see.adoc +++ /dev/null @@ -1,42 +0,0 @@ -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. - - -== Noncompliant Code Example - -[source,text] ----- -private String color = "red"; - -private void doSomething(){ - synchronized(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. - // ... - } -} ----- - - -== Compliant Solution - -[source,text] ----- -private String color = "red"; -private final Object lockObj = new Object(); - -private void doSomething(){ - synchronized(lockObj) { - //... - color = "green"; - // ... - } -} ----- - - diff --git a/rules/S2445/rule.adoc b/rules/S2445/rule.adoc deleted file mode 100644 index 63e957dbfed..00000000000 --- a/rules/S2445/rule.adoc +++ /dev/null @@ -1,3 +0,0 @@ -include::rule-except-see.adoc[] - -include::see.adoc[] diff --git a/rules/S2445/see.adoc b/rules/S2445/see.adoc deleted file mode 100644 index d1d8dd9b68b..00000000000 --- a/rules/S2445/see.adoc +++ /dev/null @@ -1,5 +0,0 @@ -== See - -* 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 -