Skip to content

CPP: Tests for taint through lambdas#1742

Merged
jbj merged 3 commits into
github:masterfrom
geoffw0:lambdataint
Aug 16, 2019
Merged

CPP: Tests for taint through lambdas#1742
jbj merged 3 commits into
github:masterfrom
geoffw0:lambdataint

Conversation

@geoffw0

@geoffw0 geoffw0 commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

Tests for taint through lambdas. For https://jira.semmle.com/browse/CPP-410.

@jbj thinks that taint flow through lambdas may come for free with flow through fields. If this occurs most of these test cases should then begin to get correct results. Otherwise, it shouldn't be very difficult to add explicit lambda support.

@geoffw0 geoffw0 added the C++ label Aug 15, 2019
@geoffw0 geoffw0 requested a review from a team as a code owner August 15, 2019 09:56
@jbj

jbj commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

Note to self: this will conflict with #1715.

@jbj

jbj commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

LGTM. The test output will need to be updated after #1715 is merged.

I just looked through Lambda.qll to confirm that we extract lambdas to a completely desugared form where the captures become fields of a compiler-generated struct. I think most of the tests in this file can be covered by flow through fields.

@geoffw0

geoffw0 commented Aug 15, 2019

Copy link
Copy Markdown
Contributor Author

I've just updated this with the effects of #1715. There's one good result, the rest are probably correct but point to things with no locations. This might be OK for many purposes but I think we need to fix it.

@jbj

jbj commented Aug 15, 2019

Copy link
Copy Markdown
Contributor

Nice!

jbj
jbj previously approved these changes Aug 15, 2019
@geoffw0

geoffw0 commented Aug 15, 2019

Copy link
Copy Markdown
Contributor Author

Additional issue: it appears that both captured variables (in this case t and u) are considered tainted even when only one (t) is ever assigned tainted data. I'm guessing in the generated code they're held in the same struct, which perhaps has something to do with the issue?

@geoffw0

geoffw0 commented Aug 15, 2019

Copy link
Copy Markdown
Contributor Author

Note: I said on Slack: "It looks like an Access to a LambdaCapture.getField() doesn't always have a getLocation()."

@geoffw0

geoffw0 commented Aug 15, 2019

Copy link
Copy Markdown
Contributor Author

Added the same test code to a dataflow test. The only cases that work in that are the two with flow through ordinary parameters to the lambdas, which worked before #1715.

I'm not sure exactly what's going on, best guess data flow through fields is not providing everything we need after all, and taint is doing something a bit too broad (in conjunction with the fields changes) which is where we're getting our results on the taint test.

@jbj jbj merged commit f3f89ff into github:master Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants