[analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

[NoQ]

clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

483–497 ↗

(On Diff #175949)

It's actually just Call.getDecl() and you can turn this into a fall-through.

clang/test/Analysis/osobject-retain-release.cpp

27 ↗

(On Diff #175949)

I think we should use size_t as much as possible, because this may otherwise have weird consequences on platforms on which size_t is not defined as unsigned long. Not sure if this checker is ran on such platforms. But the test doesn't have the triple specified, so it runs under the host triple, which may be arbitrary and cause problems on buildbots.

I.e.,

typedef __typeof(sizeof(int)) size_t;
// use size_t

[NoQ]

clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

297 ↗

(On Diff #175949)

This function is probably also entered for global operator new that isn't a method on any class. Does the function behave sanely in this case?

[NoQ]

clang/test/Analysis/osobject-retain-release.cpp

27 ↗

(On Diff #175949)

http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp

[george.karpenkov]

clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

483–497 ↗

(On Diff #175949)

Call.getDecl() returns a Decl (gotta love Obj-C methods!).

I guess we can group all those cases, and cast the returned decl to FunctionDecl instead of casting the call.

lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

if (const CallExpr *CE = dyn_cast<CallExpr>(S)) {
 
     if (const auto *MD = dyn_cast<CXXMethodDecl>(CE->getCalleeDecl())) {
       os << "Call to method '" << MD->getQualifiedNameAsString() << '\'';
     } else if (FD) {
       os << "Call to function '" << FD->getQualifiedNameAsString() << '\'';
     } else {
       os << "function call";
     }
+  } else if (const auto *NE = dyn_cast<CXXNewExpr>(S)){
+    os << "Operator new";
   } else {
     assert(isa<ObjCMessageExpr>(S));
     CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager();
     CallEventRef<ObjCMethodCall> Call =
         Mgr.getObjCMethodCall(cast<ObjCMessageExpr>(S), CurrSt, LCtx);
 
     switch (Call->getMessageKind()) {
     case OCM_Message:

lib/StaticAnalyzer/Core/RetainSummaryManager.cpp

 RetainSummaryManager::generateSummary(const FunctionDecl *FD,
                                       bool &AllowAnnotations) {
   // We generate "stop" summaries for implicitly defined functions.
   if (FD->isImplicit()) {
     return getPersistentStopSummary();
   }
 
   const IdentifierInfo *II = FD->getIdentifier();
-  if (!II)
-    return getDefaultSummary();
 
-  StringRef FName = II->getName();
+  StringRef FName = II ? II->getName() : "";
 
   // Strip away preceding '_'.  Doing this here will effect all the checks
   // down below.
   FName = FName.substr(FName.find_first_not_of('_'));
 
   // Inspect the result type. Strip away any typedefs.
   const auto *FT = FD->getType()->getAs<FunctionType>();
   QualType RetTy = FT->getReturnType();
RetainSummaryManager::generateSummary(const FunctionDecl *FD,
   if (const auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
     const CXXRecordDecl *Parent = MD->getParent();
     if (TrackOSObjects && Parent && isOSObjectSubclass(Parent)) {
       if (FName == "release")
         return getOSSummaryReleaseRule(FD);
 
       if (FName == "retain")
         return getOSSummaryRetainRule(FD);
+
+      if (MD->getOverloadedOperator() == OO_New)
+        return getOSSummaryCreateRule(MD);
     }
   }
 
   // Check for release functions, the only kind of functions that we care
   // about that don't return a pointer type.
   if (FName.size() >= 2 && FName[0] == 'C' &&
       (FName[1] == 'F' || FName[1] == 'G')) {
     // Test for 'CGCF'.
case CE_CXXMember:
     Summ = getFunctionSummary(cast<CXXMemberCall>(Call).getDecl());
     break;
   case CE_CXXMemberOperator:
     Summ = getFunctionSummary(cast<CXXMemberOperatorCall>(Call).getDecl());
     break;
   case CE_CXXConstructor:
     Summ = getFunctionSummary(cast<CXXConstructorCall>(Call).getDecl());
     break;
+  case CE_CXXAllocator:
+    Summ = getFunctionSummary(cast<CXXAllocatorCall>(Call).getDecl());
+    break;
   case CE_Block:
   case CE_CXXDestructor:
-  case CE_CXXAllocator:
     // FIXME: These calls are currently unsupported.
     return getPersistentStopSummary();
   case CE_ObjCMessage: {
     const ObjCMethodCall &Msg = cast<ObjCMethodCall>(Call);
     if (Msg.isInstanceMessage())
       Summ = getInstanceMethodSummary(Msg, ReceiverType);
     else
       Summ = getClassMethodSummary(Msg);

test/Analysis/osobject-retain-release.cpp

struct OSObject {
 
   unsigned int foo() { return 42; }
 
   static OSObject *generateObject(int);
 
   static OSObject *getObject();
   static OSObject *GetObject();
 
+
+  static void * operator new(unsigned long size);
+
   static const OSMetaClass * const metaClass;
 };
 
 struct OSIterator : public OSObject {
 
   static const OSMetaClass * const metaClass;
 };
 
struct OtherStruct {
   static void doNothingToArray(OSArray *array);
   OtherStruct(OSArray *arr);
 };
 
 struct OSMetaClassBase {
   static OSObject *safeMetaCast(const OSObject *inst, const OSMetaClass *meta);
 };
 
+unsigned int check_leak_explicit_new() {
+  OSArray *arr = new OSArray; // expected-note{{Operator new returns an OSObject of type struct OSArray * with a +1 retain count}}
+  return arr->getCount(); // expected-note{{Object leaked: allocated object of type struct OSArray * is not referenced later in this execution path and has a retain count of +1}}
+                          // expected-warning@-1{{Potential leak of an object of type struct OSArray *}}
+}
+
+unsigned int check_leak_factory() {
+  OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to function 'OSArray::withCapacity' returns an OSObject of type struct OSArray * with a +1 retain count}}
+  return arr->getCount(); // expected-note{{Object leaked: object allocated and stored into 'arr' is not referenced later in this execution path and has a retain count of +1}}
+                          // expected-warning@-1{{Potential leak of an object stored into 'arr'}}
+}
+
 void check_get_object() {
   OSObject::getObject();
 }
 
 void check_Get_object() {
   OSObject::GetObject();
 }