@@ -55,7 +55,7 @@ class SyncFileFun extends Method {
5555
5656/**
5757 * Holds if a `call` to a function is "unhandled". That is, it is either
58- * deferred or its result is not assigned to anything .
58+ * deferred or used as an expression statement, so that its result is discarded .
5959 *
6060 * TODO: maybe we should check that something is actually done with the result
6161 */
@@ -77,7 +77,6 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
7777 // get the flags expression used for opening the file
7878 call .getArgument ( 1 ) = flags and
7979 // extract individual flags from the argument
80- // flag = flag.getAChild*() and
8180 flag = getConstants ( flags .asExpr ( ) ) and
8281 // check for one which signals that the handle will be writable
8382 // note that we are underestimating here, since the flags may be
@@ -87,27 +86,18 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
8786}
8887
8988/**
90- * Holds if `os.File.Close` is called on `sink`.
89+ * Holds if `postDominator` post-dominates `node` in the control-flow graph. That is,
90+ * every path from `node` to the exit of the enclosing function passes through
91+ * `postDominator`.
9192 */
92- predicate isCloseSink ( DataFlow:: Node sink , DataFlow:: CallNode closeCall ) {
93- // find calls to the os.File.Close function
94- closeCall = any ( CloseFileFun f ) .getACall ( ) and
95- // that are unhandled
96- unhandledCall ( closeCall ) and
97- // where the function is called on the sink
98- closeCall .getReceiver ( ) = sink and
99- // and check that it is not dominated by a call to `os.File.Sync`.
100- // TODO: fix this logic when `closeCall` is in a defer statement.
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 )
93+ pragma [ inline]
94+ predicate postDominatesNode ( ControlFlow:: Node postDominator , ControlFlow:: Node node ) {
95+ exists ( ReachableBasicBlock pdbb , ReachableBasicBlock nbb , int i , int j |
96+ postDominator = pdbb .getNode ( i ) and node = nbb .getNode ( j )
97+ |
98+ pdbb .strictlyPostDominates ( nbb )
99+ or
100+ pdbb = nbb and i >= j
111101 )
112102}
113103
@@ -127,7 +117,39 @@ predicate isHandledSync(DataFlow::Node sink, DataFlow::CallNode syncCall) {
127117module UnhandledFileCloseConfig implements DataFlow:: ConfigSig {
128118 predicate isSource ( DataFlow:: Node source ) { isWritableFileHandle ( source , _) }
129119
130- predicate isSink ( DataFlow:: Node sink ) { isCloseSink ( sink , _) }
120+ predicate isSink ( DataFlow:: Node sink ) {
121+ exists ( DataFlow:: CallNode closeCall |
122+ // `closeCall` is an unhandled call to `os.File.Close` on `sink`
123+ closeCall = any ( CloseFileFun f ) .getACall ( ) and
124+ unhandledCall ( closeCall ) and
125+ closeCall .getReceiver ( ) = sink
126+ |
127+ // `closeCall` is not guaranteed to be preceded during
128+ // execution by a handled call to `os.File.Sync` on the same file handle.
129+ not exists ( DataFlow:: Node syncReceiver , DataFlow:: CallNode syncCall |
130+ // check that the call to `os.File.Sync` is handled
131+ isHandledSync ( syncReceiver , syncCall ) and
132+ // check that `os.File.Sync` is called on the same object as `os.File.Close`
133+ exists ( DataFlow:: SsaNode ssa | ssa .getAUse ( ) = sink and ssa .getAUse ( ) = syncReceiver )
134+ |
135+ if exists ( DeferStmt defer | defer .getCall ( ) = closeCall .asExpr ( ) )
136+ then
137+ // When the call to `os.File.Close` is deferred it runs when the enclosing function
138+ // returns, but the receiver of the deferred call is evaluated where the `defer`
139+ // statement appears. It is therefore enough for the handled call to `os.File.Sync`
140+ // to post-dominate that point, since that guarantees `os.File.Sync` runs before the
141+ // deferred `os.File.Close` on every path on which the `os.File.Close` is registered.
142+ // We cannot reuse the domination check below because the control-flow graph splices
143+ // the deferred call in at the function exit, where it may be reachable along paths
144+ // that do not pass through the call to `os.File.Sync`.
145+ postDominatesNode ( syncCall .asInstruction ( ) , sink .asInstruction ( ) )
146+ else
147+ // Otherwise the call to `os.File.Close` is executed where it appears, so we require
148+ // the handled call to `os.File.Sync` to dominate it.
149+ syncCall .asInstruction ( ) .dominatesNode ( closeCall .asInstruction ( ) )
150+ )
151+ )
152+ }
131153
132154 predicate observeDiffInformedIncrementalMode ( ) { any ( ) }
133155
@@ -148,14 +170,12 @@ import UnhandledFileCloseFlow::PathGraph
148170
149171from
150172 UnhandledFileCloseFlow:: PathNode source , DataFlow:: CallNode openCall ,
151- UnhandledFileCloseFlow:: PathNode sink , DataFlow :: CallNode closeCall
173+ UnhandledFileCloseFlow:: PathNode sink
152174where
153175 // find data flow from an `os.OpenFile` call to an `os.File.Close` call
154176 // where the handle is writable
155177 UnhandledFileCloseFlow:: flowPath ( source , sink ) and
156- isWritableFileHandle ( source .getNode ( ) , openCall ) and
157- // get the `CallNode` corresponding to the sink
158- isCloseSink ( sink .getNode ( ) , closeCall )
178+ isWritableFileHandle ( source .getNode ( ) , openCall )
159179select sink , source , sink ,
160180 "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." ,
161181 openCall , openCall .toString ( )
0 commit comments