Skip to content

C++: Generate a temporary IRVariable for PostfixCrementOperations #13326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented May 31, 2023

Since IR dataflow doesn't look at memory operands, it's not possible for the dataflow SSA to distinguish between *p++ and *++p (since the only difference between those two expressions is which Instruction defines the memory operand of the LoadInstruction). This causes lots of FPs in the experimental cpp/invalid-pointer-deref query.

As a workaround for this problem, this PR generates a temporary IRVariable to capture the fact that a difference value is loaded from in the LoadInstruction. That is, on this snippet int x = *p++; we now generate:

r1(glval<int>)   = VariableAddress[x]         :
r2(glval<int *>) = VariableAddress[p]         :
r3(int *)        = Load[p]                    : &:r2, m1_6
r4(glval<int *>) = VariableAddress[#temp3:12] :
m1(int *)        = Store[#temp3:12]           : &:r4, r3
r5(int *)        = Load[#temp3:12]            : &:r4, m1
r6(int)          = Constant[1]                :
r7(int *)        = PointerAdd[4]              : r3, r6
m2(int *)        = Store[p]                   : &:r2, r7
r8(int)          = Load[?]                    : &:r5, ~m1_8
m3(int)          = Store[x]                   : &:r1, r8

instead of:

r1(glval<int>)   = VariableAddress[x]  :
r2(glval<int *>) = VariableAddress[p]  :
r3(int *)        = Load[p]             : &:r2, m1_6
r4(int)          = Constant[1]         :
r5(int *)        = PointerAdd[4]       : r3, r4
m1(int *)        = Store[p]            : &:r2, r5
r6(int *)        = CopyValue           : r3
r7(int)          = Load[?]             : &:r6, ~m1_8
m2(int)          = Store[x]            : &:r1, r7

(Edit: I've updated the example to reflect the changes in bfde165.)

This means dataflow SSA now correctly concludes that the increment of x doesn't flow to the address of the Load instruction.

To reduce the number of additional instructions and IRVariables generated, I've limited the change to only cases where the postfix crement isn't in a void context.

This is currently marked as a draft because I don't quite understand the changes to range-analysis in d4d4068. I took an extra look, and these changes also seem completely fine. It's just a result of more slightly more instructions. All the other changes looks good, though - especially the ones in 9d33547 🎉!

(cc @jketema)

@github-actions github-actions bot added the C++ label May 31, 2023
@MathiasVP MathiasVP force-pushed the ir-generate-temps-for-post-crements branch from d4d4068 to e9812df Compare May 31, 2023 18:49
@MathiasVP MathiasVP marked this pull request as ready for review May 31, 2023 19:15
@MathiasVP MathiasVP requested a review from a team as a code owner May 31, 2023 19:15
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label May 31, 2023
jketema
jketema previously approved these changes Jun 1, 2023
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One small comment. I haven't looked at the DCA result changes. Will do that next (I'm assuming you haven't gotten to that yet).

@jketema
Copy link
Contributor

jketema commented Jun 1, 2023

The alert losses on the nightly query suite seem to be genuine losses (I did spot checking only). All queries with losses either use default taint tracking or are rewrites that clearly derive from default taint tracking, so it's not completely clear to me to what extent we should worry about these losses.

@jketema
Copy link
Contributor

jketema commented Jun 1, 2023

The memory corruption cpp/invalid-pointer-deref result changes look ok to me. I'm not sure about the cpp/constant-array-overflow ones. The Nelson one is difficult to follow due path stitching. The kamailio looks wrong, and vim has so many results that this is very difficult to judge and I would need to see the paths to do so.

…slatedExpr.qll

Co-authored-by: Jeroen Ketema <93738568+jketema@users.noreply.github.com>
@MathiasVP
Copy link
Contributor Author

The alert losses on the nightly query suite seem to be genuine losses (I did spot checking only). All queries with losses either use default taint tracking or are rewrites that clearly derive from default taint tracking, so it's not completely clear to me to what extent we should worry about these losses.

Thanks for looking at these. I'll take a brief look at what's going on with some of these and see if I can fix it.

MathiasVP added a commit to MathiasVP/ql that referenced this pull request Jun 8, 2023
@MathiasVP
Copy link
Contributor Author

Superseded by #13406.

@MathiasVP MathiasVP closed this Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants