Skip to content

Commit 7addd41

Browse files
calumgrantcalumgrant
committed
C#: Fixes to double-checked lock.
1 parent 937049e commit 7addd41

File tree

3 files changed

+115
-19
lines changed

3 files changed

+115
-19
lines changed

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -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+
not field.getType() instanceof Struct
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: 102 additions & 14 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

3234
// BAD
33-
if (cond1) lock (mutex) if (cond1) { }
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; }
4856

49-
// BAD: FALSE NEGATIVE - not recognized as double-checked lock
57+
58+
// GOOD: not a double-checked lock
5059
if (null == obj1)
5160
{
5261
lock (mutex)
5362
{
54-
if (obj2 == null) { }
63+
if (null == obj2)
64+
obj1 = null;
5565
}
5666
}
5767

58-
// 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.
70+
if (null == obj1)
71+
{
72+
lock (mutex)
73+
{
74+
if (obj1 == null)
75+
obj1 = null;
76+
}
77+
}
78+
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 == 2)
136+
lock (mutex)
137+
if(struct1.x == 2)
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 int 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: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
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:35:9:38:34 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:9:10:9:14 | cond1 | cond1 |
4+
| UnsafeLazyInitialization.cs:80:9:88:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:6:19:6:22 | obj1 | obj1 |
5+
| UnsafeLazyInitialization.cs:98:9:107: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:6:19:6:22 | obj1 | obj1 |
7+
| UnsafeLazyInitialization.cs:110:9:120:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:8:12:8:15 | obj3 | obj3 |
8+
| UnsafeLazyInitialization.cs:135:9:138:34 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:154:16:154:16 | x | x |
9+
| UnsafeLazyInitialization.cs:141:9:148:9 | if (...) ... | Field $@ should be 'volatile' for this double-checked lock. | UnsafeLazyInitialization.cs:12:15:12:19 | pair1 | pair1 |

0 commit comments

Comments
 (0)