Inline precededBySync

Author: mbg

go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql

@@ -90,13 +90,25 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
 /**
  * Holds if `os.File.Close` is called on `sink`.
  */
-predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode call) {
+predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
   // find calls to the os.File.Close function
-  call = any(CloseFileFun f).getACall() and
-  // that are deferred
-  unhandledCall(call) and
+  closeCall = any(CloseFileFun f).getACall() and
+  // that are unhandled
+  unhandledCall(closeCall) and
   // where the function is called on the sink
-  call.getReceiver() = 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)
+  )
 }
 
 /**
@@ -124,25 +136,6 @@ class UnhandledFileCloseDataFlowConfiguration extends DataFlow::Configuration {
   override predicate isSink(DataFlow::Node sink) { isCloseSink(sink, _) }
 }
 
-/**
- * Holds if a `node` is preceded by a call to `os.File.Sync`.
- */
-predicate precededBySync(DataFlow::Node closeReceiver, 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 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() = closeReceiver and ssa.getAUse() = syncReceiver)
-  )
-}
-
 from
   UnhandledFileCloseDataFlowConfiguration cfg, DataFlow::PathNode source,
   DataFlow::CallNode openCall, DataFlow::PathNode sink, DataFlow::CallNode closeCall
@@ -152,10 +145,7 @@ where
   cfg.hasFlowPath(source, sink) and
   isWritableFileHandle(source.getNode(), openCall) and
   // get the `CallNode` corresponding to the sink
-  isCloseSink(sink.getNode(), closeCall) and
-  // check that the call to `os.File.Close` is not preceded by a checked call to
-  // `os.File.Sync`
-  not precededBySync(sink.getNode(), closeCall)
+  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/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected

@@ -2,8 +2,6 @@ edges
 | tests.go:9:2:9:74 | ... := ...[0] : pointer type | tests.go:10:9:10:9 | f : pointer type |
 | tests.go:10:9:10:9 | f : pointer type | tests.go:43:5:43:38 | ... := ...[0] : pointer type |
 | tests.go:10:9:10:9 | f : pointer type | tests.go:60:5:60:38 | ... := ...[0] : pointer type |
-| tests.go:10:9:10:9 | f : pointer type | tests.go:86:5:86:38 | ... := ...[0] : pointer type |
-| tests.go:10:9:10:9 | f : pointer type | tests.go:96:5:96:38 | ... := ...[0] : pointer type |
 | tests.go:10:9:10:9 | f : pointer type | tests.go:108:5:108:38 | ... := ...[0] : pointer type |
 | tests.go:10:9:10:9 | f : pointer type | tests.go:124:5:124:38 | ... := ...[0] : pointer type |
 | tests.go:18:2:18:69 | return statement[0] : pointer type | tests.go:53:5:53:42 | ... := ...[0] : pointer type |
@@ -21,9 +19,6 @@ edges
 | tests.go:55:29:55:29 | f : pointer type | tests.go:25:32:25:32 | definition of f : pointer type |
 | tests.go:60:5:60:38 | ... := ...[0] : pointer type | tests.go:62:3:62:3 | f |
 | tests.go:70:5:70:42 | ... := ...[0] : pointer type | tests.go:72:3:72:3 | f |
-| tests.go:86:5:86:38 | ... := ...[0] : pointer type | tests.go:89:4:89:4 | f |
-| tests.go:86:5:86:38 | ... := ...[0] : pointer type | tests.go:91:3:91:3 | f |
-| tests.go:96:5:96:38 | ... := ...[0] : pointer type | tests.go:99:9:99:9 | f |
 | tests.go:108:5:108:38 | ... := ...[0] : pointer type | tests.go:110:9:110:9 | f |
 | tests.go:124:5:124:38 | ... := ...[0] : pointer type | tests.go:128:3:128:3 | f |
 nodes
@@ -45,11 +40,6 @@ nodes
 | tests.go:62:3:62:3 | f | semmle.label | f |
 | tests.go:70:5:70:42 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type |
 | tests.go:72:3:72:3 | f | semmle.label | f |
-| tests.go:86:5:86:38 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type |
-| tests.go:89:4:89:4 | f | semmle.label | f |
-| tests.go:91:3:91:3 | f | semmle.label | f |
-| tests.go:96:5:96:38 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type |
-| tests.go:99:9:99:9 | f | semmle.label | f |
 | tests.go:108:5:108:38 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type |
 | tests.go:110:9:110:9 | f | semmle.label | f |
 | tests.go:124:5:124:38 | ... := ...[0] : pointer type | semmle.label | ... := ...[0] : pointer type |