Merge pull request github#2439 from erik-krogh/useOfReturnlessFunctionHotfix

semmle-qlci · web-flow · commit 3d5d178b196f · 2019-11-26T11:56:57.000Z
Approved by max-schaefer
diff --git a/javascript/ql/src/Statements/UseOfReturnlessFunction.ql b/javascript/ql/src/Statements/UseOfReturnlessFunction.ql
@@ -33,7 +33,9 @@ predicate benignContext(Expr e) {
   inVoidContext(e) or 
   
   // A return statement is often used to just end the function.
-  e = any(Function f).getAReturnedExpr()
+  e = any(Function f).getBody()
+  or
+  e = any(ReturnStmt r).getExpr()
   or 
   exists(ConditionalExpr cond | cond.getABranch() = e and benignContext(cond)) 
   or 
@@ -42,7 +44,6 @@ predicate benignContext(Expr e) {
   exists(Expr parent | parent.getUnderlyingValue() = e and benignContext(parent))
   or 
   any(VoidExpr voidExpr).getOperand() = e
-
   or
   // weeds out calls inside HTML-attributes.
   e.getParent().(ExprStmt).getParent() instanceof CodeInAttribute or
@@ -70,8 +71,8 @@ predicate benignContext(Expr e) {
   e = any(ResolvedPromiseDefinition promise).getValue().asExpr()
 }
 
-predicate oneshotClosure(InvokeExpr call) {
-  call.getCallee().getUnderlyingValue() instanceof ImmediatelyInvokedFunctionExpr
+predicate oneshotClosure(DataFlow::CallNode call) {
+  call.getCalleeNode().asExpr().getUnderlyingValue() instanceof ImmediatelyInvokedFunctionExpr
 }
 
 predicate alwaysThrows(Function f) {
@@ -149,6 +150,12 @@ predicate voidArrayCallback(DataFlow::CallNode call, Function func) {
   )
 }
 
+predicate hasNonVoidReturnType(Function f) {
+  exists(TypeAnnotation type | type = f.getReturnTypeAnnotation() | 
+    not type.isVoid()
+  )
+}
+
 
 /**
  * Provides classes for working with various Deferred implementations. 
@@ -214,6 +221,8 @@ where
   not benignContext(call.getEnclosingExpr()) and
   not lastStatementHasNoEffect(func) and
   // anonymous one-shot closure. Those are used in weird ways and we ignore them.
-  not oneshotClosure(call.getEnclosingExpr())
+  not oneshotClosure(call) and
+  not hasNonVoidReturnType(func) and
+  not call.getEnclosingExpr() instanceof SuperCall
 select
   call, msg, func, name
diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.expected
@@ -6,3 +6,4 @@
 | tst.js:53:10:53:34 | bothOnl ... fects() | the $@ does not return anything, yet the return value is used. | tst.js:48:2:50:5 | functio ... )\\n    } | function onlySideEffects2 |
 | tst.js:76:12:76:46 | [1,2,3] ... n, 3)}) | the $@ does not return anything, yet the return value from the call to filter is used. | tst.js:76:27:76:45 | n => {equals(n, 3)} | callback function |
 | tst.js:80:12:80:50 | filter( ...  3) } ) | the $@ does not return anything, yet the return value from the call to filter is used. | tst.js:80:28:80:48 | x => {  ... x, 3) } | callback function |
+| tst.ts:6:13:6:25 | returnsVoid() | the $@ does not return anything, yet the return value is used. | tst.ts:1:1:1:38 | declare ... : void; | function returnsVoid |
diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tsconfig.json b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tsconfig.json
@@ -0,0 +1 @@
+{}
diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.html b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.html
@@ -12,7 +12,7 @@
       }
     </script>
   </head>
-  <body>
+  <body onload="return addHandlers();">
     <object id="container" data="editor/svg-editor.html" onload="addHandlers()"></object>
     <a href="javascript:addHandlers()">Foo</a>
     <div onclick="addHandlers()">Click me</div>
diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js
@@ -88,4 +88,20 @@
 	}
 	
 	new Deferred().resolve(onlySideEffects()); // OK
-})();
+})();
+
++function() {
+    console.log("FOO");
+}.call(this);
+
+class Foo {
+	constructor() {
+		console.log("FOO");
+	}
+}
+
+class Bar extends Foo {
+	constructor() {
+		console.log(super()); // OK.
+	}
+}
diff --git a/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.ts b/javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.ts
@@ -0,0 +1,6 @@
+declare function returnsVoid() : void;
+declare function returnsSomething(): number;
+
+console.log(returnsSomething());
+
+console.log(returnsVoid()); // NOT OK!

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@`
`12`	`12`	`}`
`13`	`13`	`</script>`
`14`	`14`	`</head>`
`15`		`- <body>`
	`15`	`+ <body onload="return addHandlers();">`
`16`	`16`	`<object id="container" data="editor/svg-editor.html" onload="addHandlers()"></object>`
`17`	`17`	`<a href="javascript:addHandlers()">Foo</a>`
`18`	`18`	`<div onclick="addHandlers()">Click me</div>`