Skip to content

Commit 87da0a5

Browse files
ex0dus-0xex0dus-0x
committed
Fix DeferReleaseInLoop
1 parent 80faff6 commit 87da0a5

4 files changed

Lines changed: 189 additions & 112 deletions

File tree

‎go/src/security/DeferReleaseInLoop/DeferReleaseInLoop.ql‎

Lines changed: 61 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,44 @@
1111
*/
1212

1313
import go
14-
import ResourceModel
14+
15+
/**
16+
* A function that acquires a resource that could leak
17+
*/
18+
class ResourceAcquisition extends Function {
19+
ResourceAcquisition() {
20+
this.hasQualifiedName("os", ["Open", "OpenFile", "Create", "CreateTemp", "NewFile", "Pipe"])
21+
or
22+
this.hasQualifiedName("net", ["Dial", "DialTimeout", "Listen", "ListenPacket"])
23+
or
24+
this.hasQualifiedName("net", ["FileConn", "FileListener", "FilePacketConn"])
25+
or
26+
this.(Method).hasQualifiedName("net", "Dialer", ["Dial", "DialContext"])
27+
or
28+
this.hasQualifiedName("net/http", ["Get", "Post", "PostForm", "Head"])
29+
or
30+
this.(Method).hasQualifiedName("net/http", "Client", ["Do", "Get", "Post", "PostForm", "Head"])
31+
or
32+
this.hasQualifiedName("crypto/tls", ["Dial", "DialWithDialer", "Client", "Server"])
33+
or
34+
this.(Method).hasQualifiedName("crypto/tls", "Dialer", "DialContext")
35+
or
36+
this.hasQualifiedName("compress/gzip", ["NewReader", "NewWriter", "NewWriterLevel"])
37+
or
38+
this.hasQualifiedName("compress/zlib",
39+
["NewReader", "NewWriter", "NewWriterLevel", "NewWriterLevelDict"])
40+
or
41+
this.hasQualifiedName("compress/flate", ["NewReader", "NewWriter"])
42+
or
43+
this.hasQualifiedName("compress/lzw", ["NewReader", "NewWriter"])
44+
or
45+
this.hasQualifiedName("archive/zip", "OpenReader")
46+
}
47+
}
1548

1649
/**
1750
* Holds if `inner` is a (transitive) child of `outer` without crossing
18-
* a function literal boundary. This ensures we don't catch defers inside
19-
* closures that are invoked per-iteration.
51+
* a function literal boundary.
2052
*/
2153
predicate parentWithoutFuncLit(AstNode inner, AstNode outer) {
2254
inner.getParent() = outer and not inner instanceof FuncLit
@@ -27,22 +59,35 @@ predicate parentWithoutFuncLit(AstNode inner, AstNode outer) {
2759
)
2860
}
2961

62+
/** Holds if `node` is inside the body of `loop`, not crossing closures. */
63+
predicate inLoopBody(AstNode node, LoopStmt loop) {
64+
parentWithoutFuncLit(node, loop.(ForStmt).getBody())
65+
or
66+
parentWithoutFuncLit(node, loop.(RangeStmt).getBody())
67+
}
68+
3069
/**
31-
* A `defer` statement that defers a resource close/release call.
70+
* Gets the innermost identifier used as the receiver in `defer x.Close()`.
71+
* For `defer f.Close()`, this is `f`.
72+
* For `defer resp.Body.Close()`, this is `resp`.
3273
*/
33-
class DeferredResourceClose extends DeferStmt {
34-
ResourceCloseMethod closeMethod;
35-
36-
DeferredResourceClose() { closeMethod = this.getCall().getTarget() }
37-
38-
string getMethodName() { result = closeMethod.getName() }
74+
DataFlow::Node deferCloseReceiverBase(DeferStmt d) {
75+
d.getCall().getTarget().getName() = "Close" and
76+
exists(Expr base | base = d.getCall().getCalleeExpr().(SelectorExpr).getBase() |
77+
// defer f.Close() — base is an identifier
78+
result.asExpr() = base.(Ident)
79+
or
80+
// defer resp.Body.Close() — base is a selector, take its base identifier
81+
result.asExpr() = base.(SelectorExpr).getBase().(Ident)
82+
)
3983
}
4084

41-
from DeferredResourceClose deferStmt, LoopStmt loop
85+
from DeferStmt deferStmt, DataFlow::CallNode acquisition, LoopStmt loop
4286
where
43-
parentWithoutFuncLit(deferStmt, loop.(ForStmt).getBody())
44-
or
45-
parentWithoutFuncLit(deferStmt, loop.(RangeStmt).getBody())
87+
acquisition.getTarget() instanceof ResourceAcquisition and
88+
inLoopBody(acquisition.asExpr(), loop) and
89+
inLoopBody(deferStmt, loop) and
90+
DataFlow::localFlow(acquisition.getResult(0), deferCloseReceiverBase(deferStmt))
4691
select deferStmt,
47-
"Deferred " + deferStmt.getMethodName() +
48-
"() in a loop will not execute until the function returns, leaking resources across iterations."
92+
"Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations.",
93+
acquisition, acquisition.getTarget().getName() + "()"

‎go/src/security/DeferReleaseInLoop/ResourceModel.qll‎

Lines changed: 0 additions & 71 deletions
This file was deleted.

‎go/test/query-tests/security/DeferReleaseInLoop/DeferReleaseInLoop.expected‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,14 @@
1-
| DeferReleaseInLoop.go:19:3:19:17 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. |
2-
| DeferReleaseInLoop.go:31:3:31:17 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. |
3-
| DeferReleaseInLoop.go:43:3:43:25 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. |
4-
| DeferReleaseInLoop.go:55:3:55:20 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. |
5-
| DeferReleaseInLoop.go:67:3:67:18 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. |
6-
| DeferReleaseInLoop.go:84:3:84:18 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. |
7-
| DeferReleaseInLoop.go:85:3:85:17 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. |
8-
| DeferReleaseInLoop.go:98:4:98:18 | defer statement | Deferred Close() in a loop will not execute until the function returns, leaking resources across iterations. |
1+
| DeferReleaseInLoop.go:22:3:22:17 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:18:13:18:25 | call to Open | Open() |
2+
| DeferReleaseInLoop.go:34:3:34:17 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:30:13:30:52 | call to Create | Create() |
3+
| DeferReleaseInLoop.go:46:3:46:17 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:42:13:42:45 | call to OpenFile | OpenFile() |
4+
| DeferReleaseInLoop.go:58:3:58:17 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:54:13:54:39 | call to CreateTemp | CreateTemp() |
5+
| DeferReleaseInLoop.go:70:3:70:25 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:66:16:66:28 | call to Get | Get() |
6+
| DeferReleaseInLoop.go:83:3:83:25 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:79:16:79:29 | call to Do | Do() |
7+
| DeferReleaseInLoop.go:95:3:95:20 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:91:16:91:36 | call to Dial | Dial() |
8+
| DeferReleaseInLoop.go:107:3:107:18 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:103:14:103:40 | call to Listen | Listen() |
9+
| DeferReleaseInLoop.go:124:3:124:18 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:119:14:119:30 | call to NewReader | NewReader() |
10+
| DeferReleaseInLoop.go:125:3:125:17 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:115:13:115:25 | call to Open | Open() |
11+
| DeferReleaseInLoop.go:142:3:142:18 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:137:14:137:30 | call to NewReader | NewReader() |
12+
| DeferReleaseInLoop.go:143:3:143:17 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:133:13:133:25 | call to Open | Open() |
13+
| DeferReleaseInLoop.go:155:3:155:18 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:151:14:151:33 | call to OpenReader | OpenReader() |
14+
| DeferReleaseInLoop.go:168:4:168:18 | defer statement | Deferred Close() of resource acquired from $@ in a loop will not execute until the function returns, leaking resources across iterations. | DeferReleaseInLoop.go:164:14:164:26 | call to Open | Open() |

0 commit comments

Comments
 (0)