diff --git a/go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql b/go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql index 8846f52fb743..211cf4b3985e 100644 --- a/go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql +++ b/go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql @@ -97,6 +97,7 @@ predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) { // 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 diff --git a/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected b/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected index 672e1a5cc8f1..41034c557961 100644 --- a/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected +++ b/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected @@ -1,48 +1,52 @@ #select -| tests.go:9:8:9:8 | f | tests.go:31:5:31:78 | ... := ...[0] | tests.go:9:8:9: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:31:15:31:78 | call to OpenFile | call to OpenFile | -| tests.go:9:8:9:8 | f | tests.go:45:5:45:76 | ... := ...[0] | tests.go:9:8:9: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:45:15:45:76 | call to OpenFile | call to OpenFile | -| tests.go:14:3:14:3 | f | tests.go:31:5:31:78 | ... := ...[0] | tests.go:14:3:14: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:31:15:31:78 | call to OpenFile | call to OpenFile | -| tests.go:14:3:14:3 | f | tests.go:45:5:45:76 | ... := ...[0] | tests.go:14:3:14: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:45:15:45:76 | call to OpenFile | call to OpenFile | -| tests.go:56:3:56:3 | f | tests.go:54:5:54:78 | ... := ...[0] | tests.go:56:3:56: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:54:15:54:78 | call to OpenFile | call to OpenFile | -| tests.go:68:3:68:3 | f | tests.go:66:5:66:76 | ... := ...[0] | tests.go:68:3:68: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:66:15:66:76 | call to OpenFile | call to OpenFile | -| tests.go:110:9:110:9 | f | tests.go:108:5:108:78 | ... := ...[0] | tests.go:110:9:110: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:108:15:108:78 | call to OpenFile | call to OpenFile | -| tests.go:129:3:129:3 | f | tests.go:125:5:125:78 | ... := ...[0] | tests.go:129:3:129: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:125:15:125:78 | call to OpenFile | call to OpenFile | +| tests.go:10:8:10:8 | f | tests.go:32:5:32:78 | ... := ...[0] | tests.go:10:8:10: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:32:15:32:78 | call to OpenFile | call to OpenFile | +| tests.go:10:8:10:8 | f | tests.go:46:5:46:76 | ... := ...[0] | tests.go:10:8:10: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:46:15:46:76 | call to OpenFile | call to OpenFile | +| tests.go:15:3:15:3 | f | tests.go:32:5:32:78 | ... := ...[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:32:15:32:78 | call to OpenFile | call to OpenFile | +| 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 | edges -| tests.go:8:24:8:24 | definition of f | tests.go:9:8:9:8 | f | provenance | | -| tests.go:12:32:12:32 | definition of f | tests.go:13:13:15:2 | capture variable f | provenance | | -| tests.go:13:13:15:2 | capture variable f | tests.go:14:3:14:3 | f | provenance | | -| tests.go:31:5:31:78 | ... := ...[0] | tests.go:32:21:32:21 | f | provenance | Src:MaD:1 | -| tests.go:31:5:31:78 | ... := ...[0] | tests.go:33:29:33:29 | f | provenance | Src:MaD:1 | -| tests.go:32:21:32:21 | f | tests.go:8:24:8:24 | definition of f | provenance | | -| tests.go:33:29:33:29 | f | tests.go:12:32:12:32 | definition of f | provenance | | -| tests.go:45:5:45:76 | ... := ...[0] | tests.go:46:21:46:21 | f | provenance | Src:MaD:1 | -| tests.go:45:5:45:76 | ... := ...[0] | tests.go:47:29:47:29 | f | provenance | Src:MaD:1 | -| tests.go:46:21:46:21 | f | tests.go:8:24:8:24 | definition of f | provenance | | -| tests.go:47:29:47:29 | f | tests.go:12:32:12:32 | definition of f | provenance | | -| tests.go:54:5:54:78 | ... := ...[0] | tests.go:56:3:56:3 | f | provenance | Src:MaD:1 | -| tests.go:66:5:66:76 | ... := ...[0] | tests.go:68:3:68:3 | f | provenance | Src:MaD:1 | -| tests.go:108:5:108:78 | ... := ...[0] | tests.go:110:9:110:9 | f | provenance | Src:MaD:1 | -| tests.go:125:5:125:78 | ... := ...[0] | tests.go:129:3:129:3 | f | provenance | Src:MaD:1 | +| 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 | | +| tests.go:14:13:16:2 | capture variable f | tests.go:15:3:15:3 | f | provenance | | +| tests.go:32:5:32:78 | ... := ...[0] | tests.go:33:21:33:21 | f | provenance | Src:MaD:1 | +| tests.go:32:5:32:78 | ... := ...[0] | tests.go:34:29:34:29 | f | provenance | Src:MaD:1 | +| tests.go:33:21:33:21 | f | tests.go:9:24:9:24 | definition of f | provenance | | +| tests.go:34:29:34:29 | f | tests.go:13:32:13:32 | definition of f | provenance | | +| tests.go:46:5:46:76 | ... := ...[0] | tests.go:47:21:47:21 | f | provenance | Src:MaD:1 | +| tests.go:46:5:46:76 | ... := ...[0] | tests.go:48:29:48:29 | f | provenance | Src:MaD:1 | +| tests.go:47:21:47:21 | f | tests.go:9:24:9:24 | definition of f | provenance | | +| 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 | models | 1 | Source: os; ; false; OpenFile; ; ; ReturnValue[0]; file; manual | nodes -| tests.go:8:24:8:24 | definition of f | semmle.label | definition of f | -| tests.go:9:8:9:8 | f | semmle.label | f | -| tests.go:12:32:12:32 | definition of f | semmle.label | definition of f | -| tests.go:13:13:15:2 | capture variable f | semmle.label | capture variable f | -| tests.go:14:3:14:3 | f | semmle.label | f | -| tests.go:31:5:31:78 | ... := ...[0] | semmle.label | ... := ...[0] | -| tests.go:32:21:32:21 | f | semmle.label | f | -| tests.go:33:29:33:29 | f | semmle.label | f | -| tests.go:45:5:45:76 | ... := ...[0] | semmle.label | ... := ...[0] | -| tests.go:46:21:46:21 | f | semmle.label | f | -| tests.go:47:29:47:29 | f | semmle.label | f | -| tests.go:54:5:54:78 | ... := ...[0] | semmle.label | ... := ...[0] | -| tests.go:56:3:56:3 | f | semmle.label | f | -| tests.go:66:5:66:76 | ... := ...[0] | semmle.label | ... := ...[0] | -| tests.go:68:3:68:3 | f | semmle.label | f | -| tests.go:108:5:108:78 | ... := ...[0] | semmle.label | ... := ...[0] | -| tests.go:110:9:110:9 | f | semmle.label | f | -| tests.go:125:5:125:78 | ... := ...[0] | semmle.label | ... := ...[0] | -| tests.go:129:3:129:3 | f | semmle.label | f | +| tests.go:9:24:9:24 | definition of f | semmle.label | definition of f | +| tests.go:10:8:10:8 | f | semmle.label | f | +| tests.go:13:32:13:32 | definition of f | semmle.label | definition of f | +| tests.go:14:13:16:2 | capture variable f | semmle.label | capture variable f | +| tests.go:15:3:15:3 | f | semmle.label | f | +| tests.go:32:5:32:78 | ... := ...[0] | semmle.label | ... := ...[0] | +| tests.go:33:21:33:21 | f | semmle.label | f | +| tests.go:34:29:34:29 | f | semmle.label | f | +| tests.go:46:5:46:76 | ... := ...[0] | semmle.label | ... := ...[0] | +| tests.go:47:21:47:21 | f | semmle.label | f | +| tests.go:48:29:48:29 | f | semmle.label | f | +| tests.go:55:5:55:78 | ... := ...[0] | semmle.label | ... := ...[0] | +| 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 | subpaths diff --git a/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.qlref b/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.qlref index 82300c2182c8..af272c9022f2 100644 --- a/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.qlref +++ b/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.qlref @@ -1,2 +1,4 @@ query: InconsistentCode/UnhandledCloseWritableHandle.ql -postprocess: utils/test/PrettyPrintModels.ql +postprocess: +- utils/test/PrettyPrintModels.ql +- utils/test/InlineExpectationsTestQuery.ql diff --git a/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go b/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go index 36c78863b624..ec74b12e5a3d 100644 --- a/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go +++ b/go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go @@ -1,17 +1,18 @@ package test import ( + "io" "log" "os" ) func closeFileDeferred(f *os.File) { - defer f.Close() // NOT OK, if `f` is writable + defer f.Close() // $ Alert=w Alert=rw } func closeFileDeferredIndirect(f *os.File) { var cont = func() { - f.Close() // NOT OK, if `f` is writable + f.Close() // $ Alert=w Alert=rw } defer cont() @@ -28,7 +29,7 @@ func closeFileDeferredIndirectReturn(f *os.File) { func deferredCalls() { // open file for writing - if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil { + if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil { // $ Source=w closeFileDeferred(f) // NOT OK closeFileDeferredIndirect(f) // NOT OK closeFileDeferredIndirectReturn(f) // OK - the error is not discarded at the call to Close (though it is discarded later) @@ -42,7 +43,7 @@ func deferredCalls() { } // open file for reading and writing - if f, err := os.OpenFile("foo.txt", os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0666); err != nil { + if f, err := os.OpenFile("foo.txt", os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0666); err != nil { // $ Source=rw closeFileDeferred(f) // NOT OK closeFileDeferredIndirect(f) // NOT OK closeFileDeferredIndirectReturn(f) // OK - the error is not discarded at the call to Close (though it is discarded later) @@ -51,9 +52,9 @@ func deferredCalls() { func notDeferred() { // open file for writing - if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil { + if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil { // $ Source // the handle is write-only and we don't check if `Close` succeeds - f.Close() // NOT OK + f.Close() // $ Alert } // open file for reading @@ -63,9 +64,9 @@ func notDeferred() { } // open file for reading and writing - if f, err := os.OpenFile("foo.txt", os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0666); err != nil { + if f, err := os.OpenFile("foo.txt", os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0666); err != nil { // $ Source // the handle is read-write and we don't check if `Close` succeeds - f.Close() // NOT OK + f.Close() // $ Alert } } @@ -105,9 +106,9 @@ func deferredCloseWithSync() { 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 { + if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil { // $ Source // a call to `Close` is deferred - defer f.Close() // NOT OK + defer f.Close() // $ Alert if n > 100 { return @@ -122,10 +123,36 @@ func deferredCloseWithSyncEarlyReturn(n int) { func unhandledSync() { // open file for writing - if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil { + if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil { // $ Source // 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 + f.Close() // $ Alert + } +} + +func returnedSync() error { + // open file for writing + f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666) + if 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 + return err + } + defer f.Close() + return f.Sync() +} + +func copyFile(destFile string, mode os.FileMode, src io.Reader) error { + f, err := os.OpenFile(destFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) // $ Source + if err != nil { + return err + } + defer f.Close() // $ SPURIOUS: Alert + + _, err = io.Copy(f, src) + if err != nil { + return err } + return f.Sync() }