JS: Add array callbacks js/use-of-returnless-function query by erik-krogh · Pull Request #2116 · github/codeql

erik-krogh · 2019-10-11T14:26:52Z

Using a callback function that does not return a value in e.g. Array.prototype.filter is generally an error.

const changePackages = packages.filter(f => {
 danger.git.modified_files.includes(f)
})
if (changePackages.length) {...}

It started out as a separate query, but after copy-pasting enough from js/use-of-returnless-function I integrated it into that query.

Just looking for methods named filter, map etc. is too imprecise and gives rise to way too many false positives. The query therefore uses type tracking to see if the receiver is an array.

It does not find many new errors, but the ones it does find are good: https://lgtm.com/query/1772403078964278178/
(The benchmarks for the above were found using the Chris API on a version of the query that did not include type tracking)

While the query on its own is slower now, it seems that performance is still good when combined with another dataflow query.
Performance evaluation is on the way.

Lots of changed lines from moving most of ExprHasNoEffect.ql into a .qll file.

asger-semmle · 2019-10-14T11:14:57Z

+  (
+    call.getReceiver().getALocalSource() = array()
+    or
+    call.getCalleeNode() instanceof LodashUnderscore::Member


I think this will miss some cases due to not having getALocalSource:

import { filter } from 'lodash' filter(xs, x => { ... } )

asger-semmle · 2019-10-14T11:20:46Z

+predicate voidArrayCallback(DataFlow::MethodCallNode call, Function func) {
+  hasNonVoidCallbackMethod(call.getMethodName()) and
+  func = call.getAnArgument().getALocalSource().asExpr() and
+  1 = count(DataFlow::Node arg | arg = call.getAnArgument() and arg.getALocalSource().asExpr() instanceof Function) and


Is this still necessary with the array() check? As far as I can tell, none of the methods we flag here could reasonably take two functions as arguments.

Yeah, you're right.
I misunderstood some instances I found where multiple functions were used.
In those there is only 1 callback function, and the second function argument is the initial value for the reduce operation.

The check should be replaced with a check that ensures that the callback is the first function argument given.

Do you still want to address this?

Yes, I somehow overlooked this when looking back at this issue.

xiemaisi · 2019-10-16T14:16:06Z

 import javascript
+import DOMProperties
+import semmle.javascript.frameworks.xUnit
+import semmle.javascript.RestrictedLocations


Do you need this in the .qll? I think it's only used by the .ql.

xiemaisi · 2019-10-16T14:18:38Z

-from DataFlow::CallNode call
-where
+predicate callToVoidFunction(DataFlow::CallNode call, Function func) {
  not call.isIndefinite(_) and 


I realise this line wasn't touched by the present PR, but what's the rationale for using isIndefinite instead of isIncomplete or isUncertain?

There was no good rationale for specifically isIndefinite. I needed something that made sure i could reasonably trust that I didn't miss anything.

I see now that isIncomplete seems like the better fit.

xiemaisi · 2019-10-16T14:21:19Z

+  (
+    callToVoidFunction(call, func) and 
+    msg = "the $@ does not return anything, yet the return value is used." and
+    name = "function " + call.getCalleeName()


getCalleeName isn't always defined, so by using it here and below you are implicitly excluding results. Is that intentional?

If not, you can use Function.describe to get a user-friendly description that generalises getCalleeName.

xiemaisi · 2019-10-16T14:23:20Z

+    name = "function " + call.getCalleeName()
+    or
+    voidArrayCallback(call, func) and 
+    msg = "the $@ does not return anything, yet the return value from the call to " + call.getCalleeName() + "() is used." and


You add () after the function name here, but not above. I'd suggest dropping it.

xiemaisi · 2019-10-16T14:24:36Z

  not benignContext(call.asExpr()) and
-
+  // Avoid double reporting from js/useless-expression
+  not hasNoEffect(func.getBodyStmt(func.getNumBodyStmt() - 1).(ExprStmt).getExpr()) and


What is this getBodyStmt business? Is it some sort of cheap AST-based approximation for a final statement? If so, would it be very difficult to replace it with a better one?

In any case, please add a test case that exercises this whitelisting.

Is it some sort of cheap AST-based approximation for a final statement?

Yes.

If so, would it be very difficult to replace it with a better one?

No. It quite close to the existing alwaysThrows predicate.
I basically duplicated the alwaysThrows predicate, but that way I'm sure about performance.

In any case, please add a test case that exercises this whitelisting.

Done.

xiemaisi · 2019-10-22T06:18:30Z

+  exists(ReachableBasicBlock entry, DataFlow::Node noEffect |
+    entry = f.getEntryBB() and
+    hasNoEffect(noEffect.asExpr()) and
+    entry.dominates(noEffect.getBasicBlock())


I don't understand this condition. Doesn't the entry node dominate every basic block?

Yes, you're right.
I was a little quick in copy-pasting from alwaysThrows without thinking about what the predicate actually did.

…sNoEffect

xiemaisi · 2019-10-22T07:53:57Z

+  exists(DataFlow::Node noEffect |
+    noEffect.getContainer() = f and
+    hasNoEffect(noEffect.asExpr()) and 
+    not exists(noEffect.getASuccessor())


Is this what you want? I don't think it matters whether noEffect has a data-flow successor, it's rather the control-flow successors we are interested in. So you probably want to look for control-flow predecessors of the exit node, or something like that. (This criterion isn't quite correct in the presence of finally blocks, but I think it's OK for whitelisting purposes.)

…ne which statement is the last

max-schaefer · 2019-10-25T12:10:50Z

+ */
+predicate voidArrayCallback(DataFlow::CallNode call, Function func) {
+  hasNonVoidCallbackMethod(call.getCalleeName()) and
+  func = call.getAnArgument().getALocalSource().asExpr() and


By using asExpr here you are excluding the case where the callback is a declared function. Perhaps use

Suggested change

func = call.getAnArgument().getALocalSource().asExpr() and

func = call.getCallback(_).getFunction() and

instead.

max-schaefer · 2019-10-25T12:11:46Z

+predicate voidArrayCallback(DataFlow::MethodCallNode call, Function func) {
+  hasNonVoidCallbackMethod(call.getMethodName()) and
+  func = call.getAnArgument().getALocalSource().asExpr() and
+  1 = count(DataFlow::Node arg | arg = call.getAnArgument() and arg.getALocalSource().asExpr() instanceof Function) and


Do you still want to address this?

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

max-schaefer · 2019-11-05T09:22:49Z

@asger-semmle, anything else you'd like to see addressed?

asger-semmle · 2019-11-05T09:33:03Z

@erik-krogh can you post a link to the evaluation? (based on an earlier commit is fine)

Otherwise LGTM

erik-krogh · 2019-11-05T10:37:31Z

@erik-krogh can you post a link to the evaluation? (based on an earlier commit is fine)

Here is an old one.
https://git.semmle.com/erik/dist-compare-reports/tree/domitian.ti.semmle.com_1570435565541
Nope, that was wrong. My evaluation seems to have gotten lost.

asger-semmle · 2019-11-05T11:09:44Z

Hm, yeah that used to happen to me a lot. Setting the branch name is crucial.

Anyways, I'm fine with landing it if you remember the evaluation as being good. @max-schaefer?

max-schaefer · 2019-11-05T11:11:41Z

Yeah, should be fine.

add array callbacks to useOfReturnlessFunction query

592cb18

erik-krogh requested a review from a team as a code owner October 11, 2019 14:26

fix test output, and add new test for array callbacks

a7c1c34

erik-krogh added the JS label Oct 11, 2019

asger-semmle reviewed Oct 14, 2019

View reviewed changes

add .getALocalSource() when testing for lodash-members

2805679

xiemaisi suggested changes Oct 16, 2019

View reviewed changes

address review feedback

2e0244c

xiemaisi suggested changes Oct 22, 2019

View reviewed changes

fix the alwaysHasNoEffect predicate, and rename it to lastStatementHa…

db22916

…sNoEffect

xiemaisi reviewed Oct 22, 2019

View reviewed changes

simplify lastStatementHasNoEffect and use the control-flow to determi…

ad3185c

…ne which statement is the last

max-schaefer reviewed Oct 25, 2019

View reviewed changes

erik-krogh mentioned this pull request Oct 25, 2019

JS: add query for detecting ignored calls to Array.prototype.concat #2203

Merged

erik-krogh added 2 commits October 25, 2019 17:46

address review feedback

841dac1

add documentation to two predicates

b2c3170

max-schaefer reviewed Oct 28, 2019

View reviewed changes

Comment thread javascript/ql/src/Statements/UseOfReturnlessFunction.ql

max-schaefer suggested changes Oct 29, 2019

View reviewed changes

Comment thread javascript/ql/src/Statements/UseOfReturnlessFunction.ql Outdated

Comment thread javascript/ql/src/Statements/UseOfReturnlessFunction.ql

add missing not

6cac961

Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>

max-schaefer approved these changes Nov 5, 2019

View reviewed changes

xiemaisi approved these changes Nov 5, 2019

View reviewed changes

semmle-qlci merged commit 794d5bd into github:master Nov 5, 2019

	func = call.getAnArgument().getALocalSource().asExpr() and
	func = call.getCallback(_).getFunction() and

Conversation

erik-krogh commented Oct 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

max-schaefer commented Nov 5, 2019

Uh oh!

asger-semmle commented Nov 5, 2019

Uh oh!

erik-krogh commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asger-semmle commented Nov 5, 2019

Uh oh!

max-schaefer commented Nov 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

erik-krogh commented Nov 5, 2019 •

edited

Loading