Skip to content

C++: Support dynamic memory allocations in IR alias analysis#2797

Merged
jbj merged 11 commits into
github:masterfrom
rdmarsh2:rdmarsh/cpp/malloc-alias-locations
Mar 2, 2020
Merged

C++: Support dynamic memory allocations in IR alias analysis#2797
jbj merged 11 commits into
github:masterfrom
rdmarsh2:rdmarsh/cpp/malloc-alias-locations

Conversation

@rdmarsh2
Copy link
Copy Markdown
Contributor

@rdmarsh2 rdmarsh2 commented Feb 8, 2020

This adds a new opcode, InitializeDynamicAllocation, adds it to IR generation for statically-resolvable calls to AllocationFunctions, and adds a new Allocation subtype in the aliased_ssa version of AliasConfiguration.qll.

@rdmarsh2 rdmarsh2 added the C++ label Feb 8, 2020
@rdmarsh2 rdmarsh2 requested review from dbartol and jbj February 8, 2020 00:45
Copy link
Copy Markdown
Contributor Author

@rdmarsh2 rdmarsh2 left a comment

Choose a reason for hiding this comment

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

I have a few TODOs added early that didn't seem to cause any issues, but I'd like a second look at them before I take them out

Comment thread cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll Outdated
Copy link
Copy Markdown

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I'll do a final pass once you've taken it out of Draft status and the tests are passing.

Comment thread cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll Outdated
@rdmarsh2 rdmarsh2 marked this pull request as ready for review February 11, 2020 21:12
@rdmarsh2 rdmarsh2 requested review from a team as code owners February 11, 2020 21:12
@rdmarsh2 rdmarsh2 requested a review from dbartol February 11, 2020 21:12
Comment thread cpp/ql/src/semmle/code/cpp/security/TaintTracking.qll
@rdmarsh2 rdmarsh2 requested a review from geoffw0 February 12, 2020 23:23
dbartol
dbartol previously approved these changes Feb 13, 2020
Copy link
Copy Markdown

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

LGTM modulo typo

dbartol
dbartol previously approved these changes Feb 13, 2020
Copy link
Copy Markdown

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

@rdmarsh2 We need perf numbers for this before we merge, but other than that it looks ready to go. I don't expect any negative perf impact.

@dbartol dbartol dismissed their stale review February 19, 2020 22:52

Waiting for perf numbers

@rdmarsh2 rdmarsh2 added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Feb 26, 2020
@rdmarsh2
Copy link
Copy Markdown
Contributor Author

https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/869/ finished - the analysis with this change was about 5% faster on Wireshark and ChakraCore. That might be noise or it might be from having fewer references to all aliased memory in SSA construction.

Copy link
Copy Markdown
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

The internal tests have passed, so I'll merge this now.

@jbj jbj merged commit ec85f9f into github:master Mar 2, 2020
@rdmarsh2 rdmarsh2 deleted the rdmarsh/cpp/malloc-alias-locations branch March 2, 2020 18:05
@jbj jbj mentioned this pull request Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants