From e8123154675eeed1ed15e4d7602f5291e4836e37 Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Mon, 13 Feb 2023 11:33:09 +0100 Subject: [PATCH] Modify rule S2445: Add C# specifics, Move Java specifics --- rules/S2445/csharp/message.adoc | 10 +++++ rules/S2445/csharp/metadata.json | 2 +- rules/S2445/csharp/rule-except-see.adoc | 47 +++++++++++++++++++++ rules/S2445/csharp/rule.adoc | 4 +- rules/S2445/{ => java}/rule-except-see.adoc | 0 rules/S2445/java/rule.adoc | 2 +- rules/S2445/rule.adoc | 2 - 7 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 rules/S2445/csharp/message.adoc create mode 100644 rules/S2445/csharp/rule-except-see.adoc rename rules/S2445/{ => java}/rule-except-see.adoc (100%) diff --git a/rules/S2445/csharp/message.adoc b/rules/S2445/csharp/message.adoc new file mode 100644 index 00000000000..02e7d72473e --- /dev/null +++ b/rules/S2445/csharp/message.adoc @@ -0,0 +1,10 @@ +=== Message + +"xxx" is not "private readonly", and should not be used for locking. + +"xxx" is a local variable, and should not be used for locking. + +Locking on a new instance is a no-op. + +Strings can be interned, and should not be used for locking. + diff --git a/rules/S2445/csharp/metadata.json b/rules/S2445/csharp/metadata.json index 17971333806..3c98a88d67a 100644 --- a/rules/S2445/csharp/metadata.json +++ b/rules/S2445/csharp/metadata.json @@ -1,3 +1,3 @@ { - + "title": "Blocks should be synchronized on \"private readonly\" fields" } diff --git a/rules/S2445/csharp/rule-except-see.adoc b/rules/S2445/csharp/rule-except-see.adoc new file mode 100644 index 00000000000..7c6e5e75329 --- /dev/null +++ b/rules/S2445/csharp/rule-except-see.adoc @@ -0,0 +1,47 @@ +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. + + +== Noncompliant Code Example + +[source,csharp] +---- +private string color = "red"; + +private void DoSomething() +{ + lock (color) // Noncompliant; lock is actually on object instance "red" referred to by the color variable + { + //... + color = "green"; // other threads now allowed into this block + // ... + } + lock (new object()) // Noncompliant this is a no-op. + { + // ... + } +} +---- + + +== Compliant Solution + +[source,csharp] +---- +private string color = "red"; +private readonly object lockObj = new object(); + +private void DoSomething() +{ + lock (lockObj) + { + //... + color = "green"; + // ... + } +} +---- + + diff --git a/rules/S2445/csharp/rule.adoc b/rules/S2445/csharp/rule.adoc index f5db9e87ac5..ff1f6afbfe0 100644 --- a/rules/S2445/csharp/rule.adoc +++ b/rules/S2445/csharp/rule.adoc @@ -1,4 +1,6 @@ -include::../rule.adoc[] +include::rule-except-see.adoc[] + +include::../see.adoc[] ifdef::env-github,rspecator-view[] diff --git a/rules/S2445/rule-except-see.adoc b/rules/S2445/java/rule-except-see.adoc similarity index 100% rename from rules/S2445/rule-except-see.adoc rename to rules/S2445/java/rule-except-see.adoc diff --git a/rules/S2445/java/rule.adoc b/rules/S2445/java/rule.adoc index 00a912506cd..cb515687e5e 100644 --- a/rules/S2445/java/rule.adoc +++ b/rules/S2445/java/rule.adoc @@ -1,4 +1,4 @@ -include::../rule-except-see.adoc[] +include::rule-except-see.adoc[] include::see.adoc[] diff --git a/rules/S2445/rule.adoc b/rules/S2445/rule.adoc index 63e957dbfed..6d518fe68a3 100644 --- a/rules/S2445/rule.adoc +++ b/rules/S2445/rule.adoc @@ -1,3 +1 @@ -include::rule-except-see.adoc[] - include::see.adoc[]