From abe38373da63d5b8fe2f62b36f4e89788bb07389 Mon Sep 17 00:00:00 2001 From: "Michael B. Gale" Date: Thu, 5 Jan 2023 16:18:30 +0000 Subject: [PATCH] Inline `precededBySync` --- .../UnhandledCloseWritableHandle.ql | 46 ++++++++----------- .../UnhandledCloseWritableHandle.expected | 10 ---- 2 files changed, 18 insertions(+), 38 deletions(-) diff --git a/go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql b/go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql index d15d883669a..c78a9d4a36f 100644 --- a/go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql +++ b/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() diff --git a/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected b/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected index bb2ab26addc..d00c51ba34f 100644 --- a/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected +++ b/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 |