[Clang] Fix potential null pointer dereferences in retain cycle detection

[smanna12]

This patch resolves a static analyzer bugs where S.getCurMethodDecl() or SemaRef.getCurMethodDecl() could return nullptr when calling getSelfDecl(() and was being dereferenced without a null check. The fix introduces a check for a non-null return value before accessing getSelfDecl() to ensure safe dereferencing.

This change prevents undefined behavior in scenarios where the current method declaration is not available, thus enhancing the robustness of the retain cycle detection logic.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (smanna12)

Changes

This patch resolves a static analyzer bug where S.getCurMethodDecl() could return nullptr when calling getSelfDecl(() and was being dereferenced without a null check. The fix introduces a check for a non-null return value before accessing getSelfDecl() to ensure safe dereferencing.

This change prevents undefined behavior in scenarios where the current method declaration is not available, thus enhancing the robustness of the retain cycle detection logic.

---

Full diff: https://github.com/llvm/llvm-project/pull/95192.diff

1 Files Affected:

• (modified) clang/lib/Sema/SemaObjC.cpp (+9-5)

diff --git a/clang/lib/Sema/SemaObjC.cpp b/clang/lib/Sema/SemaObjC.cpp
index d396258cfc7d1..69c78f034bd43 100644
--- a/clang/lib/Sema/SemaObjC.cpp
+++ b/clang/lib/Sema/SemaObjC.cpp
@@ -848,12 +848,16 @@ static bool findRetainCycleOwner(Sema &S, Expr *e, RetainCycleOwner &owner) {
 
       owner.Indirect = true;
       if (pre->isSuperReceiver()) {
-        owner.Variable = S.getCurMethodDecl()->getSelfDecl();
-        if (!owner.Variable)
+        if (const auto *CurMethodDecl = S.getCurMethodDecl()) {
+          owner.Variable = CurMethodDecl()->getSelfDecl();
+          if (!owner.Variable)
+            return false;
+          owner.Loc = pre->getLocation();
+          owner.Range = pre->getSourceRange();
+          return true;
+        } else {
           return false;
-        owner.Loc = pre->getLocation();
-        owner.Range = pre->getSourceRange();
-        return true;
+        }
       }
       e = const_cast<Expr *>(
           cast<OpaqueValueExpr>(pre->getBase())->getSourceExpr());

[rjmccall]

Do you have a test case?

[smanna12]

Do you have a test case?

Thanks @rjmccall for reviews. No I do not have any test case. The issue is reported by static analyzer tool.

[rjmccall]

Okay. It looks like it's actually impossible for this to be null — since we're looking at a super dispatch, we must be inside an Objective-C method declaration. We do appreciate people running static analysis on our code base, but please think of analysis reports as the start of an investigation rather than something that needs to be reflexively fixed.