github · owen-mc · Jun 4, 2026 · Jun 4, 2026 · Jun 4, 2026 · Jun 4, 2026
@@ -55,7 +55,7 @@ class SyncFileFun extends Method {
 
 /**
  * Holds if a `call` to a function is "unhandled". That is, it is either
- * deferred or its result is not assigned to anything.
+ * deferred or used as an expression statement, so that its result is discarded.
  *
  * TODO: maybe we should check that something is actually done with the result
  */
@@ -77,7 +77,6 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
     // 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
@@ -87,27 +86,18 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
 }
 
 /**
- * Holds if `os.File.Close` is called on `sink`.
+ * Holds if `postDominator` post-dominates `node` in the control-flow graph. That is,
+ * every path from `node` to the exit of the enclosing function passes through
+ * `postDominator`.
  */
-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`.
-  // TODO: fix this logic when `closeCall` is in a defer statement.
-  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)
+pragma[inline]
+predicate postDominatesNode(ControlFlow::Node postDominator, ControlFlow::Node node) {
+  exists(ReachableBasicBlock pdbb, ReachableBasicBlock nbb, int i, int j |
+    postDominator = pdbb.getNode(i) and node = nbb.getNode(j)
+  |
+    pdbb.strictlyPostDominates(nbb)
+    or
+    pdbb = nbb and i >= j
   )
 }
 
@@ -127,7 +117,39 @@ predicate isHandledSync(DataFlow::Node sink, DataFlow::CallNode syncCall) {
 module UnhandledFileCloseConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { isWritableFileHandle(source, _) }
 
-  predicate isSink(DataFlow::Node sink) { isCloseSink(sink, _) }
+  predicate isSink(DataFlow::Node sink) {
+    exists(DataFlow::CallNode closeCall |
+      // `closeCall` is an unhandled call to `os.File.Close` on `sink`
+      closeCall = any(CloseFileFun f).getACall() and
+      unhandledCall(closeCall) and
+      closeCall.getReceiver() = sink
+    |
+      // `closeCall` is not guaranteed to be preceded during
+      // execution by a handled call to `os.File.Sync` on the same file handle.
+      not exists(DataFlow::Node syncReceiver, DataFlow::CallNode syncCall |
+        // 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() = sink and ssa.getAUse() = syncReceiver)
+      |
+        if exists(DeferStmt defer | defer.getCall() = closeCall.asExpr())
+        then
+          // When the call to `os.File.Close` is deferred it runs when the enclosing function
+          // returns, but the receiver of the deferred call is evaluated where the `defer`
+          // statement appears. It is therefore enough for the handled call to `os.File.Sync`
+          // to post-dominate that point, since that guarantees `os.File.Sync` runs before the
+          // deferred `os.File.Close` on every path on which the `os.File.Close` is registered.
+          // We cannot reuse the domination check below because the control-flow graph splices
+          // the deferred call in at the function exit, where it may be reachable along paths
+          // that do not pass through the call to `os.File.Sync`.
+          postDominatesNode(syncCall.asInstruction(), sink.asInstruction())
+        else
+          // Otherwise the call to `os.File.Close` is executed where it appears, so we require
+          // the handled call to `os.File.Sync` to dominate it.
+          syncCall.asInstruction().dominatesNode(closeCall.asInstruction())
+      )
+    )
+  }
 
   predicate observeDiffInformedIncrementalMode() { any() }
 
@@ -148,14 +170,12 @@ import UnhandledFileCloseFlow::PathGraph
 
 from
   UnhandledFileCloseFlow::PathNode source, DataFlow::CallNode openCall,
-  UnhandledFileCloseFlow::PathNode sink, DataFlow::CallNode closeCall
+  UnhandledFileCloseFlow::PathNode sink
 where
   // find data flow from an `os.OpenFile` call to an `os.File.Close` call
   // where the handle is writable
   UnhandledFileCloseFlow::flowPath(source, sink) and
-  isWritableFileHandle(source.getNode(), openCall) and
-  // get the `CallNode` corresponding to the sink
-  isCloseSink(sink.getNode(), closeCall)
+  isWritableFileHandle(source.getNode(), openCall)
 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()
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `go/unhandled-writable-file-close` ("Writable file handle closed without error handling") now produces fewer false positives. A deferred call to `Close` that is preceded on every execution path by a handled call to `Sync` on the same file handle is no longer flagged.
@@ -5,9 +5,9 @@
 | tests.go:15:3:15:3 | f | tests.go:46:5:46:76 | ... := ...[0] | tests.go:15:3:15: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:46:15:46:76 | call to OpenFile | call to OpenFile |
 | tests.go:57:3:57:3 | f | tests.go:55:5:55:78 | ... := ...[0] | tests.go:57:3:57: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:55:15:55:78 | call to OpenFile | call to OpenFile |
 | tests.go:69:3:69:3 | f | tests.go:67:5:67:76 | ... := ...[0] | tests.go:69:3:69: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:67:15:67:76 | call to OpenFile | call to OpenFile |
-| tests.go:111:9:111:9 | f | tests.go:109:5:109:78 | ... := ...[0] | tests.go:111:9:111: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:109:15:109:78 | call to OpenFile | call to OpenFile |
-| tests.go:130:3:130:3 | f | tests.go:126:5:126:78 | ... := ...[0] | tests.go:130:3:130: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:126:15:126:78 | call to OpenFile | call to OpenFile |
-| tests.go:151:8:151:8 | f | tests.go:147:2:147:74 | ... := ...[0] | tests.go:151:8:151: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:147:12:147:74 | call to OpenFile | call to OpenFile |
+| tests.go:126:9:126:9 | f | tests.go:124:5:124:78 | ... := ...[0] | tests.go:126:9:126: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:124:15:124:78 | call to OpenFile | call to OpenFile |
+| tests.go:145:3:145:3 | f | tests.go:141:5:141:78 | ... := ...[0] | tests.go:145:3:145: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:141:15:141:78 | call to OpenFile | call to OpenFile |
+| tests.go:166:8:166:8 | f | tests.go:162:2:162:74 | ... := ...[0] | tests.go:166:8:166: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:162:12:162:74 | call to OpenFile | call to OpenFile |
 edges
 | tests.go:9:24:9:24 | definition of f | tests.go:10:8:10:8 | f | provenance |  |
 | tests.go:13:32:13:32 | definition of f | tests.go:14:13:16:2 | capture variable f | provenance |  |
@@ -22,9 +22,9 @@ edges
 | tests.go:48:29:48:29 | f | tests.go:13:32:13:32 | definition of f | provenance |  |
 | tests.go:55:5:55:78 | ... := ...[0] | tests.go:57:3:57:3 | f | provenance | Src:MaD:1  |
 | tests.go:67:5:67:76 | ... := ...[0] | tests.go:69:3:69:3 | f | provenance | Src:MaD:1  |
-| tests.go:109:5:109:78 | ... := ...[0] | tests.go:111:9:111:9 | f | provenance | Src:MaD:1  |
-| tests.go:126:5:126:78 | ... := ...[0] | tests.go:130:3:130:3 | f | provenance | Src:MaD:1  |
-| tests.go:147:2:147:74 | ... := ...[0] | tests.go:151:8:151:8 | f | provenance | Src:MaD:1  |
+| tests.go:124:5:124:78 | ... := ...[0] | tests.go:126:9:126:9 | f | provenance | Src:MaD:1  |
+| tests.go:141:5:141:78 | ... := ...[0] | tests.go:145:3:145:3 | f | provenance | Src:MaD:1  |
+| tests.go:162:2:162:74 | ... := ...[0] | tests.go:166:8:166:8 | f | provenance | Src:MaD:1  |
 models
 | 1 | Source: os; ; false; OpenFile; ; ; ReturnValue[0]; file; manual |
 nodes
@@ -43,10 +43,10 @@ nodes
 | tests.go:57:3:57:3 | f | semmle.label | f |
 | tests.go:67:5:67:76 | ... := ...[0] | semmle.label | ... := ...[0] |
 | tests.go:69:3:69:3 | f | semmle.label | f |
-| tests.go:109:5:109:78 | ... := ...[0] | semmle.label | ... := ...[0] |
-| tests.go:111:9:111:9 | f | semmle.label | f |
-| tests.go:126:5:126:78 | ... := ...[0] | semmle.label | ... := ...[0] |
-| tests.go:130:3:130:3 | f | semmle.label | f |
-| tests.go:147:2:147:74 | ... := ...[0] | semmle.label | ... := ...[0] |
-| tests.go:151:8:151:8 | f | semmle.label | f |
+| tests.go:124:5:124:78 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:126:9:126:9 | f | semmle.label | f |
+| tests.go:141:5:141:78 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:145:3:145:3 | f | semmle.label | f |
+| tests.go:162:2:162:74 | ... := ...[0] | semmle.label | ... := ...[0] |
+| tests.go:166:8:166:8 | f | semmle.label | f |
 subpaths
@@ -104,6 +104,21 @@ func deferredCloseWithSync() {
 	}
 }
 
+func deferredCloseWithSync2() {
+	// open file for writing
+	if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); 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()
+
+		if err := f.Sync(); err != nil {
+			log.Fatal(err)
+		}
+	}
+	var a int
+	_ = a
+}
+
 func deferredCloseWithSyncEarlyReturn(n int) {
 	// open file for writing
 	if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil { // $ Source