Merge pull request #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

go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.go

@@ -0,0 +1,21 @@
+package main
+
+import (
+	"os"
+)
+
+func example() error {
+	f, err := os.OpenFile("file.txt", os.O_WRONLY|os.O_CREATE, 0666)
+
+	if err != nil {
+		return err
+	}
+
+	defer f.Close()
+
+	if _, err := f.WriteString("Hello"); err != nil {
+		return err
+	}
+
+	return nil
+}

go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.qhelp

@@ -0,0 +1,55 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Data written to a file handle may not immediately be flushed to the underlying storage medium by
+the operating system. Often, the data may be cached in memory until the handle is closed, or
+until a later point after that. Only calling <code>os.File.Sync</code> gives a reasonable guarantee
+that the data is flushed. Therefore, write errors may not occur until <code>os.File.Close</code>
+or <code>os.File.Sync</code> are called. If either is called and any errors returned by them are
+discarded, then the program may be unaware that data loss occurred.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Always check whether <code>os.File.Close</code> returned an error and handle it appropriately.
+</p>
+</recommendation>
+
+<example>
+
+<p>
+In this first example, two calls to <code>os.File.Close</code> are made with the intention of
+closing the file after all work on it has been done or if writing to it fails. However, while
+errors that may arise during the call to <code>os.File.WriteString</code> are handled, any
+errors arising from the calls to <code>os.File.Close</code> are discarded silently:
+</p>
+
+<sample src="UnhandledCloseWritableHandleNotDeferred.go" />
+
+<p>
+In the second example, a call to <code>os.File.Close</code> is deferred in order to accomplish
+the same behaviour as in the first example. However, while errors that may arise during
+the call to <code>os.File.WriteString</code> are handled, any errors arising from
+<code>os.File.Close</code> are again discarded silently:
+</p>
+
+<sample src="UnhandledCloseWritableHandle.go" />
+
+<p>
+To correct this issue, handle errors arising from calls to <code>os.File.Close</code> explicitly:
+</p>
+
+<sample src="UnhandledCloseWritableHandleGood.go" />
+
+</example>
+
+<references>
+    <li>The Go Programming Language Specification: <a href="https://go.dev/ref/spec#Defer_statements">Defer statements</a>.</li>
+    <li>The Go Programming Language Specification: <a href="https://go.dev/ref/spec#Errors">Errors</a>.</li>
+</references>
+</qhelp>

go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql

@@ -0,0 +1,151 @@
+/**
+ * @name Writable file handle closed without error handling
+ * @description Errors which occur when closing a writable file handle may result in data loss
+ *              if the data could not be successfully flushed. Such errors should be handled
+ *              explicitly.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id go/unhandled-writable-file-close
+ * @tags maintainability
+ *  correctness
+ *  call
+ *  defer
+ */
+
+import go
+import DataFlow::PathGraph
+
+/**
+ * Holds if a `flag` for use with `os.OpenFile` implies that the resulting
+ * file handle will be writable.
+ */
+predicate isWritable(Entity flag) {
+  flag.hasQualifiedName("os", "O_WRONLY") or
+  flag.hasQualifiedName("os", "O_RDWR")
+}
+
+/**
+ * Gets constant names from `expr`.
+ */
+QualifiedName getConstants(ValueExpr expr) {
+  result = expr or
+  result = getConstants(expr.getAChild())
+}
+
+/**
+ * The `os.OpenFile` function.
+ */
+class OpenFileFun extends Function {
+  OpenFileFun() { this.hasQualifiedName("os", "OpenFile") }
+}
+
+/**
+ * The `os.File.Close` function.
+ */
+class CloseFileFun extends Method {
+  CloseFileFun() { this.hasQualifiedName("os", "File", "Close") }
+}
+
+/**
+ * The `os.File.Sync` function.
+ */
+class SyncFileFun extends Method {
+  SyncFileFun() { this.hasQualifiedName("os", "File", "Sync") }
+}
+
+/**
+ * Holds if a `call` to a function is "unhandled". That is, it is either
+ * deferred or its result is not assigned to anything.
+ *
+ * TODO: maybe we should check that something is actually done with the result
+ */
+predicate unhandledCall(DataFlow::CallNode call) {
+  exists(DeferStmt defer | defer.getCall() = call.asExpr()) or
+  exists(ExprStmt stmt | stmt.getExpr() = call.asExpr())
+}
+
+/**
+ * Holds if `source` is a writable file handle returned by a `call` to the
+ * `os.OpenFile` function.
+ */
+predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
+  exists(OpenFileFun f, DataFlow::Node flags, QualifiedName flag |
+    // check that the source is a result of the call
+    source = call.getAResult() and
+    // find a call to the os.OpenFile function
+    f.getACall() = call and
+    // get the flags expression used for opening the file
+    call.getArgument(1) = flags and
+    // extract individual flags from the argument
+    // flag = flag.getAChild*() and
+    flag = getConstants(flags.asExpr()) and
+    // check for one which signals that the handle will be writable
+    // note that we are underestimating here, since the flags may be
+    // specified elsewhere
+    isWritable(flag.getTarget())
+  )
+}
+
+/**
+ * Holds if `os.File.Close` is called on `sink`.
+ */
+predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
+  // find calls to the os.File.Close function
+  closeCall = any(CloseFileFun f).getACall() and
+  // that are unhandled
+  unhandledCall(closeCall) and
+  // where the function is called on the sink
+  closeCall.getReceiver() = sink and
+  // and check that it is not dominated by a call to `os.File.Sync`.
+  not exists(IR::Instruction syncInstr, DataFlow::Node syncReceiver, DataFlow::CallNode syncCall |
+    // match the instruction corresponding to an `os.File.Sync` call with the predecessor
+    syncCall.asInstruction() = syncInstr and
+    // check that the call to `os.File.Sync` is handled
+    isHandledSync(syncReceiver, syncCall) and
+    // find a predecessor to `closeCall` in the control flow graph which dominates the call to
+    // `os.File.Close`
+    syncInstr.dominatesNode(closeCall.asInstruction()) and
+    // check that `os.File.Sync` is called on the same object as `os.File.Close`
+    exists(DataFlow::SsaNode ssa | ssa.getAUse() = sink and ssa.getAUse() = syncReceiver)
+  )
+}
+
+/**
+ * Holds if `os.File.Sync` is called on `sink` and the result of the call is neither
+ * deferred nor discarded.
+ */
+predicate isHandledSync(DataFlow::Node sink, DataFlow::CallNode syncCall) {
+  // find a call of the `os.File.Sync` function
+  syncCall = any(SyncFileFun f).getACall() and
+  // match the sink with the object on which the method is called
+  syncCall.getReceiver() = sink and
+  // check that the result is neither deferred nor discarded
+  not unhandledCall(syncCall)
+}
+
+/**
+ * A data flow configuration which traces writable file handles resulting from calls to
+ * `os.OpenFile` to `os.File.Close` calls on them.
+ */
+class UnhandledFileCloseDataFlowConfiguration extends DataFlow::Configuration {
+  UnhandledFileCloseDataFlowConfiguration() { this = "UnhandledCloseWritableHandle" }
+
+  override predicate isSource(DataFlow::Node source) { isWritableFileHandle(source, _) }
+
+  override predicate isSink(DataFlow::Node sink) { isCloseSink(sink, _) }
+}
+
+from
+  UnhandledFileCloseDataFlowConfiguration cfg, DataFlow::PathNode source,
+  DataFlow::CallNode openCall, DataFlow::PathNode sink, DataFlow::CallNode closeCall
+where
+  // find data flow from an `os.OpenFile` call to an `os.File.Close` call
+  // where the handle is writable
+  cfg.hasFlowPath(source, sink) and
+  isWritableFileHandle(source.getNode(), openCall) and
+  // get the `CallNode` corresponding to the sink
+  isCloseSink(sink.getNode(), closeCall)
+select sink, source, sink,
+  "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.",
+  openCall, openCall.toString()

go/ql/src/InconsistentCode/UnhandledCloseWritableHandleGood.go

@@ -0,0 +1,27 @@
+package main
+
+import (
+	"os"
+)
+
+func example() (exampleError error) {
+	f, err := os.OpenFile("file.txt", os.O_WRONLY|os.O_CREATE, 0666)
+
+	if err != nil {
+		return err
+	}
+
+	defer func() {
+		// try to close the file; if an error occurs, set the error returned by `example`
+		// to that error, but only if `WriteString` didn't already set it to something first
+		if err := f.Close(); exampleError == nil && err != nil {
+			exampleError = err
+		}
+	}()
+
+	if _, err := f.WriteString("Hello"); err != nil {
+		exampleError = err
+	}
+
+	return
+}

go/ql/src/InconsistentCode/UnhandledCloseWritableHandleNotDeferred.go

@@ -0,0 +1,22 @@
+package main
+
+import (
+	"os"
+)
+
+func example() error {
+	f, err := os.OpenFile("file.txt", os.O_WRONLY|os.O_CREATE, 0666)
+
+	if err != nil {
+		return err
+	}
+
+	if _, err := f.WriteString("Hello"); err != nil {
+		f.Close()
+		return err
+	}
+
+	f.Close()
+
+	return nil
+}

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

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* 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

@@ -0,0 +1,56 @@
+edges
+| tests.go:9:2:9:74 | ... := ...[0] | tests.go:10:9:10:9 | f |
+| tests.go:10:9:10:9 | f | tests.go:43:5:43:38 | ... := ...[0] |
+| tests.go:10:9:10:9 | f | tests.go:60:5:60:38 | ... := ...[0] |
+| tests.go:10:9:10:9 | f | tests.go:108:5:108:38 | ... := ...[0] |
+| tests.go:10:9:10:9 | f | tests.go:124:5:124:38 | ... := ...[0] |
+| tests.go:18:2:18:69 | return statement[0] | tests.go:53:5:53:42 | ... := ...[0] |
+| tests.go:18:2:18:69 | return statement[0] | tests.go:70:5:70:42 | ... := ...[0] |
+| tests.go:21:24:21:24 | definition of f | tests.go:22:8:22:8 | f |
+| tests.go:25:32:25:32 | definition of f | tests.go:26:13:26:13 | capture variable f |
+| tests.go:26:13:26:13 | capture variable f | tests.go:27:3:27:3 | f |
+| tests.go:43:5:43:38 | ... := ...[0] | tests.go:44:21:44:21 | f |
+| tests.go:43:5:43:38 | ... := ...[0] | tests.go:45:29:45:29 | f |
+| tests.go:44:21:44:21 | f | tests.go:21:24:21:24 | definition of f |
+| tests.go:45:29:45:29 | f | tests.go:25:32:25:32 | definition of f |
+| tests.go:53:5:53:42 | ... := ...[0] | tests.go:54:21:54:21 | f |
+| tests.go:53:5:53:42 | ... := ...[0] | tests.go:55:29:55:29 | f |
+| tests.go:54:21:54:21 | f | tests.go:21:24:21:24 | definition of f |
+| tests.go:55:29:55:29 | f | tests.go:25:32:25:32 | definition of f |
+| tests.go:60:5:60:38 | ... := ...[0] | tests.go:62:3:62:3 | f |
+| tests.go:70:5:70:42 | ... := ...[0] | tests.go:72:3:72:3 | f |
+| tests.go:108:5:108:38 | ... := ...[0] | tests.go:110:9:110:9 | f |
+| tests.go:124:5:124:38 | ... := ...[0] | tests.go:128:3:128:3 | f |
+nodes
+| tests.go:9:2:9:74 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:10:9:10:9 | f | semmle.label | f |
+| tests.go:18:2:18:69 | return statement[0] | semmle.label | return statement[0] |
+| tests.go:21:24:21:24 | definition of f | semmle.label | definition of f |
+| tests.go:22:8:22:8 | f | semmle.label | f |
+| tests.go:25:32:25:32 | definition of f | semmle.label | definition of f |
+| tests.go:26:13:26:13 | capture variable f | semmle.label | capture variable f |
+| tests.go:27:3:27:3 | f | semmle.label | f |
+| tests.go:43:5:43:38 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:44:21:44:21 | f | semmle.label | f |
+| tests.go:45:29:45:29 | f | semmle.label | f |
+| tests.go:53:5:53:42 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:54:21:54:21 | f | semmle.label | f |
+| tests.go:55:29:55:29 | f | semmle.label | f |
+| tests.go:60:5:60:38 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:62:3:62:3 | f | semmle.label | f |
+| tests.go:70:5:70:42 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:72:3:72:3 | f | semmle.label | f |
+| tests.go:108:5:108:38 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:110:9:110:9 | f | semmle.label | f |
+| tests.go:124:5:124:38 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:128:3:128:3 | f | semmle.label | f |
+subpaths
+#select
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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

@@ -0,0 +1 @@
+InconsistentCode/UnhandledCloseWritableHandle.ql

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

@@ -0,0 +1,130 @@
+package test
+
+import (
+	"log"
+	"os"
+)
+
+func openFileWrite(filename string) (*os.File, error) {
+	f, err := os.OpenFile(filename, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666)
+	return f, err
+}
+
+func openFileRead(filename string) (*os.File, error) {
+	return os.OpenFile(filename, os.O_RDONLY|os.O_CREATE, 0666)
+}
+
+func openFileReadWrite(filename string) (*os.File, error) {
+	return os.OpenFile(filename, os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0666)
+}
+
+func closeFileDeferred(f *os.File) {
+	defer f.Close() // NOT OK, if `f` is writable
+}
+
+func closeFileDeferredIndirect(f *os.File) {
+	var cont = func() {
+		f.Close() // NOT OK, if `f` is writable
+	}
+
+	defer cont()
+}
+
+func closeFileDeferredIndirectReturn(f *os.File) {
+	var cont = func() error {
+		return f.Close() // OK, because this function returns the error
+	}
+
+	// different (more general) problem: deferred error
+	defer cont()
+}
+
+func deferredCalls() {
+	if f, err := openFileWrite("foo.txt"); err != nil {
+		closeFileDeferred(f)         // NOT OK
+		closeFileDeferredIndirect(f) // NOT OK
+	}
+
+	if f, err := openFileRead("foo.txt"); err != nil {
+		closeFileDeferred(f)         // OK
+		closeFileDeferredIndirect(f) // OK
+	}
+
+	if f, err := openFileReadWrite("foo.txt"); err != nil {
+		closeFileDeferred(f)         // NOT OK
+		closeFileDeferredIndirect(f) // NOT OK
+	}
+}
+
+func notDeferred() {
+	if f, err := openFileWrite("foo.txt"); err != nil {
+		// the handle is write-only and we don't check if `Close` succeeds
+		f.Close() // NOT OK
+	}
+
+	if f, err := openFileRead("foo.txt"); err != nil {
+		// the handle is read-only, so this is ok
+		f.Close() // OK
+	}
+
+	if f, err := openFileReadWrite("foo.txt"); err != nil {
+		// the handle is read-write and we don't check if `Close` succeeds
+		f.Close() // NOT OK
+	}
+}
+
+func foo() error {
+	if f, err := openFileWrite("foo.txt"); err != nil {
+		// the result of the call to `Close` is returned to the caller
+		return f.Close() // OK
+	}
+
+	return nil
+}
+
+func isSyncedFirst() {
+	if f, err := openFileWrite("foo.txt"); err != nil {
+		// we have a call to `Sync` and check whether it was successful before proceeding
+		if err := f.Sync(); err != nil {
+			f.Close() // OK
+		}
+		f.Close() // OK
+	}
+}
+
+func deferredCloseWithSync() {
+	if f, err := openFileWrite("foo.txt"); err != nil {
+		// a call to `Close` is deferred, but we have a call to `Sync` later which
+		// precedes the call to `Close` during execution
+		defer f.Close() // OK
+
+		if err := f.Sync(); err != nil {
+			log.Fatal(err)
+		}
+	}
+}
+
+func deferredCloseWithSyncEarlyReturn(n int) {
+	if f, err := openFileWrite("foo.txt"); err != nil {
+		// a call to `Close` is deferred
+		defer f.Close() // NOT OK
+
+		if n > 100 {
+			return
+		}
+
+		// we have a call to `Sync` here, but it might not get executed if n <= 100
+		if err := f.Sync(); err != nil {
+			log.Fatal(err)
+		}
+	}
+}
+
+func unhandledSync() {
+	if f, err := openFileWrite("foo.txt"); err != nil {
+		// we have a call to `Sync` which precedes the call to `Close`, but there is no check
+		// to see if `Sync` may have failed
+		f.Sync()
+		f.Close() // NOT OK
+	}
+}