Skip to content

Commit 570538b

Browse files
owen-mcowen-mc
authored
Merge pull request #14938 from owen-mc/go/improve-test-unhandled-close-writable-handle
Go: improve test unhandled close writable handle
2 parents 6516539 + e958a75 commit 570538b

File tree

2 files changed

+72
-81
lines changed

2 files changed

+72
-81
lines changed

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

Lines changed: 41 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,56 +1,46 @@
11
edges
2-
| tests.go:9:2:9:74 | ... := ...[0] | tests.go:10:9:10:9 | f |
3-
| tests.go:10:9:10:9 | f | tests.go:43:5:43:38 | ... := ...[0] |
4-
| tests.go:10:9:10:9 | f | tests.go:60:5:60:38 | ... := ...[0] |
5-
| tests.go:10:9:10:9 | f | tests.go:108:5:108:38 | ... := ...[0] |
6-
| tests.go:10:9:10:9 | f | tests.go:124:5:124:38 | ... := ...[0] |
7-
| tests.go:18:2:18:69 | return statement[0] | tests.go:53:5:53:42 | ... := ...[0] |
8-
| tests.go:18:2:18:69 | return statement[0] | tests.go:70:5:70:42 | ... := ...[0] |
9-
| tests.go:21:24:21:24 | definition of f | tests.go:22:8:22:8 | f |
10-
| tests.go:25:32:25:32 | definition of f | tests.go:26:13:26:13 | capture variable f |
11-
| tests.go:26:13:26:13 | capture variable f | tests.go:27:3:27:3 | f |
12-
| tests.go:43:5:43:38 | ... := ...[0] | tests.go:44:21:44:21 | f |
13-
| tests.go:43:5:43:38 | ... := ...[0] | tests.go:45:29:45:29 | f |
14-
| tests.go:44:21:44:21 | f | tests.go:21:24:21:24 | definition of f |
15-
| tests.go:45:29:45:29 | f | tests.go:25:32:25:32 | definition of f |
16-
| tests.go:53:5:53:42 | ... := ...[0] | tests.go:54:21:54:21 | f |
17-
| tests.go:53:5:53:42 | ... := ...[0] | tests.go:55:29:55:29 | f |
18-
| tests.go:54:21:54:21 | f | tests.go:21:24:21:24 | definition of f |
19-
| tests.go:55:29:55:29 | f | tests.go:25:32:25:32 | definition of f |
20-
| tests.go:60:5:60:38 | ... := ...[0] | tests.go:62:3:62:3 | f |
21-
| tests.go:70:5:70:42 | ... := ...[0] | tests.go:72:3:72:3 | f |
22-
| tests.go:108:5:108:38 | ... := ...[0] | tests.go:110:9:110:9 | f |
23-
| tests.go:124:5:124:38 | ... := ...[0] | tests.go:128:3:128:3 | f |
2+
| tests.go:8:24:8:24 | definition of f | tests.go:9:8:9:8 | f |
3+
| tests.go:12:32:12:32 | definition of f | tests.go:13:13:13:13 | capture variable f |
4+
| tests.go:13:13:13:13 | capture variable f | tests.go:14:3:14:3 | f |
5+
| tests.go:31:5:31:78 | ... := ...[0] | tests.go:32:21:32:21 | f |
6+
| tests.go:31:5:31:78 | ... := ...[0] | tests.go:33:29:33:29 | f |
7+
| tests.go:32:21:32:21 | f | tests.go:8:24:8:24 | definition of f |
8+
| tests.go:33:29:33:29 | f | tests.go:12:32:12:32 | definition of f |
9+
| tests.go:45:5:45:76 | ... := ...[0] | tests.go:46:21:46:21 | f |
10+
| tests.go:45:5:45:76 | ... := ...[0] | tests.go:47:29:47:29 | f |
11+
| tests.go:46:21:46:21 | f | tests.go:8:24:8:24 | definition of f |
12+
| tests.go:47:29:47:29 | f | tests.go:12:32:12:32 | definition of f |
13+
| tests.go:54:5:54:78 | ... := ...[0] | tests.go:56:3:56:3 | f |
14+
| tests.go:66:5:66:76 | ... := ...[0] | tests.go:68:3:68:3 | f |
15+
| tests.go:108:5:108:78 | ... := ...[0] | tests.go:110:9:110:9 | f |
16+
| tests.go:125:5:125:78 | ... := ...[0] | tests.go:129:3:129:3 | f |
2417
nodes
25-
| tests.go:9:2:9:74 | ... := ...[0] | semmle.label | ... := ...[0] |
26-
| tests.go:10:9:10:9 | f | semmle.label | f |
27-
| tests.go:18:2:18:69 | return statement[0] | semmle.label | return statement[0] |
28-
| tests.go:21:24:21:24 | definition of f | semmle.label | definition of f |
29-
| tests.go:22:8:22:8 | f | semmle.label | f |
30-
| tests.go:25:32:25:32 | definition of f | semmle.label | definition of f |
31-
| tests.go:26:13:26:13 | capture variable f | semmle.label | capture variable f |
32-
| tests.go:27:3:27:3 | f | semmle.label | f |
33-
| tests.go:43:5:43:38 | ... := ...[0] | semmle.label | ... := ...[0] |
34-
| tests.go:44:21:44:21 | f | semmle.label | f |
35-
| tests.go:45:29:45:29 | f | semmle.label | f |
36-
| tests.go:53:5:53:42 | ... := ...[0] | semmle.label | ... := ...[0] |
37-
| tests.go:54:21:54:21 | f | semmle.label | f |
38-
| tests.go:55:29:55:29 | f | semmle.label | f |
39-
| tests.go:60:5:60:38 | ... := ...[0] | semmle.label | ... := ...[0] |
40-
| tests.go:62:3:62:3 | f | semmle.label | f |
41-
| tests.go:70:5:70:42 | ... := ...[0] | semmle.label | ... := ...[0] |
42-
| tests.go:72:3:72:3 | f | semmle.label | f |
43-
| tests.go:108:5:108:38 | ... := ...[0] | semmle.label | ... := ...[0] |
18+
| tests.go:8:24:8:24 | definition of f | semmle.label | definition of f |
19+
| tests.go:9:8:9:8 | f | semmle.label | f |
20+
| tests.go:12:32:12:32 | definition of f | semmle.label | definition of f |
21+
| tests.go:13:13:13:13 | capture variable f | semmle.label | capture variable f |
22+
| tests.go:14:3:14:3 | f | semmle.label | f |
23+
| tests.go:31:5:31:78 | ... := ...[0] | semmle.label | ... := ...[0] |
24+
| tests.go:32:21:32:21 | f | semmle.label | f |
25+
| tests.go:33:29:33:29 | f | semmle.label | f |
26+
| tests.go:45:5:45:76 | ... := ...[0] | semmle.label | ... := ...[0] |
27+
| tests.go:46:21:46:21 | f | semmle.label | f |
28+
| tests.go:47:29:47:29 | f | semmle.label | f |
29+
| tests.go:54:5:54:78 | ... := ...[0] | semmle.label | ... := ...[0] |
30+
| tests.go:56:3:56:3 | f | semmle.label | f |
31+
| tests.go:66:5:66:76 | ... := ...[0] | semmle.label | ... := ...[0] |
32+
| tests.go:68:3:68:3 | f | semmle.label | f |
33+
| tests.go:108:5:108:78 | ... := ...[0] | semmle.label | ... := ...[0] |
4434
| tests.go:110:9:110:9 | f | semmle.label | f |
45-
| tests.go:124:5:124:38 | ... := ...[0] | semmle.label | ... := ...[0] |
46-
| tests.go:128:3:128:3 | f | semmle.label | f |
35+
| tests.go:125:5:125:78 | ... := ...[0] | semmle.label | ... := ...[0] |
36+
| tests.go:129:3:129:3 | f | semmle.label | f |
4737
subpaths
4838
#select
49-
| tests.go:22:8:22:8 | f | tests.go:9:2:9:74 | ... := ...[0] | tests.go:22:8:22:8 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:9:12:9:74 | call to OpenFile | call to OpenFile |
50-
| tests.go:22:8:22:8 | f | tests.go:18:2:18:69 | return statement[0] | tests.go:22:8:22:8 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:18:9:18:69 | call to OpenFile | call to OpenFile |
51-
| tests.go:27:3:27:3 | f | tests.go:9:2:9:74 | ... := ...[0] | tests.go:27:3:27:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:9:12:9:74 | call to OpenFile | call to OpenFile |
52-
| tests.go:27:3:27:3 | f | tests.go:18:2:18:69 | return statement[0] | tests.go:27:3:27:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:18:9:18:69 | call to OpenFile | call to OpenFile |
53-
| tests.go:62:3:62:3 | f | tests.go:9:2:9:74 | ... := ...[0] | tests.go:62:3:62:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:9:12:9:74 | call to OpenFile | call to OpenFile |
54-
| tests.go:72:3:72:3 | f | tests.go:18:2:18:69 | return statement[0] | tests.go:72:3:72:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:18:9:18:69 | call to OpenFile | call to OpenFile |
55-
| tests.go:110:9:110:9 | f | tests.go:9:2:9:74 | ... := ...[0] | tests.go:110:9:110:9 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:9:12:9:74 | call to OpenFile | call to OpenFile |
56-
| tests.go:128:3:128:3 | f | tests.go:9:2:9:74 | ... := ...[0] | tests.go:128:3:128:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:9:12:9:74 | call to OpenFile | call to OpenFile |
39+
| tests.go:9:8:9:8 | f | tests.go:31:5:31:78 | ... := ...[0] | tests.go:9:8:9:8 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:31:15:31:78 | call to OpenFile | call to OpenFile |
40+
| tests.go:9:8:9:8 | f | tests.go:45:5:45:76 | ... := ...[0] | tests.go:9:8:9:8 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:45:15:45:76 | call to OpenFile | call to OpenFile |
41+
| tests.go:14:3:14:3 | f | tests.go:31:5:31:78 | ... := ...[0] | tests.go:14:3:14:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:31:15:31:78 | call to OpenFile | call to OpenFile |
42+
| tests.go:14:3:14:3 | f | tests.go:45:5:45:76 | ... := ...[0] | tests.go:14:3:14:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:45:15:45:76 | call to OpenFile | call to OpenFile |
43+
| tests.go:56:3:56:3 | f | tests.go:54:5:54:78 | ... := ...[0] | tests.go:56:3:56:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:54:15:54:78 | call to OpenFile | call to OpenFile |
44+
| tests.go:68:3:68:3 | f | tests.go:66:5:66:76 | ... := ...[0] | tests.go:68:3:68:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:66:15:66:76 | call to OpenFile | call to OpenFile |
45+
| tests.go:110:9:110:9 | f | tests.go:108:5:108:78 | ... := ...[0] | tests.go:110:9:110:9 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:108:15:108:78 | call to OpenFile | call to OpenFile |
46+
| tests.go:129:3:129:3 | f | tests.go:125:5:125:78 | ... := ...[0] | tests.go:129:3:129:3 | f | File handle may be writable as a result of data flow from a $@ and closing it may result in data loss upon failure, which is not handled explicitly. | tests.go:125:15:125:78 | call to OpenFile | call to OpenFile |

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

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,6 @@ import (
55
"os"
66
)
77

8-
func openFileWrite(filename string) (*os.File, error) {
9-
f, err := os.OpenFile(filename, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666)
10-
return f, err
11-
}
12-
13-
func openFileRead(filename string) (*os.File, error) {
14-
return os.OpenFile(filename, os.O_RDONLY|os.O_CREATE, 0666)
15-
}
16-
17-
func openFileReadWrite(filename string) (*os.File, error) {
18-
return os.OpenFile(filename, os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0666)
19-
}
20-
218
func closeFileDeferred(f *os.File) {
229
defer f.Close() // NOT OK, if `f` is writable
2310
}
@@ -40,41 +27,51 @@ func closeFileDeferredIndirectReturn(f *os.File) {
4027
}
4128

4229
func deferredCalls() {
43-
if f, err := openFileWrite("foo.txt"); err != nil {
44-
closeFileDeferred(f) // NOT OK
45-
closeFileDeferredIndirect(f) // NOT OK
30+
// open file for writing
31+
if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
32+
closeFileDeferred(f) // NOT OK
33+
closeFileDeferredIndirect(f) // NOT OK
34+
closeFileDeferredIndirectReturn(f) // OK - the error is not discarded at the call to Close (though it is discarded later)
4635
}
4736

48-
if f, err := openFileRead("foo.txt"); err != nil {
49-
closeFileDeferred(f) // OK
50-
closeFileDeferredIndirect(f) // OK
37+
// open file for reading
38+
if f, err := os.OpenFile("foo.txt", os.O_RDONLY|os.O_CREATE, 0666); err != nil {
39+
closeFileDeferred(f) // OK
40+
closeFileDeferredIndirect(f) // OK
41+
closeFileDeferredIndirectReturn(f) // OK
5142
}
5243

53-
if f, err := openFileReadWrite("foo.txt"); err != nil {
54-
closeFileDeferred(f) // NOT OK
55-
closeFileDeferredIndirect(f) // NOT OK
44+
// open file for reading and writing
45+
if f, err := os.OpenFile("foo.txt", os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
46+
closeFileDeferred(f) // NOT OK
47+
closeFileDeferredIndirect(f) // NOT OK
48+
closeFileDeferredIndirectReturn(f) // OK - the error is not discarded at the call to Close (though it is discarded later)
5649
}
5750
}
5851

5952
func notDeferred() {
60-
if f, err := openFileWrite("foo.txt"); err != nil {
53+
// open file for writing
54+
if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
6155
// the handle is write-only and we don't check if `Close` succeeds
6256
f.Close() // NOT OK
6357
}
6458

65-
if f, err := openFileRead("foo.txt"); err != nil {
59+
// open file for reading
60+
if f, err := os.OpenFile("foo.txt", os.O_RDONLY|os.O_CREATE, 0666); err != nil {
6661
// the handle is read-only, so this is ok
6762
f.Close() // OK
6863
}
6964

70-
if f, err := openFileReadWrite("foo.txt"); err != nil {
65+
// open file for reading and writing
66+
if f, err := os.OpenFile("foo.txt", os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
7167
// the handle is read-write and we don't check if `Close` succeeds
7268
f.Close() // NOT OK
7369
}
7470
}
7571

7672
func foo() error {
77-
if f, err := openFileWrite("foo.txt"); err != nil {
73+
// open file for writing
74+
if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
7875
// the result of the call to `Close` is returned to the caller
7976
return f.Close() // OK
8077
}
@@ -83,7 +80,8 @@ func foo() error {
8380
}
8481

8582
func isSyncedFirst() {
86-
if f, err := openFileWrite("foo.txt"); err != nil {
83+
// open file for writing
84+
if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
8785
// we have a call to `Sync` and check whether it was successful before proceeding
8886
if err := f.Sync(); err != nil {
8987
f.Close() // OK
@@ -93,7 +91,8 @@ func isSyncedFirst() {
9391
}
9492

9593
func deferredCloseWithSync() {
96-
if f, err := openFileWrite("foo.txt"); err != nil {
94+
// open file for writing
95+
if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
9796
// a call to `Close` is deferred, but we have a call to `Sync` later which
9897
// precedes the call to `Close` during execution
9998
defer f.Close() // OK
@@ -105,7 +104,8 @@ func deferredCloseWithSync() {
105104
}
106105

107106
func deferredCloseWithSyncEarlyReturn(n int) {
108-
if f, err := openFileWrite("foo.txt"); err != nil {
107+
// open file for writing
108+
if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
109109
// a call to `Close` is deferred
110110
defer f.Close() // NOT OK
111111

@@ -121,7 +121,8 @@ func deferredCloseWithSyncEarlyReturn(n int) {
121121
}
122122

123123
func unhandledSync() {
124-
if f, err := openFileWrite("foo.txt"); err != nil {
124+
// open file for writing
125+
if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
125126
// we have a call to `Sync` which precedes the call to `Close`, but there is no check
126127
// to see if `Sync` may have failed
127128
f.Sync()

0 commit comments

Comments
 (0)