This is an archive of the discontinued LLVM Phabricator instance.

Fix an assertion failure that occured when custom 'operator new[]' return non-ElementRegion and 'c++-allocator-inlining' sets true.
AbandonedPublic

Authored by MTC on Aug 27 2017, 4:36 AM.

Details

Reviewers
zaks.anna
Summary

Hi All,

MallocChecker::addExtentSize() assumes that the region returned by CXXNewExpr is ElementRegion, but when the custom operator new [] is inlined, the region returned is not necessarily an ElementRegion.

Given the below code sippet:

#include <stdlib.h>
void *operator new[](std::size_t size)
{
    void *p = malloc(size);
	return p;
}
int main()
{
    int *ptr = new int[10];
}

When operator new[] is inlined, the return region is Symbolic Region, which violates the MallocChecker::addExtentSize() assumption.

Diff Detail

Event Timeline

MTC created this revision.Aug 27 2017, 4:36 AM
MTC edited the summary of this revision. (Show Details)Aug 27 2017, 4:37 AM
NoQ added a subscriber: NoQ.Aug 28 2017, 7:53 AM

I believe a custom operator new (or new[], regardless) can always return anything it wants. It can return a pointer to a concrete global variable, for example. So the only thing we can assert is that our operator is custom.

MTC added a comment.Aug 28 2017, 8:13 AM
In D37189#854062, @NoQ wrote:

I believe a custom operator new (or new[], regardless) can always return anything it wants. It can return a pointer to a concrete global variable, for example. So the only thing we can assert is that our operator is custom.

Thank you for taking the time to do the code review, and I'll update diff along with the relevant code annotations.

MTC updated this revision to Diff 112915.Aug 28 2017, 10:32 AM

Update the 'assert' condition and the code comment.

NoQ added inline comments.Dec 7 2017, 4:29 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1108

Digged a bit into this. This line should say return State;, otherwise it destroys the effort in checkPostStmt(const CXXNewExpr...) to actually track the pointer in the state.

However, testing for that would be really problematic because MallocChecker is suffering from another big problem in c++-allocator-inlining mode, namely this callback fires twice.

I'm going to describe this problem on the mailing list, because it's nasty.

I'd still wholeheartedly approve this patch with the return State change, because fixing crashes is important :)

MTC updated this revision to Diff 126262.Dec 8 2017, 8:10 PM
MTC set the repository for this revision to rC Clang.

Use 'return State' instead of 'return nullptr'.

MTC marked an inline comment as done.Dec 8 2017, 8:18 PM

Thank you for your constant attention to this problem, Artem. I've updated the diff. As you said, this is a complex problem and look forward to your work on this issue.

NoQ added a comment.Jan 17 2018, 5:58 PM

Oh well, i guess i covered this in my recent patches anyway (esp. r322787/D41406). Sorry, i just fixed everything differently and it became unclear how to integrate your patch into the whole thing.

MTC added a comment.Jan 17 2018, 6:20 PM
In D37189#979795, @NoQ wrote:

Oh well, i guess i covered this in my recent patches anyway (esp. r322787/D41406). Sorry, i just fixed everything differently and it became unclear how to integrate your patch into the whole thing.

Hi NoQ, that's all right, I'm totally fine with it :). And I don’t know how to abandon this patch, if you have the authority, please help me to abandon it, thank you!

NoQ added a comment.Jan 17 2018, 6:23 PM

That's the "Add Action..." list-box above the comment box.

MTC abandoned this revision.Jan 17 2018, 6:36 PM