Skip to content

[Clang] Prevent null pointer dereference in designated initializer check #97220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

smanna12
Copy link
Contributor

@smanna12 smanna12 commented Jun 30, 2024

This patch adds an assertion in clang::SemaObjC::BuildInstanceMessage() to ensure getCurMethodDecl() returns a valid method declaration, addressing a static analyzer finding.

This patch adds a null check for the current method declaration before
attempting to determine if it is a designated initializer.

This prevents a potential null pointer dereference when `getCurMethodDecl()`
returns nullptr, reported by static analyzer tool in clang::SemaObjC::BuildInstanceMessage().
@smanna12 smanna12 requested a review from tahonermann June 30, 2024 15:44
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 30, 2024

@llvm/pr-subscribers-clang

Author: None (smanna12)

Changes

This patch adds a null check for the current method declaration before attempting to determine if it is a designated initializer.

This prevents a potential null pointer dereference when getCurMethodDecl() returns nullptr, reported by static analyzer tool in clang::SemaObjC::BuildInstanceMessage().


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

1 Files Affected:

  • (modified) clang/lib/Sema/SemaExprObjC.cpp (+4-2)
diff --git a/clang/lib/Sema/SemaExprObjC.cpp b/clang/lib/Sema/SemaExprObjC.cpp
index 7ccecf055feed..05aa30e16ed8e 100644
--- a/clang/lib/Sema/SemaExprObjC.cpp
+++ b/clang/lib/Sema/SemaExprObjC.cpp
@@ -3206,9 +3206,11 @@ ExprResult SemaObjC::BuildInstanceMessage(
     }
     if (!isDesignatedInitChain) {
       const ObjCMethodDecl *InitMethod = nullptr;
+      auto *CurMD = SemaRef.getCurMethodDecl();
+      if (!CurMD)
+        return nullptr;
       bool isDesignated =
-          SemaRef.getCurMethodDecl()->isDesignatedInitializerForTheInterface(
-              &InitMethod);
+          CurMD->isDesignatedInitializerForTheInterface(&InitMethod);
       assert(isDesignated && InitMethod);
       (void)isDesignated;
       Diag(SelLoc, SuperLoc.isValid() ?

Copy link

github-actions bot commented Jun 30, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@tahonermann tahonermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @smanna12. See comments, I think we should take a different approach for this case.

@smanna12 smanna12 requested a review from tahonermann July 16, 2024 18:31
Copy link
Contributor

@tahonermann tahonermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @smanna12, the change looks good!

@smanna12 smanna12 merged commit c628dbd into llvm:main Jul 24, 2024
7 checks passed
@smanna12 smanna12 deleted the FixNullDeref1 branch July 24, 2024 17:43
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…eck (#97220)

Summary:
This patch adds an assertion in clang::SemaObjC::BuildInstanceMessage()
to ensure getCurMethodDecl() returns a valid method declaration,
addressing a static analyzer finding.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60250674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants