Skip to content

Commit 135271e

Browse files
hvitvedhvitved
authored
Merge pull request #287 from calumgrant/cs/lock-order
C#: Improvements to cs/inconsistent-lock-sequence
2 parents 7702b58 + 8c29d0e commit 135271e

File tree

9 files changed

+188
-35
lines changed

9 files changed

+188
-35
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@
1212

1313
## Changes to existing queries
1414

15-
| **Query** | **Expected impact** | **Change** |
16-
|----------------------------|------------------------|------------------------------------------------------------------|
15+
| Inconsistent lock sequence (`cs/inconsistent-lock-sequence`) | More results | This query now finds inconsistent lock sequences globally across calls. |
16+
1717
| *@name of query (Query ID)*| *Impact on results* | *How/why the query has changed* |
1818

1919

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@ variables in the same sequence.</p>
1717
<p>The following example shows a program running two threads, which deadlocks because <code>thread1</code> holds <code>lock1</code> and
1818
is waiting to acquire <code>lock2</code>, whilst <code>thread2</code> holds <code>lock2</code> and is waiting to acquire <code>lock1</code>.</p>
1919

20-
<sample src="LockOrder.cs" />
20+
<sample src="LockOrderBad.cs" />
2121

2222
<p>This problem is resolved by reordering the <code>lock</code> variables as shown below.</p>
23-
<sample src="LockOrderFix.cs" />
23+
<sample src="LockOrderGood.cs" />
2424
</example>
2525

2626
<references>

‎csharp/ql/src/Concurrency/LockOrder.ql‎

Lines changed: 47 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,50 @@
1313

1414
import csharp
1515

16-
from LockStmt l1, LockStmt l2, LockStmt l3, LockStmt l4, Variable v1, Variable v2
17-
where l1.getALockedStmt()=l2
18-
and l3.getALockedStmt()=l4
19-
and l1.getLockVariable()=v1
20-
and l2.getLockVariable()=v2
21-
and l3.getLockVariable()=v2
22-
and l4.getLockVariable()=v1
23-
and v1!=v2
24-
select l4, "Inconsistent lock sequence. The locks " + v1 + " and " + v2 + " are locked in a different sequence $@.",
25-
l2, "here"
16+
/**
17+
* Gets a call target conservatively only when there is
18+
* one runtime target.
19+
*/
20+
Callable getCallTarget(Call c) {
21+
count(c.getARuntimeTarget()) = 1 and
22+
result = c.getARuntimeTarget()
23+
}
24+
25+
/** Gets a lock statement reachable from a callable. */
26+
LockStmt getAReachableLockStmt(Callable callable) {
27+
result.getEnclosingCallable() = callable
28+
or
29+
exists(Call call | call.getEnclosingCallable() = callable |
30+
result = getAReachableLockStmt(getCallTarget(call))
31+
)
32+
}
33+
34+
/**
35+
* Holds if there is nested pairs of lock statements, either
36+
* inter-procedurally or intra-procedurally.
37+
*/
38+
predicate nestedLocks(Variable outerVariable, Variable innerVariable, LockStmt outer, LockStmt inner) {
39+
outerVariable = outer.getLockVariable() and
40+
innerVariable = inner.getLockVariable() and
41+
outerVariable != innerVariable and (
42+
inner = outer.getALockedStmt()
43+
or
44+
exists(Call call | call.getEnclosingStmt() = outer.getALockedStmt() |
45+
inner = getAReachableLockStmt(getCallTarget(call))
46+
) and
47+
outerVariable.(Modifiable).isStatic() and
48+
innerVariable.(Modifiable).isStatic()
49+
)
50+
}
51+
52+
from LockStmt outer1, LockStmt inner1, LockStmt outer2, LockStmt inner2, Variable v1, Variable v2
53+
where
54+
nestedLocks(v1, v2, outer1, inner1) and
55+
nestedLocks(v2, v1, outer2, inner2) and
56+
v1.getName() <= v2.getName()
57+
select v1, "Inconsistent lock sequence with $@. Lock sequences $@, $@ and $@, $@ found.",
58+
v2, v2.getName(),
59+
outer1, v1.getName(),
60+
inner1, v2.getName(),
61+
outer2, v2.getName(),
62+
inner2, v1.getName()

