hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-06-19 03:41:07 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'main' into add-yaml-comments

Browse Source
This commit is contained in:
Mathias Vorreiter Pedersen
2026-06-17 12:07:21 +01:00
committed by GitHub
parent c12cf88c52 6161922ba4
commit 71daa20313
7 changed files with 84 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
CODEOWNERS
View File

@@ -2,7 +2,7 @@
* @github/code-scanning-alert-coverage
# CodeQL language libraries
/actions/ @github/codeql-dynamic
/actions/ @github/code-scanning-alert-coverage
/cpp/ @github/codeql-c-analysis
/csharp/ @github/codeql-csharp
/csharp/autobuilder/Semmle.Autobuild.Cpp @github/codeql-c-extractor @github/code-scanning-language-coverage

74
go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql
View File

@@ -55,7 +55,7 @@ class SyncFileFun extends Method {
/**
* Holds if a `call` to a function is "unhandled". That is, it is either
* deferred or its result is not assigned to anything.
* deferred or used as an expression statement, so that its result is discarded.
*
* TODO: maybe we should check that something is actually done with the result
*/
@@ -77,7 +77,6 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
// get the flags expression used for opening the file
call.getArgument(1) = flags and
// extract individual flags from the argument
// flag = flag.getAChild*() and
flag = getConstants(flags.asExpr()) and
// check for one which signals that the handle will be writable
// note that we are underestimating here, since the flags may be
@@ -87,27 +86,18 @@ predicate isWritableFileHandle(DataFlow::Node source, DataFlow::CallNode call) {
}
/**
* Holds if `os.File.Close` is called on `sink`.
* Holds if `postDominator` post-dominates `node` in the control-flow graph. That is,
* every path from `node` to the exit of the enclosing function passes through
* `postDominator`.
*/
predicate isCloseSink(DataFlow::Node sink, DataFlow::CallNode closeCall) {
// find calls to the os.File.Close function
closeCall = any(CloseFileFun f).getACall() and
// that are unhandled
unhandledCall(closeCall) and
// 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
// 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)
pragma[inline]
predicate postDominatesNode(ControlFlow::Node postDominator, ControlFlow::Node node) {
exists(ReachableBasicBlock pdbb, ReachableBasicBlock nbb, int i, int j |
postDominator = pdbb.getNode(i) and node = nbb.getNode(j)
|
pdbb.strictlyPostDominates(nbb)
or
pdbb = nbb and i >= j
)
}
@@ -127,7 +117,39 @@ predicate isHandledSync(DataFlow::Node sink, DataFlow::CallNode syncCall) {
module UnhandledFileCloseConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { isWritableFileHandle(source, _) }
predicate isSink(DataFlow::Node sink) { isCloseSink(sink, _) }
predicate isSink(DataFlow::Node sink) {
exists(DataFlow::CallNode closeCall |
// `closeCall` is an unhandled call to `os.File.Close` on `sink`
closeCall = any(CloseFileFun f).getACall() and
unhandledCall(closeCall) and
closeCall.getReceiver() = sink
|
// `closeCall` is not guaranteed to be preceded during
// execution by a handled call to `os.File.Sync` on the same file handle.
not exists(DataFlow::Node syncReceiver, DataFlow::CallNode syncCall |
// check that the call to `os.File.Sync` is handled
isHandledSync(syncReceiver, syncCall) 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)
|
if exists(DeferStmt defer | defer.getCall() = closeCall.asExpr())
then
// When the call to `os.File.Close` is deferred it runs when the enclosing function
// returns, but the receiver of the deferred call is evaluated where the `defer`
// statement appears. It is therefore enough for the handled call to `os.File.Sync`
// to post-dominate that point, since that guarantees `os.File.Sync` runs before the
// deferred `os.File.Close` on every path on which the `os.File.Close` is registered.
// We cannot reuse the domination check below because the control-flow graph splices
// the deferred call in at the function exit, where it may be reachable along paths
// that do not pass through the call to `os.File.Sync`.
postDominatesNode(syncCall.asInstruction(), sink.asInstruction())
else
// Otherwise the call to `os.File.Close` is executed where it appears, so we require
// the handled call to `os.File.Sync` to dominate it.
syncCall.asInstruction().dominatesNode(closeCall.asInstruction())
)
)
}
predicate observeDiffInformedIncrementalMode() { any() }
@@ -148,14 +170,12 @@ import UnhandledFileCloseFlow::PathGraph
from
UnhandledFileCloseFlow::PathNode source, DataFlow::CallNode openCall,
UnhandledFileCloseFlow::PathNode sink, DataFlow::CallNode closeCall
UnhandledFileCloseFlow::PathNode sink
where
// find data flow from an `os.OpenFile` call to an `os.File.Close` call
// where the handle is writable
UnhandledFileCloseFlow::flowPath(source, sink) and
isWritableFileHandle(source.getNode(), openCall) and
// get the `CallNode` corresponding to the sink
isCloseSink(sink.getNode(), closeCall)
isWritableFileHandle(source.getNode(), openCall)
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()

4
go/ql/src/change-notes/2026-06-04-unhandled-writable-file-close.md Normal file
View File

@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The query `go/unhandled-writable-file-close` ("Writable file handle closed without error handling") now produces fewer false positives. A deferred call to `Close` that is preceded on every execution path by a handled call to `Sync` on the same file handle is no longer flagged.

24
go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected
View File

@@ -5,9 +5,9 @@
| 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 |
| tests.go:126:9:126:9 | f | tests.go:124:5:124:78 | ... := ...[0] | tests.go:126:9:126: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:124:15:124:78 | call to OpenFile | call to OpenFile |
| tests.go:145:3:145:3 | f | tests.go:141:5:141:78 | ... := ...[0] | tests.go:145:3:145: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:141:15:141:78 | call to OpenFile | call to OpenFile |
| tests.go:166:8:166:8 | f | tests.go:162:2:162:74 | ... := ...[0] | tests.go:166:8:166: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:162:12:162:74 | call to OpenFile | call to OpenFile |
edges
| 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 | |
@@ -22,9 +22,9 @@ edges
| 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 |
| tests.go:124:5:124:78 | ... := ...[0] | tests.go:126:9:126:9 | f | provenance | Src:MaD:1 |
| tests.go:141:5:141:78 | ... := ...[0] | tests.go:145:3:145:3 | f | provenance | Src:MaD:1 |
| tests.go:162:2:162:74 | ... := ...[0] | tests.go:166:8:166:8 | f | provenance | Src:MaD:1 |
models
| 1 | Source: os; ; false; OpenFile; ; ; ReturnValue[0]; file; manual |
nodes
@@ -43,10 +43,10 @@ nodes
| 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 |
| tests.go:124:5:124:78 | ... := ...[0] | semmle.label | ... := ...[0] |
| tests.go:126:9:126:9 | f | semmle.label | f |
| tests.go:141:5:141:78 | ... := ...[0] | semmle.label | ... := ...[0] |
| tests.go:145:3:145:3 | f | semmle.label | f |
| tests.go:162:2:162:74 | ... := ...[0] | semmle.label | ... := ...[0] |
| tests.go:166:8:166:8 | f | semmle.label | f |
subpaths

15
go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go
View File

@@ -104,6 +104,21 @@ func deferredCloseWithSync() {
}
}
func deferredCloseWithSync2() {
// open file for writing
if f, err := os.OpenFile("foo.txt", os.O_WRONLY|os.O_TRUNC|os.O_CREATE, 0666); err != nil {
// a call to `Close` is deferred, but we have a call to `Sync` later which
// precedes the call to `Close` during execution
defer f.Close()
if err := f.Sync(); err != nil {
log.Fatal(err)
}
}
var a int
_ = a
}
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 { // $ Source

4
swift/ql/integration-tests/osx/hello-xcode/Files.ql
View File

@@ -1,5 +1,7 @@
import swift
from File f
where exists(f.getRelativePath()) or f instanceof UnknownFile
where
(exists(f.getRelativePath()) or f instanceof UnknownFile) and
not f.getBaseName() = "<module-includes>"
select f

2
swift/tools/tracing-config.lua
View File

@@ -54,6 +54,8 @@ function RegisterExtractorPack(id)
strip_unsupported_arg(args, '-experimental-skip-non-inlinable-function-bodies-without-types', 0)
strip_unsupported_clang_arg(args, '-ivfsstatcache', 1)
strip_unsupported_clang_arg(args, '-fno-odr-hash-protocols', 0)
strip_unsupported_clang_arg(args, '-fobjc-msgsend-selector-stubs', 0)
strip_unsupported_clang_arg(args, '-fstack-check', 0)
strip_unsupported_clang_arg(args, '-clang-vendor-feature=+disableNonDependentMemberExprInCurrentInstantiation', 0)
strip_unsupported_clang_arg(args, '-clang-vendor-feature=+enableAggressiveVLAFolding', 0)
strip_unsupported_clang_arg(args, '-clang-vendor-feature=+revert09abecef7bbf', 0)