[StaticAnalyzer] Fix nullptr dereference issue found by static analyzer tool

[Manna]

Summary

This patch uses castAs instead of getAs which will assert if the type doesn't match and directly uses ReceiverType->castAs<ObjCObjectPointerType>() instead of ReceiverObjectPtrType when calling canAssignObjCInterfaces() in findMethodDecl(clang::​ObjCMessageExpr const *, clang::​ObjCObjectPointerType const *, clang::​ASTContext &).

[Manna]

clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

754–755

We are assigning: ReceiverObjectPtrType = nullptr return value from getAs.

const auto *ReceiverObjectPtrType =
      ReceiverType->getAs<ObjCObjectPointerType>();

Then we are dereferencing nullptr ReceiverObjectPtrType when calling canAssignObjCInterfaces()

[erichkeane]

clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

754–755

This isn't NFC, as ReceiverObjectPtrType is only used here. If the MessageExpr ReceiverKind is not Instance or Class, we never dereference this. So the declaration should be in this branch.

754–755

Oh, and ALSO, we don't dereference it if ReceiverType is ObjCIdType or ObhjCClassType.

[steakhal]

I agree with @erichkeane.
However, I can also see that the code could be improved.

I don't understand why we have that variable hoisted from the guarded block in the first place.
We could directly use ReceiverType->castAs<ObjCObjectPointerType>() instead of ReceiverObjectPtrType.

[Manna]

Thank you @erichkeane and @steakhal for reviews and feedbacks. I have updated patch to address your review comments.

[steakhal]

Please update the title and the summary of this patch to reflect what it actually achieves.
We also usually use the [analyzer] tag only for changes touching StaticAnalyzer stuff.

After these, you can merge the patch.
Thanks for refactoring the to express the intent more clearly.

[Manna]

Thank you @erichkeane and @steakhal for reviews and comments.

In D152194#4402377, @steakhal wrote:

Please update the title and the summary of this patch to reflect what it actually achieves.
We also usually use the [analyzer] tag only for changes touching StaticAnalyzer stuff.

After these, you can merge the patch.

Yes, i will update the title and summary of this patch before merging the patch.

clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp

 /// the method. The more specialized the enclosing class of the method is, the
 /// more likely that the parameter substitution will be successful.
 static const ObjCMethodDecl *
 findMethodDecl(const ObjCMessageExpr *MessageExpr,
                const ObjCObjectPointerType *TrackedType, ASTContext &ASTCtxt) {
   const ObjCMethodDecl *Method = nullptr;
 
   QualType ReceiverType = MessageExpr->getReceiverType();
-  const auto *ReceiverObjectPtrType =
-      ReceiverType->getAs<ObjCObjectPointerType>();
 
   // Do this "devirtualization" on instance and class methods only. Trust the
   // static type on super and super class calls.
   if (MessageExpr->getReceiverKind() == ObjCMessageExpr::Instance ||
       MessageExpr->getReceiverKind() == ObjCMessageExpr::Class) {
     // When the receiver type is id, Class, or some super class of the tracked
     // type, look up the method in the tracked type, not in the receiver type.
     // This way we preserve more information.
     if (ReceiverType->isObjCIdType() || ReceiverType->isObjCClassType() ||
-        ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType, TrackedType)) {
+        ASTCtxt.canAssignObjCInterfaces(
+            ReceiverType->castAs<ObjCObjectPointerType>(), TrackedType)) {
       const ObjCInterfaceDecl *InterfaceDecl = TrackedType->getInterfaceDecl();
       // The method might not be found.
       Selector Sel = MessageExpr->getSelector();
       Method = InterfaceDecl->lookupInstanceMethod(Sel);
       if (!Method)
         Method = InterfaceDecl->lookupClassMethod(Sel);
     }
   }