This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Fix Bug34144-[MallocChecker] MallocChecker::MallocUpdateRefState(): Assertion `Sym' failed.
AbandonedPublic

Authored by MTC on Aug 14 2017, 12:24 PM.

Details

Summary

Dear All,
I would like to propose a patch to fix the Bug34144[https://bugs.llvm.org/show_bug.cgi?id=34144].

Bug34144 mainly because MallocChecker assumes that the pointer returned by CXXNewExpr can't be null.
However, in some cases, c++ allocator may return a Null pointer, and when 'c++-allocator-inlining'
config option sets true, the CXXNewExpr may bind a Null Pointer. This will trigger an assertion failure
in MallocChecker::MallocUpdateRefState().
Given the below code sippet:

#include <new>
void *operator new(std::size_t, const std::nothrow_t&) noexcept
{
  return 0;
}
int main()
{
  int *i = new(std::nothrow) int(1);
  delete i;
  return 0;
}

Please let me know if this is an acceptable change.

Regards,
Henry Wong

Diff Detail

Event Timeline

MTC created this revision.Aug 14 2017, 12:24 PM
NoQ edited edge metadata.Aug 15 2017, 2:38 AM

Totally makes sense, thanks!

I think the better place for the test would be NewDelete-custom.cpp, not sure it'd be easy to stick another operator new in there, could you have a look?

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1282

I think that within this branch we can/should assert that E is indeed a custom operator new call.

MTC added a comment.EditedAug 16 2017, 9:19 AM
In D36708#841768, @NoQ wrote:

Totally makes sense, thanks!

I think the better place for the test would be NewDelete-custom.cpp, not sure it'd be easy to stick another operator new in there, could you have a look?

Hi NoQ,
Thank you for your reply, NewDelete-custom.cpp is really a better a choice. But I have encounter another assertion failure about inlining operater new[].

Code sippet from NewDelete-custom.cpp:

// clang --analyze -Xanalyzer -analyzer-checker=unix.Malloc -Xanalyzer -analyzer-config -Xanalyzer c++-allocator-inlining=true NewDelete-custom.cpp
void *allocator(size_t);
void *operator new[](size_t size) { return allocator(size); }
void testNewExprArray() {
  int *p = new int[0];
}

At the end of the operator new[] inline, analyzer will bind a conjured symbol to operator new[], that is, operator new[] will return a symbolic region.
When CXXNewExpr is an array version, MallocChecker::addExtentSize() requires an ElementRegion, so this will trigger an assertion failure. So I think
c ++ - allocator-inlining still has some problems to solve, and I'll to give a new patch.

NoQ added a comment.Aug 17 2017, 8:37 AM

Aha, right, makes sense. Yeah, there must be more problems with this mode. Which doesn't make the current patch less useful :)

I'd approve this if the assert is partially brought back, as i mentioned in the inline comment. If you like, you can make a new test file - there'd anyway be many test files for many possible definitions of the single global operator new().

MTC updated this revision to Diff 112197.EditedAug 22 2017, 10:34 AM

a. Fix Bug34144-[MallocChecker] MallocChecker::MallocUpdateRefState(): Assertion 'Sym' failed.
b. Like Bug34144, inline the custom operator new[] will trigger a assertion failure of MallocChecker::addExtentsize. This is just a temporary solution, the final solution still requires the analyzer can fully simulate the CXXNewAllocator.
c. When the custom operator new/new[] is inlined, MallocChecker:: checkPostStmt (const, CallExpr, *CE,...) will be returned directly, leading to false negative about memory leaks. Therefore, I do special handling of operator new/new[].

MTC added a comment.Aug 22 2017, 10:37 AM
In D36708#844449, @NoQ wrote:

Aha, right, makes sense. Yeah, there must be more problems with this mode. Which doesn't make the current patch less useful :)

I'd approve this if the assert is partially brought back, as i mentioned in the inline comment. If you like, you can make a new test file - there'd anyway be many test files for many possible definitions of the single global operator new().

Hi, NoQ.
Appreciate for your reply! I have updated the diff and look forward to your comments.

MTC updated this revision to Diff 112202.Aug 22 2017, 11:05 AM

Correct a careless mistake by adding 'return nullptr' to MallocChecker :: addExtentSize().

MTC updated this revision to Diff 112688.EditedAug 25 2017, 6:27 AM
MTC marked an inline comment as done.

Update comments.

Hi MTC,

Could you please split out the fix (b) into it's separate patch? We would like to have the patches be as incremental as possible.

Thanks for working on this!

MTC added a comment.Aug 27 2017, 2:32 AM
This comment was removed by MTC.
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1282

Hi NoQ, thanks for your patience. I've updated the code!

MTC updated this revision to Diff 112818.Aug 27 2017, 5:07 AM

Because the way I submit the diff is not reasonable, the same diff corresponds to multiple purposes. This diff is primarily to address Bug34144.

MTC added a comment.EditedAug 27 2017, 5:20 AM

Hi MTC,

Could you please split out the fix (b) into it's separate patch? We would like to have the patches be as incremental as possible.

Thanks for working on this!

Hi Anna,
Thanks for your correction. I put (b)'s patch on the D37189, another point to note that I did not add the test for the D36708. NewDelete-custom.cpp is a suitable place for the test case, but when I add 'c++-allocator-inlining=true' option to the lit command, an assertion failure will occur(see D37189). What's your suggestion, please?

NoQ added a comment.Aug 28 2017, 7:48 AM

We can probably land D37189 first, and then land this patch with a test?

NoQ added inline comments.Aug 28 2017, 7:53 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1285–1286

A null symbol doesn't necessarily mean a null pointer. A non-null concrete pointer or a pointer to a specific variable would also be a non-symbol.

MTC added a comment.Aug 28 2017, 8:02 AM
In D36708#854060, @NoQ wrote:

We can probably land D37189 first, and then land this patch with a test?

Thanks a lot, it's a practical way.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1285–1286

Thanks for your reply, and I will correct the error message of this assert.

MTC abandoned this revision.Jan 17 2018, 9:29 PM