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

[MTC]

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.

[NoQ]

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]

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]

Update the 'assert' condition and the code comment.

[NoQ]

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]

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

[MTC]

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]

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]

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]

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

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

                  ->getAs<SubRegion>()
                  ->getSuperRegion()
                  ->getAs<SubRegion>();
+    // Custom 'operator new[]' does not always return ElementRegion.
+    if (!Region) {
+      assert(!C.getSourceManager().isInSystemHeader(
+                 NE->getOperatorNew()->getLocStart()) &&
+             "Only custom 'operator new[]' can return non-ElementRegion.");
+      return nullptr;
+    }
   } else {
     ElementCount = svalBuilder.makeIntVal(1, true);
     Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>();

test/Analysis/NewDelete-custom.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -fblocks -verify %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.NewDelete -analyzer-config c++-allocator-inlining=true -std=c++11 -fblocks -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 #ifndef LEAKS