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 stores through label locations #89265

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

steakhal
Copy link
Contributor

Interestingly, this case crashed from the very beginning of the project, at least starting by clang-3.

As a "fix" I just do the same thing as we do for concrete integers. It might not be the best we could do, but arguably, it's still better than crashing.

Fixes #89185

Interestingly, this case crashed from the very beginning of the project,
at least starting by clang-3.

As a "fix" I just do the same thing as we do for concrete integers.
It might not be the best we could do, but arguably, it's still better
than crashing.

Fixes 89185
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Apr 18, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 18, 2024

@llvm/pr-subscribers-clang

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

Author: Balazs Benics (steakhal)

Changes

Interestingly, this case crashed from the very beginning of the project, at least starting by clang-3.

As a "fix" I just do the same thing as we do for concrete integers. It might not be the best we could do, but arguably, it's still better than crashing.

Fixes #89185


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/StaticAnalyzer/Core/RegionStore.cpp (+4-3)
  • (added) clang/test/Analysis/gh-issue-89185.c (+14)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 6099f8ab02f443..1d98330d2e4a00 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -681,6 +681,8 @@ Static Analyzer
 - Support C++23 static operator calls. (#GH84972)
 - Fixed a crash in ``security.cert.env.InvalidPtr`` checker when accidentally
   matched user-defined ``strerror`` and similar library functions. (GH#88181)
+- Fixed a crash when storing through an address that refers to the address of
+  a label. (GH#89185)
 
 New features
 ^^^^^^^^^^^^
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 755a8c4b22fd9e..2379db9fdb0576 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2358,11 +2358,12 @@ StoreRef RegionStoreManager::killBinding(Store ST, Loc L) {
 
 RegionBindingsRef
 RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
-  if (L.getAs<loc::ConcreteInt>())
+  // We only care about region locations.
+  auto MemRegVal = L.getAs<loc::MemRegionVal>();
+  if (!MemRegVal.has_value())
     return B;
 
-  // If we get here, the location should be a region.
-  const MemRegion *R = L.castAs<loc::MemRegionVal>().getRegion();
+  const MemRegion *R = MemRegVal->getRegion();
 
   // Check if the region is a struct region.
   if (const TypedValueRegion* TR = dyn_cast<TypedValueRegion>(R)) {
diff --git a/clang/test/Analysis/gh-issue-89185.c b/clang/test/Analysis/gh-issue-89185.c
new file mode 100644
index 00000000000000..8a907f198a5fd5
--- /dev/null
+++ b/clang/test/Analysis/gh-issue-89185.c
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_dump(char);
+void clang_analyzer_dump_ptr(char*);
+
+// https://github.com/llvm/llvm-project/issues/89185
+void binding_to_label_loc() {
+  char *b = &&MyLabel;
+MyLabel:
+  *b = 0; // no-crash
+  clang_analyzer_dump_ptr(b); // expected-warning {{&&MyLabel}}
+  clang_analyzer_dump(*b); // expected-warning {{Unknown}}
+  // FIXME: We should never reach here, as storing to a label is invalid.
+}

@steakhal
Copy link
Contributor Author

#89264 Will implement a diagnostic for storing to label addresses. In this patch, I only focus on resolving the crash.

Copy link
Contributor

@NagyDonat NagyDonat left a comment

Choose a reason for hiding this comment

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

LGTM nice quickfix :)

clang/lib/StaticAnalyzer/Core/RegionStore.cpp Outdated Show resolved Hide resolved
@steakhal steakhal merged commit 7d8616e into llvm:main Apr 19, 2024
4 of 5 checks passed
@steakhal steakhal deleted the bb/fix-binding-on-label-locations branch April 19, 2024 14:26
aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 21, 2024
Interestingly, this case crashed from the very beginning of the project,
at least starting by clang-3.

As a "fix" I just do the same thing as we do for concrete integers. It
might not be the best we could do, but arguably, it's still better than
crashing.

Fixes llvm#89185
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.

[analyzer] Clang-19 crash: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
3 participants