Skip to content

[clang][NFC] Avoid potential null dereferences #127017

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 2 commits into from
Feb 14, 2025

Conversation

schittir
Copy link
Contributor

@schittir schittir commented Feb 13, 2025

Add null checking.

@schittir schittir changed the title [NFC] Avoid potential null deref by adding an assert. [NFC] Avoid potential null dereference. Feb 13, 2025
@schittir schittir marked this pull request as ready for review February 13, 2025 08:31
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. clang:static analyzer clang:openmp OpenMP related changes to Clang labels Feb 13, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 13, 2025

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang-codegen

Author: None (schittir)

Changes

Add null checking.


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

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGOpenMPRuntime.cpp (+2-2)
  • (modified) clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp (+1)
diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index b679d63874b3b..9f7db25a15bec 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -7447,7 +7447,7 @@ class MappableExprsHandler {
           // Update info about the lowest and highest elements for this struct
           if (!PartialStruct.Base.isValid()) {
             PartialStruct.LowestElem = {FieldIndex, LowestElem};
-            if (IsFinalArraySection) {
+            if (IsFinalArraySection && OASE) {
               Address HB =
                   CGF.EmitArraySectionExpr(OASE, /*IsLowerBound=*/false)
                       .getAddress();
@@ -7460,7 +7460,7 @@ class MappableExprsHandler {
           } else if (FieldIndex < PartialStruct.LowestElem.first) {
             PartialStruct.LowestElem = {FieldIndex, LowestElem};
           } else if (FieldIndex > PartialStruct.HighestElem.first) {
-            if (IsFinalArraySection) {
+            if (IsFinalArraySection && OASE) {
               Address HB =
                   CGF.EmitArraySectionExpr(OASE, /*IsLowerBound=*/false)
                       .getAddress();
diff --git a/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
index 12bf12a0b2322..8955cb209c399 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
@@ -314,6 +314,7 @@ void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE,
     RegionArgIsBad = true;
   }
 
+  assert(ArgSM);
   // Is the argument to the call being tracked?
   const AllocationState *AS = State->get<AllocatedData>(ArgSM);
   if (!AS)

@steakhal steakhal changed the title [NFC] Avoid potential null dereference. [clang][NFC] Avoid potential null dereferences Feb 13, 2025
Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

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

Im good with the analyzer part, but im not familiar with the other.
That part is not obvious to me, so id suggest an additional reviewer

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM!

@schittir schittir merged commit 8a0914c into llvm:main Feb 14, 2025
14 checks passed
@schittir schittir deleted the avoid_null_deref branch February 14, 2025 05:26
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025
@steakhal
Copy link
Contributor

@schittir Could you please revert the Static Analyzer part?
It seems a user tripped on it at #128427

sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
steakhal added a commit to steakhal/llvm-project that referenced this pull request Feb 25, 2025
This assertion was hit, as reported by a user.
llvm#128427 (comment)

Ideally, we would reduce and add a regression test for this, but I don't
have the bandwidth for it.

See the summary of the issue llvm#128427 for the reproducer.
@steakhal
Copy link
Contributor

@schittir Could you please revert the Static Analyzer part? It seems a user tripped on it at #128427

The revert is proposed at #128642

steakhal added a commit that referenced this pull request Feb 25, 2025
This assertion was hit, as reported by a user.

#128427 (comment)

Ideally, we would reduce and add a regression test for this, but I don't
have the bandwidth for it.

See the summary of the issue #128427 for the reproducer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang:openmp OpenMP related changes to Clang clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants