Skip to content

Commit 3abf321

Browse files
mbgmbg
authored
Merge pull request github#11496 from github/mbg/add/writable-file-closed-error-query
Go: Add query to detect lack of error handling for `os.File.Close` on writable handles
2 parents 7f09684 + f7a2a86 commit 3abf321

9 files changed

+467
-0
lines changed

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
package main
2+
3+
import (
4+
"os"
5+
)
6+
7+
func example() error {
8+
f, err := os.OpenFile("file.txt", os.O_WRONLY|os.O_CREATE, 0666)
9+
10+
if err != nil {
11+
return err
12+
}
13+
14+
defer f.Close()
15+
16+
if _, err := f.WriteString("Hello"); err != nil {
17+
return err
18+
}
19+
20+
return nil
21+
}

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Data written to a file handle may not immediately be flushed to the underlying storage medium by
9+
the operating system. Often, the data may be cached in memory until the handle is closed, or
10+
until a later point after that. Only calling <code>os.File.Sync</code> gives a reasonable guarantee
11+
that the data is flushed. Therefore, write errors may not occur until <code>os.File.Close</code>
12+
or <code>os.File.Sync</code> are called. If either is called and any errors returned by them are
13+
discarded, then the program may be unaware that data loss occurred.
14+
</p>
15+
</overview>
16+
17+
<recommendation>
18+
<p>
19+
Always check whether <code>os.File.Close</code> returned an error and handle it appropriately.
20+
</p>
21+
</recommendation>
22+
23+
<example>
24+
25+
<p>
26+
In this first example, two calls to <code>os.File.Close</code> are made with the intention of
27+
closing the file after all work on it has been done or if writing to it fails. However, while
28+
errors that may arise during the call to <code>os.File.WriteString</code> are handled, any
29+
errors arising from the calls to <code>os.File.Close</code> are discarded silently:
30+
</p>
31+
32+
<sample src="UnhandledCloseWritableHandleNotDeferred.go" />
33+
34+
<p>
35+
In the second example, a call to <code>os.File.Close</code> is deferred in order to accomplish
36+
the same behaviour as in the first example. However, while errors that may arise during
37+
the call to <code>os.File.WriteString</code> are handled, any errors arising from
38+
<code>os.File.Close</code> are again discarded silently:
39+
</p>
40+
41+
<sample src="UnhandledCloseWritableHandle.go" />
42+
43+
<p>
44+
To correct this issue, handle errors arising from calls to <code>os.File.Close</code> explicitly:
45+
</p>
46+
47+
<sample src="UnhandledCloseWritableHandleGood.go" />
48+
49+
</example>
50+
51+
<references>
52+
<li>The Go Programming Language Specification: <a href="https://go.dev/ref/spec#Defer_statements">Defer statements</a>.</li>
53+
<li>The Go Programming Language Specification: <a href="https://go.dev/ref/spec#Errors">Errors</a>.</li>
54+
</references>
55+
</qhelp>

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

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
/**
2+
* @name Writable file handle closed without error handling
3+
* @description Errors which occur when closing a writable file handle may result in data loss
4+
* if the data could not be successfully flushed. Such errors should be handled
5+
* explicitly.
6+
* @kind path-problem
7+
* @problem.severity warning
8+
* @precision high
9+
* @id go/unhandled-writable-file-close
10+
* @tags maintainability
11+
* correctness
12+
* call
13+
* defer
14+
*/
15+
16+
import go
17+
import DataFlow::PathGraph
18+
19+
/**
20+
* Holds if a `flag` for use with `os.OpenFile` implies that the resulting
21+
* file handle will be writable.
22+
*/
23+
predicate isWritable(Entity flag) {
24+
flag.hasQualifiedName("os", "O_WRONLY") or
25+
flag.hasQualifiedName("os", "O_RDWR")
26+
}
27+
28+
/**
29+
* Gets constant names from `expr`.
30+
*/
31+
QualifiedName getConstants(ValueExpr expr) {
32+
result = expr or
33+
result = getConstants(expr.getAChild())
34+
}
35+
36+
/**
37+
* The `os.OpenFile` function.
38+
*/
39+
class OpenFileFun extends Function {
40+
OpenFileFun() { this.hasQualifiedName("os", "OpenFile") }
41+
}
42+
43+
/**
44+
* The `os.File.Close` function.
45+
*/
46+
class CloseFileFun extends Method {
47+
CloseFileFun() { this.hasQualifiedName("os", "File", "Close") }
48+
}
49+
50+
/**
51+
* The `os.File.Sync` function.
52+
*/
53+
class SyncFileFun extends Method {
54+
SyncFileFun() { this.hasQualifiedName("os", "File", "Sync") }
55+
}
56+
57+
/**
58+
* Holds if a `call` to a function is "unhandled". That is, it is either
59+
* deferred or its result is not assigned to anything.
60+
*
61+
* TODO: maybe we should check that something is actually done with the result
62+
*/
63+
predicate unhandledCall(DataFlow::CallNode call) {
64+
exists(DeferStmt defer | defer.getCall() = call.asExpr()) or
65+
exists(ExprStmt stmt | stmt.getExpr() = call.asExpr())
66+
}
67+
68+
/**
69+
* Holds if `source` is a writable file handle returned by a `call` to the
70+
* `os.OpenFile` function.
71+
*/
72+
predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
73+
exists(OpenFileFun f, DataFlow::Node flags, QualifiedName flag |
74+
// check that the source is a result of the call
75+
source = call.getAResult() and
76+
// find a call to the os.OpenFile function
77+
f.getACall() = call and
78+
// get the flags expression used for opening the file
79+
call.getArgument(1) = flags and
80+
// extract individual flags from the argument
81+
// flag = flag.getAChild*() and
82+
flag = getConstants(flags.asExpr()) and
83+
// check for one which signals that the handle will be writable
84+
// note that we are underestimating here, since the flags may be
85+
// specified elsewhere
86+
isWritable(flag.getTarget())
87+
)
88+
}
89+
90+
/**
91+
* Holds if `os.File.Close` is called on `sink`.
92+
*/
93+
predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
94+
// find calls to the os.File.Close function
95+
closeCall = any(CloseFileFun f).getACall() and
96+
// that are unhandled
97+
unhandledCall(closeCall) and
98+
// where the function is called on the sink
99+
closeCall.getReceiver() = sink and
100+
// and check that it is not dominated by a call to `os.File.Sync`.
101+
not exists(IR::Instruction syncInstr, DataFlow::Node syncReceiver, DataFlow::CallNode syncCall |
102+
// match the instruction corresponding to an `os.File.Sync` call with the predecessor
103+
syncCall.asInstruction() = syncInstr and
104+
// check that the call to `os.File.Sync` is handled
105+
isHandledSync(syncReceiver, syncCall) and
106+
// find a predecessor to `closeCall` in the control flow graph which dominates the call to
107+
// `os.File.Close`
108+
syncInstr.dominatesNode(closeCall.asInstruction()) and
109+
// check that `os.File.Sync` is called on the same object as `os.File.Close`
110+
exists(DataFlow::SsaNode ssa | ssa.getAUse() = sink and ssa.getAUse() = syncReceiver)
111+
)
112+
}
113+
114+
/**
115+
* Holds if `os.File.Sync` is called on `sink` and the result of the call is neither
116+
* deferred nor discarded.
117+
*/
118+
predicate isHandledSync(DataFlow::Node sink, DataFlow::CallNode syncCall) {
119+
// find a call of the `os.File.Sync` function
120+
syncCall = any(SyncFileFun f).getACall() and
121+
// match the sink with the object on which the method is called
122+
syncCall.getReceiver() = sink and
123+
// check that the result is neither deferred nor discarded
124+
not unhandledCall(syncCall)
125+
}
126+
127+
/**
128+
* A data flow configuration which traces writable file handles resulting from calls to
129+
* `os.OpenFile` to `os.File.Close` calls on them.
130+
*/
131+
class UnhandledFileCloseDataFlowConfiguration extends DataFlow::Configuration {
132+
UnhandledFileCloseDataFlowConfiguration() { this = "UnhandledCloseWritableHandle" }
133+
134+
override predicate isSource(DataFlow::Node source) { isWritableFileHandle(source, _) }
135+
136+
override predicate isSink(DataFlow::Node sink) { isCloseSink(sink, _) }
137+
}
138+
139+
from
140+
UnhandledFileCloseDataFlowConfiguration cfg, DataFlow::PathNode source,
141+
DataFlow::CallNode openCall, DataFlow::PathNode sink, DataFlow::CallNode closeCall
142+
where
143+
// find data flow from an `os.OpenFile` call to an `os.File.Close` call
144+
// where the handle is writable
145+
cfg.hasFlowPath(source, sink) and
146+
isWritableFileHandle(source.getNode(), openCall) and
147+
// get the `CallNode` corresponding to the sink
148+
isCloseSink(sink.getNode(), closeCall)
149+
select sink, source, sink,
150+
"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.",
151+
openCall, openCall.toString()

‎go/ql/src/InconsistentCode/UnhandledCloseWritableHandleGood.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package main
2+
3+
import (
4+
"os"
5+
)
6+
7+
func example() (exampleError error) {
8+
f, err := os.OpenFile("file.txt", os.O_WRONLY|os.O_CREATE, 0666)
9+
10+
if err != nil {
11+
return err
12+
}
13+
14+
defer func() {
15+
// try to close the file; if an error occurs, set the error returned by `example`
16+
// to that error, but only if `WriteString` didn't already set it to something first
17+
if err := f.Close(); exampleError == nil && err != nil {
18+
exampleError = err
19+
}
20+
}()
21+
22+
if _, err := f.WriteString("Hello"); err != nil {
23+
exampleError = err
24+
}
25+
26+
return
27+
}

‎go/ql/src/InconsistentCode/UnhandledCloseWritableHandleNotDeferred.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
package main
2+
3+
import (
4+
"os"
5+
)
6+
7+
func example() error {
8+
f, err := os.OpenFile("file.txt", os.O_WRONLY|os.O_CREATE, 0666)
9+
10+
if err != nil {
11+
return err
12+
}
13+
14+
if _, err := f.WriteString("Hello"); err != nil {
15+
f.Close()
16+
return err
17+
}
18+
19+
f.Close()
20+
21+
return nil
22+
}

‎go/ql/src/change-notes/2023-02-06-unhandled-close-writable-handle.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `go/unhandled-writable-file-close`, to detect instances where writable file handles are closed without appropriate checks for errors.

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
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 |
24+
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] |
44+
| 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 |
47+
subpaths
48+
#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 |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
InconsistentCode/UnhandledCloseWritableHandle.ql

0 commit comments

Comments
 (0)