Detect lack of error handling for os.File.Close

Author: mbg

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,45 @@
+<!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 the following example, a call to <code>os.File.Close</code> is deferred with the intention of
+closing the file after all work on it has been done. 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 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,160 @@
+/**
+ * @name Writable file handled 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 problem
+ * @problem.severity warning
+ * @precision medium
+ * @id go/unhandled-writable-file-close
+ * @tags maintainability
+ *  correctness
+ *  call
+ *  defer
+ */
+
+import go
+
+/**
+ * Determines whether 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")
+}
+
+/**
+ * Recursively extracts constant names from an expression.
+ */
+QualifiedName getConstants(ValueExpr expr) {
+  result = expr or
+  result = getConstants(expr.getAChild())
+}
+
+/**
+ * Matches the `os.OpenFile` function.
+ */
+class OpenFileFun extends Function {
+  OpenFileFun() { this.hasQualifiedName("os", "OpenFile") }
+}
+
+/**
+ * Matches the `os.File.Close` function.
+ */
+class CloseFileFun extends Function {
+  CloseFileFun() { this.hasQualifiedName("os.File", "Close") }
+}
+
+/**
+ * Matches the `os.File.Sync` function.
+ */
+class SyncFileFun extends Function {
+  SyncFileFun() { this.hasQualifiedName("os.File", "Sync") }
+}
+
+/**
+ * Determines whether 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())
+}
+
+/**
+ * Determines whether `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())
+  )
+}
+
+/**
+ * Determines whether `os.File.Close` is called on `sink`.
+ */
+predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode call) {
+  exists(CloseFileFun f |
+    // find calls to the os.File.Close function
+    f.getACall() = call and
+    // that are deferred
+    unhandledCall(call) and
+    // where the function is called on the sink
+    call.getReceiver() = sink
+  )
+}
+
+/**
+ * Determines whether `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, _) }
+}
+
+/**
+ * Determines whether a `DataFlow::CallNode` is preceded by a call to `os.File.Sync`.
+ */
+predicate precededBySync(DataFlow::Node close, DataFlow::CallNode closeCall) {
+  // using the control flow graph, try to find a call to a handled call to `os.File.Sync`
+  // which precedes `closeCall`.
+  exists(IR::Instruction instr, DataFlow::Node syncReceiver, DataFlow::CallNode syncCall |
+    // find a predecessor to `closeCall` in the control flow graph
+    instr = closeCall.asInstruction().getAPredecessor*() and
+    // match the instruction corresponding to an `os.File.Sync` call with the predecessor
+    syncCall.asInstruction() = instr and
+    // check that the call to `os.File.Sync` is handled
+    isHandledSync(syncReceiver, syncCall) and
+    // check that `os.File.Sync` is called on the same object as `os.File.Close`
+    exists(DataFlow::SsaNode ssa | ssa.getAUse() = close and ssa.getAUse() = syncReceiver)
+  )
+}
+
+from
+  UnhandledFileCloseDataFlowConfiguration cfg, DataFlow::Node source, DataFlow::CallNode openCall,
+  DataFlow::Node close, DataFlow::CallNode closeCall
+where
+  // find data flow from an `os.OpenFile` call to an `os.File.Close` call
+  // where the handle is writable
+  cfg.hasFlow(source, close) and
+  isWritableFileHandle(source, openCall) and
+  // get the `CallNode` corresponding to the sink
+  isCloseSink(close, closeCall) and
+  // check that the call to `os.File.Close` is not preceded by a checked call to
+  // `os.File.Sync`
+  not precededBySync(close, closeCall)
+select close,
+  "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/change-notes/2022-11-30-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,7 @@
+| 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 | 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 | 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 | 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:53:3:53: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:63:3:63: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:119:3:119: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,121 @@
+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 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 - false negative
+
+		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
+	}
+}