Skip to content

Go: Add test for FP in go/unhandled-writable-file-close #18940

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 11, 2025
Merged

Go: Add test for FP in go/unhandled-writable-file-close #18940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63bfa36
Convert to inline expectations test
owen-mc Mar 5, 2025
7b29123
Add failing test for `os.File.Sync` with defered `Close` calls
owen-mc Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
88 changes: 46 additions & 42 deletions ...tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
4 changes: 3 additions & 1 deletion ...ry-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
query: InconsistentCode/UnhandledCloseWritableHandle.ql
postprocess: utils/test/PrettyPrintModels.ql
postprocess:
- utils/test/PrettyPrintModels.ql
- utils/test/InlineExpectationsTestQuery.ql
51 changes: 39 additions & 12 deletions go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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()
}