Skip to content

Commit dbee8fc

Browse files
owen-mcowen-mc
committed
Inline isCloseCall into isSink 
1 parent c87bfd5 commit dbee8fc

6 files changed

Lines changed: 434 additions & 39 deletions

File tree

‎go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql‎

Lines changed: 37 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -101,44 +101,6 @@ predicate postDominatesNode(ControlFlow::Node postDominator, ControlFlow::Node n
101101
)
102102
}
103103

104-
/**
105-
* Holds if `closeCall` is an unhandled call to `os.File.Close` on `sink` that is not
106-
* guaranteed to be preceded during execution by a handled call to `os.File.Sync` on the
107-
* same file handle.
108-
*/
109-
predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
110-
// find calls to the os.File.Close function
111-
closeCall = any(CloseFileFun f).getACall() and
112-
// that are unhandled
113-
unhandledCall(closeCall) and
114-
// where the function is called on the sink
115-
closeCall.getReceiver() = sink and
116-
// and check that the call to `os.File.Close` is not guaranteed to be preceded during
117-
// execution by a handled call to `os.File.Sync` on the same file handle.
118-
not exists(DataFlow::Node syncReceiver, DataFlow::CallNode syncCall |
119-
// check that the call to `os.File.Sync` is handled
120-
isHandledSync(syncReceiver, syncCall) and
121-
// check that `os.File.Sync` is called on the same object as `os.File.Close`
122-
exists(DataFlow::SsaNode ssa | ssa.getAUse() = sink and ssa.getAUse() = syncReceiver)
123-
|
124-
if exists(DeferStmt defer | defer.getCall() = closeCall.asExpr())
125-
then
126-
// When the call to `os.File.Close` is deferred it runs when the enclosing function
127-
// returns, but the receiver of the deferred call is evaluated where the `defer`
128-
// statement appears. It is therefore enough for the handled call to `os.File.Sync`
129-
// to post-dominate that point, since that guarantees `os.File.Sync` runs before the
130-
// deferred `os.File.Close` on every path on which the `os.File.Close` is registered.
131-
// We cannot reuse the domination check below because the control-flow graph splices
132-
// the deferred call in at the function exit, where it may be reachable along paths
133-
// that do not pass through the call to `os.File.Sync`.
134-
postDominatesNode(syncCall.asInstruction(), sink.asInstruction())
135-
else
136-
// Otherwise the call to `os.File.Close` is executed where it appears, so we require
137-
// the handled call to `os.File.Sync` to dominate it.
138-
syncCall.asInstruction().dominatesNode(closeCall.asInstruction())
139-
)
140-
}
141-
142104
/**
143105
* Holds if `os.File.Sync` is called on `sink` and the result of the call is neither
144106
* deferred nor discarded.
@@ -155,7 +117,43 @@ predicate isHandledSync(DataFlow::Node sink, DataFlow::CallNode syncCall) {
155117
module UnhandledFileCloseConfig implements DataFlow::ConfigSig {
156118
predicate isSource(DataFlow::Node source) { isWritableFileHandle(source, _) }
157119

158-
predicate isSink(DataFlow::Node sink) { isCloseSink(sink, _) }
120+
predicate isSink(DataFlow::Node sink) {
121+
// `closeCall` is an unhandled call to `os.File.Close` on `sink` that is not
122+
// guaranteed to be preceded during execution by a handled call to `os.File.Sync` on the
123+
// same file handle.
124+
exists(DataFlow::CallNode closeCall |
125+
// find calls to the os.File.Close function
126+
closeCall = any(CloseFileFun f).getACall() and
127+
// that are unhandled
128+
unhandledCall(closeCall) and
129+
// where the function is called on the sink
130+
closeCall.getReceiver() = sink and
131+
// and check that the call to `os.File.Close` is not guaranteed to be preceded during
132+
// execution by a handled call to `os.File.Sync` on the same file handle.
133+
not exists(DataFlow::Node syncReceiver, DataFlow::CallNode syncCall |
134+
// check that the call to `os.File.Sync` is handled
135+
isHandledSync(syncReceiver, syncCall) and
136+
// check that `os.File.Sync` is called on the same object as `os.File.Close`
137+
exists(DataFlow::SsaNode ssa | ssa.getAUse() = sink and ssa.getAUse() = syncReceiver)
138+
|
139+
if exists(DeferStmt defer | defer.getCall() = closeCall.asExpr())
140+
then
141+
// When the call to `os.File.Close` is deferred it runs when the enclosing function
142+
// returns, but the receiver of the deferred call is evaluated where the `defer`
143+
// statement appears. It is therefore enough for the handled call to `os.File.Sync`
144+
// to post-dominate that point, since that guarantees `os.File.Sync` runs before the
145+
// deferred `os.File.Close` on every path on which the `os.File.Close` is registered.
146+
// We cannot reuse the domination check below because the control-flow graph splices
147+
// the deferred call in at the function exit, where it may be reachable along paths
148+
// that do not pass through the call to `os.File.Sync`.
149+
postDominatesNode(syncCall.asInstruction(), sink.asInstruction())
150+
else
151+
// Otherwise the call to `os.File.Close` is executed where it appears, so we require
152+
// the handled call to `os.File.Sync` to dominate it.
153+
syncCall.asInstruction().dominatesNode(closeCall.asInstruction())
154+
)
155+
)
156+
}
159157

160158
predicate observeDiffInformedIncrementalMode() { any() }
161159

‎go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/DebugDefer.ql‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import go
2+
3+
from ControlFlow::Node n, ControlFlow::Node succ, FuncDecl fd
4+
where
5+
n.getRoot() = fd.getFuncDef() and
6+
fd.getName() =
7+
["deferredCloseWithSync", "returnedSync", "copyFile", "deferredCloseWithSyncEarlyReturn"] and
8+
succ = n.getASuccessor()
9+
select fd.getName(), n.toString(),
10+
n.getLocation().getStartLine() + ":" + n.getLocation().getStartColumn(), "->", succ.toString(),
11+
succ.getLocation().getStartLine() + ":" + succ.getLocation().getStartColumn()

‎go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/DebugDefer2.expected‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
| copyFile | 161:1 entry | -> | 161:15 argument corresponding to destFile |
2+
| copyFile | 161:6 skip | -> | 161:1 function declaration |
3+
| copyFile | 161:15 argument corresponding to destFile | -> | 161:15 initialization of destFile |
4+
| copyFile | 161:15 initialization of destFile | -> | 161:32 argument corresponding to mode |
5+
| copyFile | 161:32 argument corresponding to mode | -> | 161:32 initialization of mode |
6+
| copyFile | 161:32 initialization of mode | -> | 161:50 argument corresponding to src |
7+
| copyFile | 161:50 argument corresponding to src | -> | 161:50 initialization of src |
8+
| copyFile | 161:50 initialization of src | -> | 162:2 skip |
9+
| copyFile | 162:2 ... := ...[0] | -> | 162:2 assignment to f |
10+
| copyFile | 162:2 ... := ...[1] | -> | 162:5 assignment to err |
11+
| copyFile | 162:2 assignment to f | -> | 162:2 ... := ...[1] |
12+
| copyFile | 162:2 skip | -> | 162:5 skip |
13+
| copyFile | 162:5 assignment to err | -> | 163:5 err |
14+
| copyFile | 162:5 skip | -> | 162:12 selection of OpenFile |
15+
| copyFile | 162:12 call to OpenFile | -> | 161:1 exit |
16+
| copyFile | 162:12 call to OpenFile | -> | 162:2 ... := ...[0] |
17+
| copyFile | 162:12 selection of OpenFile | -> | 162:24 destFile |
18+
| copyFile | 162:24 destFile | -> | 162:34 ...\|... |
19+
| copyFile | 162:34 ...\|... | -> | 162:70 mode |
20+
| copyFile | 162:70 mode | -> | 162:12 call to OpenFile |
21+
| copyFile | 163:5 ...!=... | -> | 161:1 exit |
22+
| copyFile | 163:5 ...!=... | -> | 163:5 ...!=... is false |
23+
| copyFile | 163:5 ...!=... | -> | 163:5 ...!=... is true |
24+
| copyFile | 163:5 ...!=... is false | -> | 166:8 f |
25+
| copyFile | 163:5 ...!=... is true | -> | 164:10 err |
26+
| copyFile | 163:5 err | -> | 163:12 nil |
27+
| copyFile | 163:12 nil | -> | 163:5 ...!=... |
28+
| copyFile | 164:3 return statement | -> | 161:1 exit |
29+
| copyFile | 164:10 err | -> | 164:3 return statement |
30+
| copyFile | 166:2 defer statement | -> | 168:2 skip |
31+
| copyFile | 166:8 call to Close | -> | 161:1 exit |
32+
| copyFile | 166:8 f | -> | 166:8 selection of Close |
33+
| copyFile | 166:8 selection of Close | -> | 166:2 defer statement |
34+
| copyFile | 168:2 ... = ...[0] | -> | 168:2 ... = ...[1] |
35+
| copyFile | 168:2 ... = ...[1] | -> | 168:5 assignment to err |
36+
| copyFile | 168:2 skip | -> | 168:5 skip |
37+
| copyFile | 168:5 assignment to err | -> | 169:5 err |
38+
| copyFile | 168:5 skip | -> | 168:11 selection of Copy |
39+
| copyFile | 168:11 call to Copy | -> | 166:8 call to Close |
40+
| copyFile | 168:11 call to Copy | -> | 168:2 ... = ...[0] |
41+
| copyFile | 168:11 selection of Copy | -> | 168:19 f |
42+
| copyFile | 168:19 f | -> | 168:22 src |
43+
| copyFile | 168:22 src | -> | 168:11 call to Copy |
44+
| copyFile | 169:5 ...!=... | -> | 166:8 call to Close |
45+
| copyFile | 169:5 ...!=... | -> | 169:5 ...!=... is false |
46+
| copyFile | 169:5 ...!=... | -> | 169:5 ...!=... is true |
47+
| copyFile | 169:5 ...!=... is false | -> | 172:9 f |
48+
| copyFile | 169:5 ...!=... is true | -> | 170:10 err |
49+
| copyFile | 169:5 err | -> | 169:12 nil |
50+
| copyFile | 169:12 nil | -> | 169:5 ...!=... |
51+
| copyFile | 170:3 return statement | -> | 166:8 call to Close |
52+
| copyFile | 170:10 err | -> | 170:3 return statement |
53+
| copyFile | 172:2 return statement | -> | 166:8 call to Close |
54+
| copyFile | 172:9 call to Sync | -> | 166:8 call to Close |
55+
| copyFile | 172:9 call to Sync | -> | 172:2 return statement |
56+
| copyFile | 172:9 f | -> | 172:9 selection of Sync |
57+
| copyFile | 172:9 selection of Sync | -> | 172:9 call to Sync |
58+
| deferredCloseWithSync | 94:1 entry | -> | 96:5 skip |
59+
| deferredCloseWithSync | 94:6 skip | -> | 94:1 function declaration |
60+
| deferredCloseWithSync | 96:5 ... := ...[0] | -> | 96:5 assignment to f |
61+
| deferredCloseWithSync | 96:5 ... := ...[1] | -> | 96:8 assignment to err |
62+
| deferredCloseWithSync | 96:5 assignment to f | -> | 96:5 ... := ...[1] |
63+
| deferredCloseWithSync | 96:5 skip | -> | 96:8 skip |
64+
| deferredCloseWithSync | 96:8 assignment to err | -> | 96:81 err |
65+
| deferredCloseWithSync | 96:8 skip | -> | 96:15 selection of OpenFile |
66+
| deferredCloseWithSync | 96:15 call to OpenFile | -> | 94:1 exit |
67+
| deferredCloseWithSync | 96:15 call to OpenFile | -> | 96:5 ... := ...[0] |
68+
| deferredCloseWithSync | 96:15 selection of OpenFile | -> | 96:27 "foo.txt" |
69+
| deferredCloseWithSync | 96:27 "foo.txt" | -> | 96:38 ...\|... |
70+
| deferredCloseWithSync | 96:38 ...\|... | -> | 96:74 0666 |
71+
| deferredCloseWithSync | 96:74 0666 | -> | 96:15 call to OpenFile |
72+
| deferredCloseWithSync | 96:81 ...!=... | -> | 94:1 exit |
73+
| deferredCloseWithSync | 96:81 ...!=... | -> | 96:81 ...!=... is false |
74+
| deferredCloseWithSync | 96:81 ...!=... | -> | 96:81 ...!=... is true |
75+
| deferredCloseWithSync | 96:81 ...!=... is false | -> | 94:1 exit |
76+
| deferredCloseWithSync | 96:81 ...!=... is true | -> | 99:9 f |
77+
| deferredCloseWithSync | 96:81 err | -> | 96:88 nil |
78+
| deferredCloseWithSync | 96:88 nil | -> | 96:81 ...!=... |
79+
| deferredCloseWithSync | 99:3 defer statement | -> | 101:6 skip |
80+
| deferredCloseWithSync | 99:9 call to Close | -> | 94:1 exit |
81+
| deferredCloseWithSync | 99:9 f | -> | 99:9 selection of Close |
82+
| deferredCloseWithSync | 99:9 selection of Close | -> | 99:3 defer statement |
83+
| deferredCloseWithSync | 101:6 assignment to err | -> | 101:23 err |
84+
| deferredCloseWithSync | 101:6 skip | -> | 101:13 f |
85+
| deferredCloseWithSync | 101:13 call to Sync | -> | 99:9 call to Close |
86+
| deferredCloseWithSync | 101:13 call to Sync | -> | 101:6 assignment to err |
87+
| deferredCloseWithSync | 101:13 f | -> | 101:13 selection of Sync |
88+
| deferredCloseWithSync | 101:13 selection of Sync | -> | 101:13 call to Sync |
89+
| deferredCloseWithSync | 101:23 ...!=... | -> | 99:9 call to Close |
90+
| deferredCloseWithSync | 101:23 ...!=... | -> | 101:23 ...!=... is false |
91+
| deferredCloseWithSync | 101:23 ...!=... | -> | 101:23 ...!=... is true |
92+
| deferredCloseWithSync | 101:23 ...!=... is false | -> | 99:9 call to Close |
93+
| deferredCloseWithSync | 101:23 ...!=... is true | -> | 102:4 selection of Fatal |
94+
| deferredCloseWithSync | 101:23 err | -> | 101:30 nil |
95+
| deferredCloseWithSync | 101:30 nil | -> | 101:23 ...!=... |
96+
| deferredCloseWithSync | 102:4 call to Fatal | -> | 99:9 call to Close |
97+
| deferredCloseWithSync | 102:4 selection of Fatal | -> | 102:14 err |
98+
| deferredCloseWithSync | 102:14 err | -> | 102:4 call to Fatal |
99+
| deferredCloseWithSyncEarlyReturn | 122:1 entry | -> | 122:39 argument corresponding to n |
100+
| deferredCloseWithSyncEarlyReturn | 122:6 skip | -> | 122:1 function declaration |
101+
| deferredCloseWithSyncEarlyReturn | 122:39 argument corresponding to n | -> | 122:39 initialization of n |
102+
| deferredCloseWithSyncEarlyReturn | 122:39 initialization of n | -> | 124:5 skip |
103+
| deferredCloseWithSyncEarlyReturn | 124:5 ... := ...[0] | -> | 124:5 assignment to f |
104+
| deferredCloseWithSyncEarlyReturn | 124:5 ... := ...[1] | -> | 124:8 assignment to err |
105+
| deferredCloseWithSyncEarlyReturn | 124:5 assignment to f | -> | 124:5 ... := ...[1] |
106+
| deferredCloseWithSyncEarlyReturn | 124:5 skip | -> | 124:8 skip |
107+
| deferredCloseWithSyncEarlyReturn | 124:8 assignment to err | -> | 124:81 err |
108+
| deferredCloseWithSyncEarlyReturn | 124:8 skip | -> | 124:15 selection of OpenFile |
109+
| deferredCloseWithSyncEarlyReturn | 124:15 call to OpenFile | -> | 122:1 exit |
110+
| deferredCloseWithSyncEarlyReturn | 124:15 call to OpenFile | -> | 124:5 ... := ...[0] |
111+
| deferredCloseWithSyncEarlyReturn | 124:15 selection of OpenFile | -> | 124:27 "foo.txt" |
112+
| deferredCloseWithSyncEarlyReturn | 124:27 "foo.txt" | -> | 124:38 ...\|... |
113+
| deferredCloseWithSyncEarlyReturn | 124:38 ...\|... | -> | 124:74 0666 |
114+
| deferredCloseWithSyncEarlyReturn | 124:74 0666 | -> | 124:15 call to OpenFile |
115+
| deferredCloseWithSyncEarlyReturn | 124:81 ...!=... | -> | 122:1 exit |
116+
| deferredCloseWithSyncEarlyReturn | 124:81 ...!=... | -> | 124:81 ...!=... is false |
117+
| deferredCloseWithSyncEarlyReturn | 124:81 ...!=... | -> | 124:81 ...!=... is true |
118+
| deferredCloseWithSyncEarlyReturn | 124:81 ...!=... is false | -> | 122:1 exit |
119+
| deferredCloseWithSyncEarlyReturn | 124:81 ...!=... is true | -> | 126:9 f |
120+
| deferredCloseWithSyncEarlyReturn | 124:81 err | -> | 124:88 nil |
121+
| deferredCloseWithSyncEarlyReturn | 124:88 nil | -> | 124:81 ...!=... |
122+
| deferredCloseWithSyncEarlyReturn | 126:3 defer statement | -> | 128:6 n |
123+
| deferredCloseWithSyncEarlyReturn | 126:9 call to Close | -> | 122:1 exit |
124+
| deferredCloseWithSyncEarlyReturn | 126:9 f | -> | 126:9 selection of Close |
125+
| deferredCloseWithSyncEarlyReturn | 126:9 selection of Close | -> | 126:3 defer statement |
126+
| deferredCloseWithSyncEarlyReturn | 128:6 ...>... | -> | 128:6 ...>... is false |
127+
| deferredCloseWithSyncEarlyReturn | 128:6 ...>... | -> | 128:6 ...>... is true |
128+
| deferredCloseWithSyncEarlyReturn | 128:6 ...>... is false | -> | 133:6 skip |
129+
| deferredCloseWithSyncEarlyReturn | 128:6 ...>... is true | -> | 129:4 return statement |
130+
| deferredCloseWithSyncEarlyReturn | 128:6 n | -> | 128:10 100 |
131+
| deferredCloseWithSyncEarlyReturn | 128:10 100 | -> | 128:6 ...>... |
132+
| deferredCloseWithSyncEarlyReturn | 129:4 return statement | -> | 126:9 call to Close |
133+
| deferredCloseWithSyncEarlyReturn | 133:6 assignment to err | -> | 133:23 err |
134+
| deferredCloseWithSyncEarlyReturn | 133:6 skip | -> | 133:13 f |
135+
| deferredCloseWithSyncEarlyReturn | 133:13 call to Sync | -> | 126:9 call to Close |
136+
| deferredCloseWithSyncEarlyReturn | 133:13 call to Sync | -> | 133:6 assignment to err |
137+
| deferredCloseWithSyncEarlyReturn | 133:13 f | -> | 133:13 selection of Sync |
138+
| deferredCloseWithSyncEarlyReturn | 133:13 selection of Sync | -> | 133:13 call to Sync |
139+
| deferredCloseWithSyncEarlyReturn | 133:23 ...!=... | -> | 126:9 call to Close |
140+
| deferredCloseWithSyncEarlyReturn | 133:23 ...!=... | -> | 133:23 ...!=... is false |
141+
| deferredCloseWithSyncEarlyReturn | 133:23 ...!=... | -> | 133:23 ...!=... is true |
142+
| deferredCloseWithSyncEarlyReturn | 133:23 ...!=... is false | -> | 126:9 call to Close |
143+
| deferredCloseWithSyncEarlyReturn | 133:23 ...!=... is true | -> | 134:4 selection of Fatal |
144+
| deferredCloseWithSyncEarlyReturn | 133:23 err | -> | 133:30 nil |
145+
| deferredCloseWithSyncEarlyReturn | 133:30 nil | -> | 133:23 ...!=... |
146+
| deferredCloseWithSyncEarlyReturn | 134:4 call to Fatal | -> | 126:9 call to Close |
147+
| deferredCloseWithSyncEarlyReturn | 134:4 selection of Fatal | -> | 134:14 err |
148+
| deferredCloseWithSyncEarlyReturn | 134:14 err | -> | 134:4 call to Fatal |
149+
| returnedSync | 149:1 entry | -> | 151:2 skip |
150+
| returnedSync | 149:6 skip | -> | 149:1 function declaration |
151+
| returnedSync | 151:2 ... := ...[0] | -> | 151:2 assignment to f |
152+
| returnedSync | 151:2 ... := ...[1] | -> | 151:5 assignment to err |
153+
| returnedSync | 151:2 assignment to f | -> | 151:2 ... := ...[1] |
154+
| returnedSync | 151:2 skip | -> | 151:5 skip |
155+
| returnedSync | 151:5 assignment to err | -> | 152:5 err |
156+
| returnedSync | 151:5 skip | -> | 151:12 selection of OpenFile |
157+
| returnedSync | 151:12 call to OpenFile | -> | 149:1 exit |
158+
| returnedSync | 151:12 call to OpenFile | -> | 151:2 ... := ...[0] |
159+
| returnedSync | 151:12 selection of OpenFile | -> | 151:24 "foo.txt" |
160+
| returnedSync | 151:24 "foo.txt" | -> | 151:35 ...\|... |
161+
| returnedSync | 151:35 ...\|... | -> | 151:71 0666 |
162+
| returnedSync | 151:71 0666 | -> | 151:12 call to OpenFile |
163+
| returnedSync | 152:5 ...!=... | -> | 149:1 exit |
164+
| returnedSync | 152:5 ...!=... | -> | 152:5 ...!=... is false |
165+
| returnedSync | 152:5 ...!=... | -> | 152:5 ...!=... is true |
166+
| returnedSync | 152:5 ...!=... is false | -> | 157:8 f |
167+
| returnedSync | 152:5 ...!=... is true | -> | 155:10 err |
168+
| returnedSync | 152:5 err | -> | 152:12 nil |
169+
| returnedSync | 152:12 nil | -> | 152:5 ...!=... |
170+
| returnedSync | 155:3 return statement | -> | 149:1 exit |
171+
| returnedSync | 155:10 err | -> | 155:3 return statement |
172+
| returnedSync | 157:2 defer statement | -> | 158:9 f |
173+
| returnedSync | 157:8 call to Close | -> | 149:1 exit |
174+
| returnedSync | 157:8 f | -> | 157:8 selection of Close |
175+
| returnedSync | 157:8 selection of Close | -> | 157:2 defer statement |
176+
| returnedSync | 158:2 return statement | -> | 157:8 call to Close |
177+
| returnedSync | 158:9 call to Sync | -> | 157:8 call to Close |
178+
| returnedSync | 158:9 call to Sync | -> | 158:2 return statement |
179+
| returnedSync | 158:9 f | -> | 158:9 selection of Sync |
180+
| returnedSync | 158:9 selection of Sync | -> | 158:9 call to Sync |

‎go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/DebugDefer2.ql‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import go
2+
3+
from ControlFlow::Node n, ControlFlow::Node succ, FuncDecl fd
4+
where
5+
n.getRoot() = fd and
6+
fd.getName() =
7+
["deferredCloseWithSync", "returnedSync", "copyFile", "deferredCloseWithSyncEarlyReturn"] and
8+
succ = n.getASuccessor()
9+
select fd.getName(),
10+
n.getLocation().getStartLine() + ":" + n.getLocation().getStartColumn() + " " + n.toString(),
11+
"->",
12+
succ.getLocation().getStartLine() + ":" + succ.getLocation().getStartColumn() + " " +
13+
succ.toString()

0 commit comments

Comments
 (0)