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

[MTC]

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

[NoQ]

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]

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]

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]

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]

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]

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

[MTC]

Update comments.

[zaks.anna]

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]

This comment was removed by MTC.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

1282

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

[MTC]

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]

In D36708#853442, @zaks.anna wrote:

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]

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

[NoQ]

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]

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.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

ProgramStateRef MallocChecker::MallocUpdateRefState(CheckerContext &C,
   // Get the return value.
   SVal retVal = State->getSVal(E, C.getLocationContext());
 
   // We expect the malloc functions to return a pointer.
   if (!retVal.getAs<Loc>())
     return nullptr;
 
   SymbolRef Sym = retVal.getAsLocSymbol();
-  assert(Sym);
+
+  // Special case when the 'c++-allocator-inlining' config option sets true and
+  // the c++ allocator return a Null pointer.
+  if (!Sym)
+    return nullptr;
 
   // Set the symbol's state to Allocated.
   return State->set<RegionState>(Sym, RefState::getAllocated(Family, E));
 }
 
 ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C,
                                            const CallExpr *CE,
                                            const OwnershipAttr *Att,
                                            ProgramStateRef State) const {
   if (!State)
     return nullptr;
 

test/Analysis/inline.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-config ipa=inlining -analyzer-config c++-allocator-inlining=true -verify %s
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-config ipa=inlining -analyzer-config c++-allocator-inlining=true -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_checkInlined(bool);
 
 typedef __typeof__(sizeof(int)) size_t;
 extern "C" void *malloc(size_t);
 
 // This is the standard placement new.

 }
 
 namespace bug16307 {
   void one_argument(int a) { }
   void call_with_less() {
     reinterpret_cast<void (*)()>(one_argument)(); // expected-warning{{Function taking 1 argument is called with fewer (0)}}
   }
 }
+
+namespace std {
+  struct nothrow_t {};
+  extern const nothrow_t nothrow;
+}
+
+// Operator new, the nothrow version.
+void* operator new(size_t size, const std::nothrow_t&) noexcept {
+  void *p = nullptr;
+  return p;
+}
+
+namespace bug34144 {
+  // Don't crash when the 'c++-allocator-inlining' config option sets true and
+  // the c++ allocator return a Null pointer.
+  void call_new() {
+    int *i = new(std::nothrow) int(1);
+    delete i;
+  }
+}