Skip to content
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

[analyzer] Fix crash on using bitcast(<type>, <array>) as array subscript #101647

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

pskrgag
Copy link
Contributor

@pskrgag pskrgag commented Aug 2, 2024

Current CSA logic does not expect LazyCompoundValKind as array index. This may happen if array is used as subscript to another, in case of bitcast to integer type.

Catch such cases and return UnknownVal, since CSA cannot model array ->
int casts.

Closes #94496

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Aug 2, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 2, 2024

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

Author: Pavel Skripkin (pskrgag)

Changes

Current CSA logic does not expect LazyCompoundValKind as array index. This may happen if array is used as subscript to another, in case of bitcast to integer type.

Catch such cases and return UnknownVal, since CSA cannot model array ->
int casts.

Closes #94496


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

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Core/Store.cpp (+13-1)
  • (modified) clang/test/Analysis/exercise-ps.c (+7)
diff --git a/clang/lib/StaticAnalyzer/Core/Store.cpp b/clang/lib/StaticAnalyzer/Core/Store.cpp
index 67ca61bb56ba2..72587ef31a17c 100644
--- a/clang/lib/StaticAnalyzer/Core/Store.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -472,7 +472,19 @@ SVal StoreManager::getLValueElement(QualType elementType, NonLoc Offset,
   const auto *ElemR = dyn_cast<ElementRegion>(BaseRegion);
 
   // Convert the offset to the appropriate size and signedness.
-  Offset = svalBuilder.convertToArrayIndex(Offset).castAs<NonLoc>();
+  auto Off = svalBuilder.convertToArrayIndex(Offset).getAs<NonLoc>();
+  if (!Off) {
+    // Handle cases when LazyCompoundVal is used for an array index.
+    // Such case is possible if code does:
+    //
+    //   char b[4];
+    //   a[__builtin_bitcast(int, b)];
+    //
+    // Return UnknownVal, since we cannot model it.
+    return UnknownVal();
+  }
+
+  Offset = Off.value();
 
   if (!ElemR) {
     // If the base region is not an ElementRegion, create one.
diff --git a/clang/test/Analysis/exercise-ps.c b/clang/test/Analysis/exercise-ps.c
index d1e1771afddb5..9bba16c282967 100644
--- a/clang/test/Analysis/exercise-ps.c
+++ b/clang/test/Analysis/exercise-ps.c
@@ -30,3 +30,10 @@ void f3(void *dest) {
   void *src = __builtin_alloca(5);
   memcpy(dest, src, 1); // expected-warning{{2nd function call argument is a pointer to uninitialized value}}
 }
+
+// Reproduce crash from GH#94496. When array is used as subcript to another array, CSA cannot model it
+// and should just assume it's unknown and do not crash.
+void f4(char *array) {
+  char b[4] = {0};
+  array[__builtin_bit_cast(int, b)] = 0x10; // no crash
+}

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 2, 2024

@llvm/pr-subscribers-clang

Author: Pavel Skripkin (pskrgag)

Changes

Current CSA logic does not expect LazyCompoundValKind as array index. This may happen if array is used as subscript to another, in case of bitcast to integer type.

Catch such cases and return UnknownVal, since CSA cannot model array ->
int casts.

Closes #94496


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

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Core/Store.cpp (+13-1)
  • (modified) clang/test/Analysis/exercise-ps.c (+7)
diff --git a/clang/lib/StaticAnalyzer/Core/Store.cpp b/clang/lib/StaticAnalyzer/Core/Store.cpp
index 67ca61bb56ba2..72587ef31a17c 100644
--- a/clang/lib/StaticAnalyzer/Core/Store.cpp
+++ b/clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -472,7 +472,19 @@ SVal StoreManager::getLValueElement(QualType elementType, NonLoc Offset,
   const auto *ElemR = dyn_cast<ElementRegion>(BaseRegion);
 
   // Convert the offset to the appropriate size and signedness.
-  Offset = svalBuilder.convertToArrayIndex(Offset).castAs<NonLoc>();
+  auto Off = svalBuilder.convertToArrayIndex(Offset).getAs<NonLoc>();
+  if (!Off) {
+    // Handle cases when LazyCompoundVal is used for an array index.
+    // Such case is possible if code does:
+    //
+    //   char b[4];
+    //   a[__builtin_bitcast(int, b)];
+    //
+    // Return UnknownVal, since we cannot model it.
+    return UnknownVal();
+  }
+
+  Offset = Off.value();
 
   if (!ElemR) {
     // If the base region is not an ElementRegion, create one.
diff --git a/clang/test/Analysis/exercise-ps.c b/clang/test/Analysis/exercise-ps.c
index d1e1771afddb5..9bba16c282967 100644
--- a/clang/test/Analysis/exercise-ps.c
+++ b/clang/test/Analysis/exercise-ps.c
@@ -30,3 +30,10 @@ void f3(void *dest) {
   void *src = __builtin_alloca(5);
   memcpy(dest, src, 1); // expected-warning{{2nd function call argument is a pointer to uninitialized value}}
 }
+
+// Reproduce crash from GH#94496. When array is used as subcript to another array, CSA cannot model it
+// and should just assume it's unknown and do not crash.
+void f4(char *array) {
+  char b[4] = {0};
+  array[__builtin_bit_cast(int, b)] = 0x10; // no crash
+}

clang/test/Analysis/exercise-ps.c Show resolved Hide resolved
clang/lib/StaticAnalyzer/Core/Store.cpp Outdated Show resolved Hide resolved
clang/lib/StaticAnalyzer/Core/Store.cpp Show resolved Hide resolved
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.

LGTM already. You may decide to fix add the extra dump or not.

@steakhal steakhal changed the title [clang][Static analyzer] fix crash on using bitcast(<type>, <array>) as array subscript [analyzer] Fix crash on using bitcast(<type>, <array>) as array subscript Aug 2, 2024
@steakhal steakhal merged commit d96569e into llvm:main Aug 2, 2024
5 of 6 checks passed
@steakhal
Copy link
Contributor

steakhal commented Aug 2, 2024

Backporting to clang-19 in #101684

tru pushed a commit to steakhal/llvm-project that referenced this pull request Aug 4, 2024
…script (llvm#101647)

Current CSA logic does not expect `LazyCompoundValKind` as array index.
This may happen if array is used as subscript to another, in case of
bitcast to integer type.

Catch such cases and return `UnknownVal`, since CSA cannot model
array -> int casts.

Closes llvm#94496

(cherry picked from commit d96569e)
@ormris
Copy link
Collaborator

ormris commented Aug 5, 2024

Thanks! Appreciate the fix.

banach-space pushed a commit to banach-space/llvm-project that referenced this pull request Aug 7, 2024
…script (llvm#101647)

Current CSA logic does not expect `LazyCompoundValKind` as array index.
This may happen if array is used as subscript to another, in case of
bitcast to integer type.

Catch such cases and return `UnknownVal`, since CSA cannot model
array -> int casts.

Closes llvm#94496
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[clang][static analyzer] Crash when program contains __builtin_bit_cast
4 participants