csharp/ql/src/Concurrency/LockOrder.cs renamed to csharp/ql/src/Concurrency/LockOrderBad.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
using System;
2+
using System.Threading;
3+
14
class Deadlock
25
{
36
private readonly Object lock1 = new Object();

csharp/ql/src/Concurrency/LockOrderFix.cs renamed to csharp/ql/src/Concurrency/LockOrderGood.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
using System;
2+
using System.Threading;
3+
14
class DeadlockFixed
25
{
36
private readonly Object lock1 = new Object();

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

Lines changed: 59 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,66 @@
11
using System;
22

3-
class Foo
3+
class LocalTest
44
{
5+
// BAD: b is flagged.
56
Object a, b, c;
67

7-
void f()
8-
{
9-
lock (a) lock (b) { }
10-
lock (b) lock (a) { }
11-
lock (b) lock (a) { }
12-
lock (a) lock (b) { }
13-
lock (b) lock (c) { }
14-
lock (c) lock (b) { }
15-
lock (a) lock (a) { }
16-
lock (a) lock (a) { }
8+
void F()
9+
{
10+
lock (a) lock (b) lock(c) { }
11+
}
12+
13+
void G()
14+
{
15+
lock (a) lock (c) lock(b) { }
16+
}
17+
18+
void H()
19+
{
20+
lock (a) lock(c) { }
1721
}
1822
}
23+
24+
class GlobalTest
25+
{
26+
// BAD: b is flagged.
27+
static Object a, b, c;
28+
29+
void F()
30+
{
31+
lock (a) G();
32+
}
33+
34+
void G()
35+
{
36+
lock (b) H();
37+
lock (c) I();
38+
}
39+
40+
void H()
41+
{
42+
lock (c) { }
43+
}
44+
45+
void I()
46+
{
47+
lock (b) { }
48+
}
49+
}
50+
51+
class LambdaTest
52+
{
53+
// BAD: a is flagged.
54+
static Object a, b;
55+
56+
void F()
57+
{
58+
Action lock_a = () => { lock(a) { } };
59+
Action lock_b = () => { lock(b) { } };
60+
61+
lock(a) lock_b();
62+
lock(b) lock_a();
63+
}
64+
}
65+
66+
// semmle-extractor-options: /r:System.Runtime.Extensions.dll /r:System.Threading.dll /r:System.Threading.Thread.dll

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

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,4 @@
1-
| LockOrder.cs:9:18:9:29 | lock (...) {...} | Inconsistent lock sequence. The locks b and a are locked in a different sequence $@. | LockOrder.cs:10:18:10:29 | lock (...) {...} | here |
2-
| LockOrder.cs:9:18:9:29 | lock (...) {...} | Inconsistent lock sequence. The locks b and a are locked in a different sequence $@. | LockOrder.cs:11:18:11:29 | lock (...) {...} | here |
3-
| LockOrder.cs:10:18:10:29 | lock (...) {...} | Inconsistent lock sequence. The locks a and b are locked in a different sequence $@. | LockOrder.cs:9:18:9:29 | lock (...) {...} | here |
4-
| LockOrder.cs:10:18:10:29 | lock (...) {...} | Inconsistent lock sequence. The locks a and b are locked in a different sequence $@. | LockOrder.cs:12:18:12:29 | lock (...) {...} | here |
5-
| LockOrder.cs:11:18:11:29 | lock (...) {...} | Inconsistent lock sequence. The locks a and b are locked in a different sequence $@. | LockOrder.cs:9:18:9:29 | lock (...) {...} | here |
6-
| LockOrder.cs:11:18:11:29 | lock (...) {...} | Inconsistent lock sequence. The locks a and b are locked in a different sequence $@. | LockOrder.cs:12:18:12:29 | lock (...) {...} | here |
7-
| LockOrder.cs:12:18:12:29 | lock (...) {...} | Inconsistent lock sequence. The locks b and a are locked in a different sequence $@. | LockOrder.cs:10:18:10:29 | lock (...) {...} | here |
8-
| LockOrder.cs:12:18:12:29 | lock (...) {...} | Inconsistent lock sequence. The locks b and a are locked in a different sequence $@. | LockOrder.cs:11:18:11:29 | lock (...) {...} | here |
9-
| LockOrder.cs:13:18:13:29 | lock (...) {...} | Inconsistent lock sequence. The locks c and b are locked in a different sequence $@. | LockOrder.cs:14:18:14:29 | lock (...) {...} | here |
10-
| LockOrder.cs:14:18:14:29 | lock (...) {...} | Inconsistent lock sequence. The locks b and c are locked in a different sequence $@. | LockOrder.cs:13:18:13:29 | lock (...) {...} | here |
1+
| LockOrder.cs:6:15:6:15 | b | Inconsistent lock sequence with $@. Lock sequences $@, $@ and $@, $@ found. | LockOrder.cs:6:18:6:18 | c | c | LockOrder.cs:10:18:10:37 | lock (...) {...} | b | LockOrder.cs:10:27:10:37 | lock (...) {...} | c | LockOrder.cs:15:18:15:37 | lock (...) {...} | c | LockOrder.cs:15:27:15:37 | lock (...) {...} | b |
2+
| LockOrder.cs:27:22:27:22 | b | Inconsistent lock sequence with $@. Lock sequences $@, $@ and $@, $@ found. | LockOrder.cs:27:25:27:25 | c | c | LockOrder.cs:36:8:36:20 | lock (...) {...} | b | LockOrder.cs:42:9:42:20 | lock (...) {...} | c | LockOrder.cs:37:8:37:20 | lock (...) {...} | c | LockOrder.cs:47:9:47:20 | lock (...) {...} | b |
3+
| LockOrder.cs:54:19:54:19 | a | Inconsistent lock sequence with $@. Lock sequences $@, $@ and $@, $@ found. | LockOrder.cs:54:22:54:22 | b | b | LockOrder.cs:61:9:61:25 | lock (...) {...} | a | LockOrder.cs:59:33:59:43 | lock (...) {...} | b | LockOrder.cs:62:9:62:25 | lock (...) {...} | b | LockOrder.cs:58:33:58:43 | lock (...) {...} | a |
4+
| LockOrderBad.cs:6:29:6:33 | lock1 | Inconsistent lock sequence with $@. Lock sequences $@, $@ and $@, $@ found. | LockOrderBad.cs:7:29:7:33 | lock2 | lock2 | LockOrderBad.cs:11:9:19:9 | lock (...) {...} | lock1 | LockOrderBad.cs:16:13:18:13 | lock (...) {...} | lock2 | LockOrderBad.cs:24:9:32:9 | lock (...) {...} | lock2 | LockOrderBad.cs:29:13:31:13 | lock (...) {...} | lock1 |

‎csharp/ql/test/query-tests/Concurrency/LockOrder/LockOrderBad.cs‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
using System;
2+
using System.Threading;
3+
4+
class Deadlock
5+
{
6+
private readonly Object lock1 = new Object();
7+
private readonly Object lock2 = new Object();
8+
9+
public void thread1()
10+
{
11+
lock (lock1)
12+
{
13+
Console.Out.WriteLine("Thread 1 acquired lock1");
14+
Thread.Sleep(10);
15+
Console.Out.WriteLine("Thread 1 waiting on lock2");
16+
lock (lock2) // Deadlock here
17+
{
18+
}
19+
}
20+
}
21+
22+
public void thread2()
23+
{
24+
lock (lock2)
25+
{
26+
Console.Out.WriteLine("Thread 2 acquired lock2");
27+
Thread.Sleep(10);
28+
Console.Out.WriteLine("Thread 2 waiting on lock1");
29+
lock (lock1) // Deadlock here
30+
{
31+
}
32+
}
33+
}
34+
}

‎csharp/ql/test/query-tests/Concurrency/LockOrder/LockOrderGood.cs‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
using System;
2+
using System.Threading;
3+
4+
class DeadlockFixed
5+
{
6+
private readonly Object lock1 = new Object();
7+
private readonly Object lock2 = new Object();
8+
9+
public void thread1()
10+
{
11+
lock (lock1)
12+
{
13+
Console.Out.WriteLine("Thread 1 acquired lock1");
14+
Thread.Sleep(10);
15+
Console.Out.WriteLine("Thread 1 waiting on lock2");
16+
lock (lock2)
17+
{
18+
}
19+
}
20+
}
21+
22+
public void thread2()
23+
{
24+
lock (lock1) // Fixed
25+
{
26+
Console.Out.WriteLine("Thread 2 acquired lock1");
27+
Thread.Sleep(10);
28+
Console.Out.WriteLine("Thread 2 waiting on lock2");
29+
lock (lock2) // Fixed
30+
{
31+
}
32+
}
33+
}
34+
}

0 commit comments

Comments
 (0)