Skip to content

Commit a382a58

Browse files
hvitvedhvitved
authored
Merge pull request #808 from calumgrant/cs/double-checked-locks
C#: Work on cs/unsafe-double-checked-lock
2 parents f5e419e + 40f3b8b commit a382a58

File tree

5 files changed

+147
-47
lines changed

5 files changed

+147
-47
lines changed

‎change-notes/1.20/analysis-csharp.md‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,15 @@
1111

1212
| *@name of query (Query ID)* | *Impact on results* | *How/why the query has changed* |
1313
|------------------------------|------------------------|-----------------------------------|
14-
| Off-by-one comparison against container length (cs/index-out-of-bounds) | Fewer false positives | Results have been removed when there are additional guards on the index. |
15-
| Dereferenced variable is always null (cs/dereferenced-value-is-always-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
16-
| Dereferenced variable may be null (cs/dereferenced-value-may-be-null) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
17-
| SQL query built from user-controlled sources (cs/sql-injection), Improper control of generation of code (cs/code-injection), Uncontrolled format string (cs/uncontrolled-format-string), Clear text storage of sensitive information (cs/cleartext-storage-of-sensitive-information), Exposure of private information (cs/exposure-of-sensitive-information) | More results | Data sources have been added from user controls in `System.Windows.Forms`. |
18-
| Use of default ToString() (cs/call-to-object-tostring) | Fewer false positives | Results have been removed for `char` arrays passed to `StringBuilder.Append()`, which were incorrectly marked as using `ToString`. |
19-
| Use of default ToString() (cs/call-to-object-tostring) | Fewer results | Results have been removed when the object is an interface or an abstract class. |
20-
| Unused format argument (cs/format-argument-unused) | Fewer false positives | Results have been removed where the format string is empty. This is often used as a default value and is not an interesting result. |
21-
14+
| Off-by-one comparison against container length (`cs/index-out-of-bounds`) | Fewer false positives | Results have been removed when there are additional guards on the index. |
15+
| Dereferenced variable is always null (`cs/dereferenced-value-is-always-null`) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
16+
| Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | Improved results | The query has been rewritten from scratch, and the analysis is now based on static single assignment (SSA) forms. The query is now enabled by default in LGTM. |
17+
| SQL query built from user-controlled sources (`cs/sql-injection`), Improper control of generation of code (`cs/code-injection`), Uncontrolled format string (`cs/uncontrolled-format-string`), Clear text storage of sensitive information (`cs/cleartext-storage-of-sensitive-information`), Exposure of private information (`cs/exposure-of-sensitive-information`) | More results | Data sources have been added from user controls in `System.Windows.Forms`. |
18+
| Use of default ToString() (`cs/call-to-object-tostring`) | Fewer false positives | Results have been removed for `char` arrays passed to `StringBuilder.Append()`, which were incorrectly marked as using `ToString`. |
19+
| Use of default ToString() (`cs/call-to-object-tostring`) | Fewer results | Results have been removed when the object is an interface or an abstract class. |
20+
| Unused format argument (`cs/format-argument-unused`) | Fewer false positives | Results have been removed where the format string is empty. This is often used as a default value and is not an interesting result. |
21+
| Double-checked lock is not thread-safe (`cs/unsafe-double-checked-lock`) | Fewer false positives, more true positives | Results have been removed where the underlying field was not updated in the `lock` statement, or where the field is a `struct`. Results have been added where there are other statements inside the `lock` statement. |
22+
2223
## Changes to code extraction
2324

2425
* Fix extraction of `for` statements where the condition declares new variables using `is`.

‎csharp/ql/src/Concurrency/UnsafeLazyInitialization.qhelp‎

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
22
<qhelp>
33
<overview>
4-
<p>Double-checked locking requires that the underlying field is <code>volatile</code>,
5-
otherwise the program can behave incorrectly when running in multiple threads,
4+
<p>Double-checked locking requires that the underlying field is <code>volatile</code>,
5+
otherwise the program can behave incorrectly when running in multiple threads,
66
for example by computing the field twice.
77
</p>
88
</overview>
@@ -16,6 +16,7 @@ for example by computing the field twice.
1616
<li>Make the field volatile using the <code>volatile</code> keyword.</li>
1717
<li>Use the <code>System.Lazy</code> class, which is guaranteed to be thread-safe.
1818
This can often lead to more elegant code.</li>
19+
<li>Use <code>System.Threading.LazyInitializer</code>.</li>
1920
</ol>
2021

2122
</recommendation>
@@ -51,19 +52,23 @@ automatically thread-safe (Recommendation 3):</p>
5152
</example>
5253

5354
<references>
54-
55-
<li>
56-
MSDN: <a href="https://msdn.microsoft.com/en-us/library/ff650316.aspx">Implementing Singleton in C#</a>.
57-
</li>
58-
<li>
59-
MSDN Magazine: <a href="https://msdn.microsoft.com/magazine/jj863136">The C# Memory Model in Theory and Practice</a>.
60-
</li>
61-
<li>
62-
MSDN, C# Reference: <a href="https://msdn.microsoft.com/en-us/library/x13ttww7.aspx">volatile</a>.
63-
</li>
64-
<li>
65-
Wikipedia: <a href="https://en.wikipedia.org/wiki/Double-checked_locking">Double-checked locking</a>.
66-
</li>
67-
55+
<li>
56+
MSDN: <a href="https://docs.microsoft.com/en-us/dotnet/api/system.lazy-1">Lazy&lt;T&gt; Class</a>.
57+
</li>
58+
<li>
59+
MSDN: <a href="https://docs.microsoft.com/en-us/dotnet/api/system.threading.lazyinitializer.ensureinitialized">LazyInitializer.EnsureInitialized Method</a>.
60+
</li>
61+
<li>
62+
MSDN: <a href="https://msdn.microsoft.com/en-us/library/ff650316.aspx">Implementing Singleton in C#</a>.
63+
</li>
64+
<li>
65+
MSDN Magazine: <a href="https://msdn.microsoft.com/magazine/jj863136">The C# Memory Model in Theory and Practice</a>.
66+
</li>
67+
<li>
68+
MSDN, C# Reference: <a href="https://msdn.microsoft.com/en-us/library/x13ttww7.aspx">volatile</a>.
69+
</li>
70+
<li>
71+
Wikipedia: <a href="https://en.wikipedia.org/wiki/Double-checked_locking">Double-checked locking</a>.
72+
</li>
6873
</references>
6974
</qhelp>

‎csharp/ql/src/Concurrency/UnsafeLazyInitialization.ql‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
/**
22
* @name Double-checked lock is not thread-safe
3-
* @description A repeated check on a non-volatile field is not thread-safe, and
4-
* could result in unexpected behavior.
3+
* @description A repeated check on a non-volatile field is not thread-safe on some platforms,
4+
* and could result in unexpected behavior.
55
* @kind problem
66
* @problem.severity error
7-
* @precision high
7+
* @precision medium
88
* @id cs/unsafe-double-checked-lock
99
* @tags correctness
1010
* concurrency
@@ -22,7 +22,7 @@ class DoubleCheckedLock extends StructuralComparisonConfiguration {
2222
x = unlockedIf.getCondition() and
2323
y = lockedIf.getCondition() and
2424
lock = unlockedIf.getThen().stripSingletonBlocks() and
25-
lockedIf = lock.getBlock().stripSingletonBlocks()
25+
lockedIf.getParent*() = lock.getBlock()
2626
)
2727
}
2828
}
@@ -38,5 +38,7 @@ predicate doubleCheckedLock(Field field, IfStmt ifs) {
3838
from Field field, IfStmt ifs
3939
where
4040
doubleCheckedLock(field, ifs) and
41-
not field.isVolatile()
41+
not field.isVolatile() and
42+
exists(VariableWrite write | write = ifs.getThen().getAChild+() and write.getTarget() = field) and
43+
field.getType() instanceof RefType
4244
select ifs, "Field $@ should be 'volatile' for this double-checked lock.", field, field.getName()

‎csharp/ql/test/query-tests/Concurrency/UnsafeLazyInitialization/UnsafeLazyInitialization.cs‎

Lines changed: 103 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
1+
using System.Collections.Generic;
12

23
class Program
34
{
45
static object mutex = new object();
56
static object obj1;
67
static volatile object obj2;
7-
static bool cond1;
8-
static volatile bool cond2;
8+
object obj3;
9+
bool cond1;
10+
volatile bool cond2;
11+
Coord struct1, struct2;
12+
(int,int) pair1;
913

10-
static void Main(string[] args)
14+
void Fn()
1115
{
1216
// BAD
1317
if (obj1 == null)
@@ -16,7 +20,7 @@ static void Main(string[] args)
1620
{
1721
if (obj1 == null)
1822
{
19-
// ...
23+
obj1 = null;
2024
}
2125
}
2226
}
@@ -25,12 +29,13 @@ static void Main(string[] args)
2529
if (obj1 == null)
2630
lock (mutex)
2731
if (obj1 == null)
28-
{
29-
// ...
30-
}
32+
obj1 = null;
3133

32-
// BAD
33-
if (cond1) lock (mutex) if (cond1) { }
34+
// GOOD: A value-type
35+
if (cond1)
36+
lock (mutex)
37+
if (cond1)
38+
cond1 = false;
3439

3540
// GOOD: volatile
3641
if (obj2 == null)
@@ -39,34 +44,117 @@ static void Main(string[] args)
3944
{
4045
if (obj2 == null)
4146
{
47+
obj2 = null;
4248
}
4349
}
4450
}
4551

4652
// GOOD: volatile
47-
if (cond2) lock (mutex) if (cond2) { }
53+
if (cond2)
54+
lock (mutex)
55+
if (cond2) { cond2 = false; }
56+
57+
58+
// GOOD: not a double-checked lock
59+
if (null == obj1)
60+
{
61+
lock (mutex)
62+
{
63+
if (null == obj2)
64+
obj1 = null;
65+
}
66+
}
4867

49-
// BAD: FALSE NEGATIVE - not recognized as double-checked lock
68+
// BAD (false-positive): not a double-checked lock
69+
// as the condition is not the same.
5070
if (null == obj1)
5171
{
5272
lock (mutex)
5373
{
54-
if (obj2 == null) { }
74+
if (obj1 == null)
75+
obj1 = null;
5576
}
5677
}
5778

58-
// BAD: FALSE NEGATIVE - not recognized as double-checked lock
79+
// BAD
5980
if (null == obj1)
6081
{
6182
lock (mutex)
6283
{
6384
int x;
64-
if (obj2 == null) { }
85+
if (null == obj1)
86+
obj1 = null;
6587
}
6688
}
6789

6890
// GOOD: not a field
6991
object a = null;
70-
if (a == null) lock (mutex) if (a == null) { }
92+
if (a == null)
93+
lock (mutex)
94+
if (a == null)
95+
a = new object();
96+
97+
// BAD: only obj1 is flagged.
98+
if (obj1 == null && obj2 == null)
99+
{
100+
lock (mutex)
101+
{
102+
if (obj1 == null && obj2 == null)
103+
{
104+
obj1 = null;
105+
}
106+
}
107+
}
108+
109+
// BAD: both obj1 and obj3 are flagged.
110+
if (obj1 == null && obj3 == null)
111+
{
112+
lock (mutex)
113+
{
114+
if (obj1 == null && obj3 == null)
115+
{
116+
obj1 = null;
117+
obj3 = null;
118+
}
119+
}
120+
}
121+
122+
// GOOD: Locking a struct
123+
if (struct1 == struct2)
124+
{
125+
lock(mutex)
126+
{
127+
if (struct1 == struct2)
128+
{
129+
struct1 = new Coord();
130+
}
131+
}
132+
}
133+
134+
// BAD: Field x should be volatile
135+
if (struct1.x is null)
136+
lock (mutex)
137+
if(struct1.x is null)
138+
struct1.x = 3;
139+
140+
// GOOD: Tuples are structs so cannot be volatile.
141+
if(pair1 == (1,2))
142+
{
143+
lock(mutex)
144+
{
145+
if(pair1 == (1,2))
146+
pair1 = (2,3);
147+
}
148+
}
71149
}
72150
}
151+
152+
struct Coord
153+
{
154+
public object x, y;
155+
156+
public static bool operator==(Coord c1, Coord c2) => c1.x==c2.x && c1.y == c2.y;
157+
public static bool operator!=(Coord c1, Coord c2) => !(c1==c2);
158+
}
159+
160+
// semmle-extractor-options: -langversion:latest

‎csharp/ql/test/query-tests/Concurrency/UnsafeLazyInitialization/UnsafeLazyInitialization.expected‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1-
| UnsafeLazyInitialization.cs:13:9:22:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:5:19:5:22 | obj1 | obj1 |
2-
| UnsafeLazyInitialization.cs:25:9:30:17 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:5:19:5:22 | obj1 | obj1 |
3-
| UnsafeLazyInitialization.cs:33:9:33:46 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:7:17:7:21 | cond1 | cond1 |
1+
| UnsafeLazyInitialization.cs:17:9:26:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:6:19:6:22 | obj1 | obj1 |
2+
| UnsafeLazyInitialization.cs:29:9:32:32 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:6:19:6:22 | obj1 | obj1 |
3+
| UnsafeLazyInitialization.cs:80:9:88:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:6:19:6:22 | obj1 | obj1 |
4+
| UnsafeLazyInitialization.cs:98:9:107:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:6:19:6:22 | obj1 | obj1 |
5+
| UnsafeLazyInitialization.cs:110:9:120:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:6:19:6:22 | obj1 | obj1 |
6+
| UnsafeLazyInitialization.cs:110:9:120:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:8:12:8:15 | obj3 | obj3 |
7+
| UnsafeLazyInitialization.cs:135:9:138:34 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:154:19:154:19 | x | x |

0 commit comments

Comments
 (0)