Skip to content

C++: Support operator new and operator delete in models library #3173

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

Merged
merged 15 commits into from
Apr 2, 2020

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Mar 31, 2020

AllocationExpr directly supports new, but for various reasons we also want to model operator new as an AllocationFunction. Same for operator new[], operator delete and operator delete[].

@geoffw0 geoffw0 added the C++ label Mar 31, 2020
@geoffw0 geoffw0 requested a review from a team as a code owner March 31, 2020 13:38

override int getSizeArg() { result = 0 }

override predicate requiresDealloc() { not exists(getPlacementArgument()) }
Copy link
Contributor

Choose a reason for hiding this comment

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

For the overloads where this predicate holds (9 and 10 in https://en.cppreference.com/w/cpp/memory/new/operator_new), I don't think they should be AllocationFunctions. They are merely identity functions. The IR alias analysis assumes that an allocation function returns a fresh memory location that doesn't alias anything else reachable, and that's also my own understanding of what an allocation function is.

The requiresDealloc predicate is currently overridden by alloca, and that makes sense to me: it returns fresh memory but doesn't require deallocation. In the AllocationExpr hierarchy, requiresDealloc is overridden by NewAllocationExpr and NewArrayAllocationExpr, and I think the placement versions of those expressions should not be AllocationExprs for the same reasons. Unless the allocation classes are used in other queries where we do want to include the placement versions.

Functions that always return their parameter can instead be modelled as AliasFunction.

There may be some C++ standard rules about how it's forbidden to reference an old pointer value that's passed through placement new, which effectively means it behaves like it's allocating fresh memory, but I don't think programmers obey that rule. I also couldn't find the rule on https://en.cppreference.com/w/cpp/language/new, so I may be misremembering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The IR alias analysis assumes that an allocation function returns a fresh memory location that doesn't alias anything else reachable

That's a slightly dubious assumption when you're looking inside the implementation of an operator new, which presumably needs to keep hold of some data about the allocation for bookkeeping purposes (I'm not sure exactly how modern non-toy implementations do this). But that data should be well compartmentalized from user code, so I doubt this is something we need to model.

Functions that always return their parameter can instead be modelled as AliasFunction.

Good idea. On balance, I think your distinction is clearer than mine (which is roughly 'Does this thing claim to be an allocator? Then it is one.').

However due to the existing behaviour in AllocationExpr (which is fairly widely used), I'd rather address this concern in a follow-up PR and give it the care and attention it deserves.

There may be some C++ standard rules about how it's forbidden to reference an old pointer value that's passed through placement new

It seems like you should be able to keep a reference for memory management purposes, e.g.

char *data = new char[BIG_NUMBER];

MyObject *myObject1 = new (data) MyObject();
MyObject *myObject2 = new (data + sizeof(MyObject)) MyObject();
...

delete [] data;

Copy link
Contributor

Choose a reason for hiding this comment

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

However due to the existing behaviour in AllocationExpr (which is fairly widely used), I'd rather address this concern in a follow-up PR and give it the care and attention it deserves.

Okay.

It seems like you should be able to keep a reference for memory management purposes

That sounds right.

@MathiasVP
Copy link
Contributor

MathiasVP commented Mar 31, 2020

As far as I can tell, this doesn't seem to introduce any semantic conflicts with #3171. #3171 assumes that malloc expressions are instances of CallAllocationExpr, and that new and new[] are instances of NewAllocationExpr, and NewArrayAllocationExpr respectively. These facts still seem to be the case in this PR.

This PR also merges cleanly with mine.

@jbj
Copy link
Contributor

jbj commented Apr 1, 2020

There are test failures in the IR sanity queries. I took a quick look, and my guess is that they appear because we don't translate allocator calls but now we translate a side effect of those calls.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 1, 2020

Fixes:

  • fixed the unintended consequence upon the IR's TranslatedCall.qll of making operator new and operator new[] an AllocationFunction.
  • made a PR to accept the changes to the failing internal test: https://git.semmle.com/Semmle/code/pull/36598

The expected result is one test failure remains on this PR (the MISRA test), and none on the corresponding PR (when that goes green, both can be merged at the same time).

@jbj jbj added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Apr 1, 2020
@jbj jbj merged commit 4825774 into github:master Apr 2, 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.

3 participants