Skip to content

Commit 5bb29b6

Browse files
NapalysNapalys
committed
Now flags only .pipe calls which have an error somewhere down the stream, but not on the source stream.
1 parent 5214cc0 commit 5bb29b6

File tree

4 files changed

+183
-27
lines changed

4 files changed

+183
-27
lines changed

‎javascript/ql/src/Quality/UnhandledStreamPipe.ql

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,11 @@ string getStreamMethodName() {
110110
* A call to register an event handler on a Node.js stream.
111111
* This includes methods like `on`, `once`, and `addListener`.
112112
*/
113-
class StreamEventRegistration extends DataFlow::MethodCallNode {
114-
StreamEventRegistration() { this.getMethodName() = getEventHandlerMethodName() }
113+
class ErrorHandlerRegistration extends DataFlow::MethodCallNode {
114+
ErrorHandlerRegistration() {
115+
this.getMethodName() = getEventHandlerMethodName() and
116+
this.getArgument(0).getStringValue() = "error"
117+
}
115118
}
116119

117120
/**
@@ -136,7 +139,12 @@ predicate streamFlowStep(DataFlow::Node streamNode, DataFlow::Node relatedNode)
136139
* Tracks the result of a pipe call as it flows through the program.
137140
*/
138141
private DataFlow::SourceNode pipeResultTracker(DataFlow::TypeTracker t, PipeCall pipe) {
139-
t.start() and result = pipe
142+
t.start() and result = pipe.getALocalSource()
143+
or
144+
exists(DataFlow::SourceNode prev |
145+
prev = pipeResultTracker(t.continue(), pipe) and
146+
streamFlowStep(result.getALocalUse(), prev)
147+
)
140148
or
141149
exists(DataFlow::TypeTracker t2 | result = pipeResultTracker(t2, pipe).track(t2, t))
142150
}
@@ -166,7 +174,7 @@ predicate isPipeFollowedByNonStreamMethod(PipeCall pipeCall) {
166174
predicate isPipeFollowedByNonStreamProperty(PipeCall pipeCall) {
167175
exists(DataFlow::PropRef propRef |
168176
propRef = pipeResultRef(pipeCall).getAPropertyRead() and
169-
not propRef.getPropertyName() = getStreamPropertyName()
177+
not propRef.getPropertyName() = [getStreamPropertyName(), getStreamMethodName()]
170178
)
171179
}
172180

@@ -206,9 +214,8 @@ private DataFlow::SourceNode streamRef(PipeCall pipeCall) {
206214
* Holds if the source stream of the given pipe call has an `error` handler registered.
207215
*/
208216
predicate hasErrorHandlerRegistered(PipeCall pipeCall) {
209-
exists(StreamEventRegistration handler |
210-
handler = streamRef(pipeCall).getAMethodCall(getEventHandlerMethodName()) and
211-
handler.getArgument(0).getStringValue() = "error"
217+
exists(ErrorHandlerRegistration handler |
218+
handler = streamRef(pipeCall).getAMethodCall(getEventHandlerMethodName())
212219
)
213220
or
214221
hasPlumber(pipeCall)
@@ -218,6 +225,8 @@ predicate hasErrorHandlerRegistered(PipeCall pipeCall) {
218225
* Holds if one of the arguments of the pipe call is a `gulp-plumber` monkey patch.
219226
*/
220227
predicate hasPlumber(PipeCall pipeCall) {
228+
pipeCall.getDestinationStream().getALocalSource() = API::moduleImport("gulp-plumber").getACall()
229+
or
221230
streamRef+(pipeCall) = API::moduleImport("gulp-plumber").getACall()
222231
}
223232

@@ -246,9 +255,19 @@ private predicate hasNonStreamSourceLikeUsage(PipeCall pipeCall) {
246255
)
247256
}
248257

258+
/**
259+
* Holds if the pipe call destination stream has an error handler registered.
260+
*/
261+
predicate hasErrorHandlerDownstream(PipeCall pipeCall) {
262+
exists(ErrorHandlerRegistration handler |
263+
handler.getReceiver().getALocalSource() = pipeResultRef(pipeCall)
264+
)
265+
}
266+
249267
from PipeCall pipeCall
250268
where
251269
not hasErrorHandlerRegistered(pipeCall) and
270+
hasErrorHandlerDownstream(pipeCall) and
252271
not isPipeFollowedByNonStreamAccess(pipeCall) and
253272
not hasNonStreamSourceLikeUsage(pipeCall) and
254273
not hasNonNodeJsStreamSource(pipeCall)

‎javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.expected

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,16 @@
55
| test.js:66:5:66:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
66
| test.js:79:5:79:25 | s2.pipe ... ation2) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
77
| test.js:94:5:94:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
8-
| test.js:109:26:109:37 | s.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
9-
| test.js:116:5:116:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
10-
| test.js:125:5:125:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
11-
| test.js:143:5:143:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
12-
| test.js:175:17:175:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
13-
| test.js:185:5:185:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
8+
| test.js:110:11:110:22 | s.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
9+
| test.js:119:5:119:21 | stream.pipe(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
10+
| test.js:128:5:128:26 | getStre ... e(dest) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
11+
| test.js:146:5:146:62 | stream. ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
12+
| test.js:182:17:182:40 | notStre ... itable) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
13+
| test.js:192:5:192:32 | copyStr ... nation) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
14+
| tst.js:8:5:8:21 | source.pipe(gzip) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
15+
| tst.js:29:5:29:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
16+
| tst.js:37:21:37:56 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
17+
| tst.js:44:5:44:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
18+
| tst.js:59:18:59:39 | stream. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
19+
| tst.js:73:5:73:40 | wrapper ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |
20+
| tst.js:111:5:111:26 | stream. ... Stream) | Stream pipe without error handling on the source stream. Errors won't propagate downstream and may be silently dropped. |

‎javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/test.js

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
function test() {
22
{
33
const stream = getStream();
4-
stream.pipe(destination); // $Alert
4+
stream.pipe(destination).on("error", e); // $Alert
55
}
66
{
77
const stream = getStream();
@@ -16,7 +16,7 @@ function test() {
1616
{
1717
const stream = getStream();
1818
const s2 = stream;
19-
s2.pipe(dest); // $Alert
19+
s2.pipe(dest).on("error", e); // $Alert
2020
}
2121
{
2222
const stream = getStream();
@@ -42,7 +42,7 @@ function test() {
4242
const stream = getStream();
4343
stream.on('error', handleError);
4444
const stream2 = stream.pipe(destination);
45-
stream2.pipe(destination2); // $Alert
45+
stream2.pipe(destination2).on("error", e); // $Alert
4646
}
4747
{
4848
const stream = getStream();
@@ -57,13 +57,13 @@ function test() {
5757
const stream = getStream();
5858
stream.on('error', handleError);
5959
const stream2 = stream.pipe(destination);
60-
stream2.pipe(destination2); // $Alert
60+
stream2.pipe(destination2).on("error", e); // $Alert
6161
}
6262
{ // Error handler on destination instead of source
6363
const stream = getStream();
6464
const dest = getDest();
6565
dest.on('error', handler);
66-
stream.pipe(dest); // $Alert
66+
stream.pipe(dest).on("error", e); // $Alert
6767
}
6868
{ // Multiple aliases, error handler on one
6969
const stream = getStream();
@@ -76,7 +76,7 @@ function test() {
7676
const stream = getStream();
7777
const s2 = stream.pipe(destination1);
7878
stream.on('error', handleError);
79-
s2.pipe(destination2); // $Alert
79+
s2.pipe(destination2).on("error", e); // $Alert
8080
}
8181
{ // Handler registered via .once
8282
const stream = getStream();
@@ -91,7 +91,7 @@ function test() {
9191
{ // Handler registered for unrelated event
9292
const stream = getStream();
9393
stream.on('close', handleClose);
94-
stream.pipe(dest); // $Alert
94+
stream.pipe(dest).on("error", e); // $Alert
9595
}
9696
{ // Error handler registered after pipe, but before error
9797
const stream = getStream();
@@ -106,14 +106,17 @@ function test() {
106106
}
107107
{ // Pipe in a function, error handler not set
108108
const stream = getStream();
109-
function doPipe(s) { s.pipe(dest); } // $Alert
109+
function doPipe(s) {
110+
f = s.pipe(dest); // $Alert
111+
f.on("error", e);
112+
}
110113
doPipe(stream);
111114
}
112115
{ // Dynamic event assignment
113116
const stream = getStream();
114117
const event = 'error';
115118
stream.on(event, handleError);
116-
stream.pipe(dest); // $SPURIOUS:Alert
119+
stream.pipe(dest).on("error", e); // $SPURIOUS:Alert
117120
}
118121
{ // Handler assigned via variable property
119122
const stream = getStream();
@@ -122,12 +125,12 @@ function test() {
122125
stream.pipe(dest);
123126
}
124127
{ // Pipe with no intermediate variable, no error handler
125-
getStream().pipe(dest); // $Alert
128+
getStream().pipe(dest).on("error", e); // $Alert
126129
}
127130
{ // Handler set via .addListener synonym
128131
const stream = getStream();
129132
stream.addListener('error', handleError);
130-
stream.pipe(dest);
133+
stream.pipe(dest).on("error", e);
131134
}
132135
{ // Handler set via .once after .pipe
133136
const stream = getStream();
@@ -140,7 +143,11 @@ function test() {
140143
}
141144
{ // Long chained pipe without error handler
142145
const stream = getStream();
143-
stream.pause().setEncoding('utf8').resume().pipe(writable); // $Alert
146+
stream.pause().setEncoding('utf8').resume().pipe(writable).on("error", e); // $Alert
147+
}
148+
{ // Long chained pipe without error handler
149+
const stream = getStream();
150+
stream.pause().setEncoding('utf8').on("error", e).resume().pipe(writable).on("error", e);
144151
}
145152
{ // Non-stream with pipe method that returns subscribable object (Streams do not have subscribe method)
146153
const notStream = getNotAStream();
@@ -172,7 +179,7 @@ function test() {
172179
}
173180
{ // Member access on a stream after pipe
174181
const notStream = getNotAStream();
175-
const val = notStream.pipe(writable).readable; // $Alert
182+
const val = notStream.pipe(writable).on("error", e).readable; // $Alert
176183
}
177184
{ // Method access on a non-stream after pipe
178185
const notStream = getNotAStream();
@@ -182,7 +189,7 @@ function test() {
182189
const fs = require('fs');
183190
const stream = fs.createReadStream('file.txt');
184191
const copyStream = stream;
185-
copyStream.pipe(destination); // $Alert
192+
copyStream.pipe(destination).on("error", e); // $Alert
186193
}
187194
{
188195
const notStream = getNotAStream();
@@ -211,6 +218,16 @@ function test() {
211218
const p = plumber();
212219
getStream().pipe(p).pipe(dest).pipe(dest).pipe(dest);
213220
}
221+
{
222+
const plumber = require('gulp-plumber');
223+
const p = plumber();
224+
getStream().pipe(p);
225+
}
226+
{
227+
const plumber = require('gulp-plumber');
228+
const p = plumber();
229+
getStream().pipe(p).pipe(dest);
230+
}
214231
{
215232
const notStream = getNotAStream();
216233
notStream.pipe(getStream(),()=>{});

‎javascript/ql/test/query-tests/Quality/UnhandledStreamPipe/tst.js

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
const fs = require('fs');
2+
const zlib = require('zlib');
3+
4+
function foo(){
5+
const source = fs.createReadStream('input.txt');
6+
const gzip = zlib.createGzip();
7+
const destination = fs.createWriteStream('output.txt.gz');
8+
source.pipe(gzip).pipe(destination); // $Alert
9+
gzip.on('error', e);
10+
}
11+
class StreamWrapper {
12+
constructor() {
13+
this.outputStream = getStream();
14+
}
15+
}
16+
17+
function zip() {
18+
const zipStream = createWriteStream(zipPath);
19+
let wrapper = new StreamWrapper();
20+
let stream = wrapper.outputStream;
21+
stream.on('error', e);
22+
stream.pipe(zipStream);
23+
zipStream.on('error', e);
24+
}
25+
26+
function zip1() {
27+
const zipStream = createWriteStream(zipPath);
28+
let wrapper = new StreamWrapper();
29+
wrapper.outputStream.pipe(zipStream); // $SPURIOUS:Alert
30+
wrapper.outputStream.on('error', e);
31+
zipStream.on('error', e);
32+
}
33+
34+
function zip2() {
35+
const zipStream = createWriteStream(zipPath);
36+
let wrapper = new StreamWrapper();
37+
let outStream = wrapper.outputStream.pipe(zipStream); // $Alert
38+
outStream.on('error', e);
39+
}
40+
41+
function zip3() {
42+
const zipStream = createWriteStream(zipPath);
43+
let wrapper = new StreamWrapper();
44+
wrapper.outputStream.pipe(zipStream); // $Alert
45+
zipStream.on('error', e);
46+
}
47+
48+
function zip3() {
49+
const zipStream = createWriteStream(zipPath);
50+
let wrapper = new StreamWrapper();
51+
let source = getStream();
52+
source.pipe(wrapper.outputStream); // $MISSING:Alert
53+
wrapper.outputStream.on('error', e);
54+
}
55+
56+
function zip4() {
57+
const zipStream = createWriteStream(zipPath);
58+
let stream = getStream();
59+
let output = stream.pipe(zipStream); // $Alert
60+
output.on('error', e);
61+
}
62+
63+
class StreamWrapper2 {
64+
constructor() {
65+
this.outputStream = getStream();
66+
this.outputStream.on('error', e);
67+
}
68+
69+
}
70+
function zip5() {
71+
const zipStream = createWriteStream(zipPath);
72+
let wrapper = new StreamWrapper2();
73+
wrapper.outputStream.pipe(zipStream); // $SPURIOUS:Alert
74+
zipStream.on('error', e);
75+
}
76+
77+
class StreamWrapper3 {
78+
constructor() {
79+
this.stream = getStream();
80+
}
81+
pipeIt(dest) {
82+
return this.stream.pipe(dest);
83+
}
84+
register_error_handler(listener) {
85+
return this.stream.on('error', listener);
86+
}
87+
}
88+
89+
function zip5() {
90+
const zipStream = createWriteStream(zipPath);
91+
let wrapper = new StreamWrapper3();
92+
wrapper.pipeIt(zipStream); // $MISSING:Alert
93+
zipStream.on('error', e);
94+
}
95+
function zip6() {
96+
const zipStream = createWriteStream(zipPath);
97+
let wrapper = new StreamWrapper3();
98+
wrapper.pipeIt(zipStream);
99+
wrapper.register_error_handler(e);
100+
zipStream.on('error', e);
101+
}
102+
103+
function registerErr(stream, listerner) {
104+
stream.on('error', listerner);
105+
}
106+
107+
function zip7() {
108+
const zipStream = createWriteStream(zipPath);
109+
let stream = getStream();
110+
registerErr(stream, e);
111+
stream.pipe(zipStream); // $SPURIOUS:Alert
112+
zipStream.on('error', e);
113+
}

0 commit comments

Comments
 (0)