FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Simplify handling of wrapper classes and exception flow + improve qld… · github/codeql@b2acfbc · GitHub

  1. github
  2. codeql

Repository navigation

Commit b2acfbc

Browse files
joefarebrother
committed
Simplify handling of wrapper classes and exception flow + improve qldoc and annotate tests.
1 parent f8a0b1c commit b2acfbc

5 files changed

Lines changed: 38 additions & 34 deletions

File tree

‎python/ql/src/Resources/FileNotAlwaysClosed.qhelp‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,16 @@
44
<qhelp>
55

66
<overview>
7-
<p>When a file is opened, it should always be closed. Failure to close files could result in loss of data or resource leaks.</p>
8-
7+
<p>When a file is opened, it should always be closed.
8+
</p>
9+
<p>A file opened for writing that is not closed when the application exits may result in data loss, where not all of the data written may be saved to the file.
10+
A file opened for reading or writing that is not closed may also use up file descriptors, which is a resource leak that in long running applications could lead to a failure to open additional files.
11+
</p>
912
</overview>
1013
<recommendation>
1114

1215
<p>Ensure that opened files are always closed, including when an exception could be raised.
13-
The best practice is to use a <code>with</code> statement to automatically clean up resources.
16+
The best practice is often to use a <code>with</code> statement to automatically clean up resources.
1417
Otherwise, ensure that <code>.close()</code> is called in a <code>try...except</code> or <code>try...finally</code>
1518
block to handle any possible exceptions.
1619
</p>

‎python/ql/src/Resources/FileNotAlwaysClosed.ql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,4 @@ where
2222
or
2323
fileMayNotBeClosedOnException(fo, _) and
2424
msg = "File may not be closed if an exception is raised."
25-
select fo.getLocalSource(), msg
25+
select fo, msg

‎python/ql/src/Resources/FileNotAlwaysClosedQuery.qll‎

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,17 +24,10 @@ private DataFlow::TypeTrackingNode fileOpenInstance(DataFlow::TypeTracker t) {
2424
/** A call that returns an instance of an open file object. */
2525
class FileOpen extends DataFlow::CallCfgNode {
2626
FileOpen() { fileOpenInstance(DataFlow::TypeTracker::end()).flowsTo(this) }
27-
28-
/** Gets the local source of this file object, through any wrapper calls. */
29-
FileOpen getLocalSource() {
30-
if this instanceof FileWrapperCall
31-
then result = this.(FileWrapperCall).getWrapped().getLocalSource()
32-
else result = this
33-
}
3427
}
3528

3629
/** A call that may wrap a file object in a wrapper class or `os.fdopen`. */
37-
class FileWrapperCall extends FileOpenSource, DataFlow::CallCfgNode {
30+
class FileWrapperCall extends DataFlow::CallCfgNode {
3831
FileOpen wrapped;
3932

4033
FileWrapperCall() {
@@ -53,14 +46,11 @@ class FileWrapperCall extends FileOpenSource, DataFlow::CallCfgNode {
5346
abstract class FileClose extends DataFlow::CfgNode {
5447
/** Holds if this file close will occur if an exception is thrown at `e`. */
5548
predicate guardsExceptions(Expr e) {
56-
exists(Try try |
57-
e = try.getAStmt().getAChildNode*() and
58-
(
59-
this.asExpr() = try.getAHandler().getAChildNode*()
60-
or
61-
this.asExpr() = try.getAFinalstmt().getAChildNode*()
62-
)
63-
)
49+
this.asCfgNode() =
50+
DataFlow::exprNode(e).asCfgNode().getAnExceptionalSuccessor().getASuccessor*()
51+
or
52+
// the expression is after the close call
53+
DataFlow::exprNode(e).asCfgNode() = this.asCfgNode().getASuccessor*()
6454
}
6555
}
6656

@@ -95,8 +85,20 @@ private predicate mayRaiseWithFile(DataFlow::CfgNode node) {
9585
not node instanceof FileClose
9686
}
9787

88+
/** Holds if data flows from `nodeFrom` to `nodeTo` in one step that also includes file wrapper classes. */
89+
private predicate fileLocalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
90+
DataFlow::localFlowStep(nodeFrom, nodeTo)
91+
or
92+
exists(FileWrapperCall fw | nodeFrom = fw.getWrapped() and nodeTo = fw)
93+
}
94+
95+
/** Holds if data flows from `source` to `sink`, including file wrapper classes. */
96+
private predicate fileLocalFlow(DataFlow::Node source, DataFlow::Node sink) {
97+
fileLocalFlowStep*(source, sink)
98+
}
99+
98100
/** Holds if the file opened at `fo` is closed. */
99-
predicate fileIsClosed(FileOpen fo) { exists(FileClose fc | DataFlow::localFlow(fo, fc)) }
101+
predicate fileIsClosed(FileOpen fo) { exists(FileClose fc | fileLocalFlow(fo, fc)) }
100102

101103
/** Holds if the file opened at `fo` is returned to the caller. This makes the caller responsible for closing the file. */
102104
predicate fileIsReturned(FileOpen fo) {
@@ -108,27 +110,26 @@ predicate fileIsReturned(FileOpen fo) {
108110
or
109111
retVal = ret.getValue().(Tuple).getAnElt()
110112
) and
111-
DataFlow::localFlow(fo, DataFlow::exprNode(retVal))
113+
fileLocalFlow(fo, DataFlow::exprNode(retVal))
112114
)
113115
}
114116

115117
/** Holds if the file opened at `fo` is stored in a field. We assume that another method is then responsible for closing the file. */
116118
predicate fileIsStoredInField(FileOpen fo) {
117-
exists(DataFlow::AttrWrite aw | DataFlow::localFlow(fo, aw.getValue()))
119+
exists(DataFlow::AttrWrite aw | fileLocalFlow(fo, aw.getValue()))
118120
}
119121

120122
/** Holds if the file opened at `fo` is not closed, and is expected to be closed. */
121123
predicate fileNotClosed(FileOpen fo) {
122124
not fileIsClosed(fo) and
123125
not fileIsReturned(fo) and
124-
not fileIsStoredInField(fo) and
125-
not exists(FileWrapperCall fwc | fo = fwc.getWrapped())
126+
not fileIsStoredInField(fo)
126127
}
127128

128129
predicate fileMayNotBeClosedOnException(FileOpen fo, DataFlow::Node raises) {
129130
fileIsClosed(fo) and
130131
mayRaiseWithFile(raises) and
131-
DataFlow::localFlow(fo, raises) and
132+
fileLocalFlow(fo, raises) and
132133
not exists(FileClose fc |
133134
DataFlow::localFlow(fo, fc) and
134135
fc.guardsExceptions(raises.asExpr())

‎python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,10 @@ def closed7():
4646
def not_closed8():
4747
f8 = None
4848
try:
49-
f8 = open("filename") # $ MISSING:notClosedOnException
49+
f8 = open("filename") # $ MISSING:notClosedOnException
5050
f8.write("Error could occur")
5151
finally:
52-
if f8 is None:
52+
if f8 is None: # We don't precisely consider this condition, so this result is MISSING. However, this seems uncommon.
5353
f8.close()
5454

5555
def not_closed9():
@@ -58,7 +58,7 @@ def not_closed9():
5858
f9 = open("filename") # $ MISSING:notAlwaysClosed
5959
f9.write("Error could occur")
6060
finally:
61-
if not f9:
61+
if not f9: # We don't precisely consider this condition, so this result is MISSING.However, this seems uncommon.
6262
f9.close()
6363

6464
def not_closed_but_cant_tell_locally():
@@ -81,7 +81,7 @@ def not_closed11():
8181
f11.write("IOError could occur")
8282
f11.write("IOError could occur")
8383
f11.close()
84-
except AttributeError:
84+
except AttributeError: # We don't consider the type of exception handled here, so this result is MISSING.
8585
f11.close()
8686

8787
def doesnt_raise(*args):
@@ -121,7 +121,7 @@ def closer2(t3):
121121

122122
def closed15():
123123
f15 = opener_func2() # $ SPURIOUS:notClosed
124-
closer2(f15)
124+
closer2(f15) # We don't detect that this call closes the file, so this result is SPURIOUS.
125125

126126

127127
def may_not_be_closed16(name):
@@ -144,7 +144,7 @@ def not_closed17():
144144
f17.write("IOError could occur")
145145
may_raise("ValueError could occur") # FN here.
146146
f17.close()
147-
except IOError:
147+
except IOError: # We don't detect that a ValueErrror could be raised that isn't handled here, so this result is MISSING.
148148
f17.close()
149149

150150
#ODASA-3779
@@ -241,7 +241,7 @@ def not_closed22(path):
241241
if foo:
242242
f22.close()
243243
finally:
244-
if f22.closed: # Wrong sense
244+
if f22.closed: # We don't precisely consider this condition, so this result is MISSING. However, this seems uncommon.
245245
f22.close()
246246

247247
def not_closed23(path):

‎python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ module MethodArgTest implements TestSig {
77

88
predicate hasActualResult(Location location, string element, string tag, string value) {
99
exists(DataFlow::CfgNode el, FileOpen fo |
10-
el = fo.getLocalSource() and
10+
el = fo and
1111
element = el.toString() and
1212
location = el.getLocation() and
1313
value = "" and

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL