Skip to content

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jul 7, 2020

This is a follow-up to #3733, requested by @jbj in https://github.com/github/codeql-c-analysis-team/issues/90, adding taint tests for swap that work like they did originally (i.e. where taint flow has to be deduced from the function bodies and can't just from their constructor/assignment operator signatures).

@MathiasVP my changes seem to have had a non-local effect on the result for swap2.cpp line 87 (look at the second commit on its own to see this). I'd like to know your thoughts on this result and whether it is well understood?

@geoffw0 geoffw0 added the C++ label Jul 7, 2020
@geoffw0 geoffw0 requested a review from a team as a code owner July 7, 2020 10:43
@MathiasVP
Copy link
Contributor

@MathiasVP my changes seem to have had a non-local effect on the result for swap2.cpp line 87 (look at the second commit on its own to see this). I'd like to know your thoughts on this result and whether it is well understood?

I'm not quite sure why we lose that result all of a sudden. I'll investigate it later today.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jul 7, 2020

Thanks.

I believe it comes back if you comment out either the call y.move_assign(std::move(x)), or swap(that) inside move_assign. And it only disappeared in the swap2 test despite the changes being the same.

@MathiasVP MathiasVP assigned jbj and unassigned MathiasVP Jul 8, 2020
@jbj
Copy link
Contributor

jbj commented Jul 9, 2020

I'll merge this now and investigate the mysterious non-local effect later.

@jbj jbj merged commit 2fa5455 into github:master Jul 9, 2020
@jbj
Copy link
Contributor

jbj commented Nov 26, 2020

I attempted to investigate the mysterious non-local effect in this test last week, but I wasn't able to isolate it in a few hours. I can't justify spending more time on it, given that we've never seen other instances of this bug. It only affects AST data flow.

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.

3 participants