[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-19 Thread Balazs Benics via cfe-commits 

https://github.com/steakhal closed 
https://github.com/llvm/llvm-project/pull/89265
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-19 Thread Balazs Benics via cfe-commits 

https://github.com/steakhal updated 
https://github.com/llvm/llvm-project/pull/89265

>From 50964bf4f694ae21c2ba86b648b241b335e5d6e8 Mon Sep 17 00:00:00 2001
From: Balazs Benics 
Date: Thu, 18 Apr 2024 18:36:29 +0200
Subject: [PATCH 1/2] [analyzer] Fix stores through label locations

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
---
 clang/docs/ReleaseNotes.rst   |  2 ++
 clang/lib/StaticAnalyzer/Core/RegionStore.cpp |  7 ---
 clang/test/Analysis/gh-issue-89185.c  | 14 ++
 3 files changed, 20 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/Analysis/gh-issue-89185.c

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())
+  // We only care about region locations.
+  auto MemRegVal = L.getAs();
+  if (!MemRegVal.has_value())
 return B;
 
-  // If we get here, the location should be a region.
-  const MemRegion *R = L.castAs().getRegion();
+  const MemRegion *R = MemRegVal->getRegion();
 
   // Check if the region is a struct region.
   if (const TypedValueRegion* TR = dyn_cast(R)) {
diff --git a/clang/test/Analysis/gh-issue-89185.c 
b/clang/test/Analysis/gh-issue-89185.c
new file mode 100644
index 00..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:
+  *b = 0; // no-crash
+  clang_analyzer_dump_ptr(b); // expected-warning {{&}}
+  clang_analyzer_dump(*b); // expected-warning {{Unknown}}
+  // FIXME: We should never reach here, as storing to a label is invalid.
+}

>From 739425eafcf1b7f3f1c6e41b8c102fd5eba7e0e8 Mon Sep 17 00:00:00 2001
From: Balazs Benics 
Date: Fri, 19 Apr 2024 15:42:54 +0200
Subject: [PATCH 2/2] NFC Remove `.has_value()`

---
 clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp 
b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 2379db9fdb0576..54e88c07255db7 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2360,7 +2360,7 @@ RegionBindingsRef
 RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
   // We only care about region locations.
   auto MemRegVal = L.getAs();
-  if (!MemRegVal.has_value())
+  if (!MemRegVal)
 return B;
 
   const MemRegion *R = MemRegVal->getRegion();

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-19 Thread Balazs Benics via cfe-commits 

https://github.com/steakhal updated 
https://github.com/llvm/llvm-project/pull/89265

>From 50964bf4f694ae21c2ba86b648b241b335e5d6e8 Mon Sep 17 00:00:00 2001
From: Balazs Benics 
Date: Thu, 18 Apr 2024 18:36:29 +0200
Subject: [PATCH 1/2] [analyzer] Fix stores through label locations

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
---
 clang/docs/ReleaseNotes.rst   |  2 ++
 clang/lib/StaticAnalyzer/Core/RegionStore.cpp |  7 ---
 clang/test/Analysis/gh-issue-89185.c  | 14 ++
 3 files changed, 20 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/Analysis/gh-issue-89185.c

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())
+  // We only care about region locations.
+  auto MemRegVal = L.getAs();
+  if (!MemRegVal.has_value())
 return B;
 
-  // If we get here, the location should be a region.
-  const MemRegion *R = L.castAs().getRegion();
+  const MemRegion *R = MemRegVal->getRegion();
 
   // Check if the region is a struct region.
   if (const TypedValueRegion* TR = dyn_cast(R)) {
diff --git a/clang/test/Analysis/gh-issue-89185.c 
b/clang/test/Analysis/gh-issue-89185.c
new file mode 100644
index 00..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:
+  *b = 0; // no-crash
+  clang_analyzer_dump_ptr(b); // expected-warning {{&}}
+  clang_analyzer_dump(*b); // expected-warning {{Unknown}}
+  // FIXME: We should never reach here, as storing to a label is invalid.
+}

>From 739425eafcf1b7f3f1c6e41b8c102fd5eba7e0e8 Mon Sep 17 00:00:00 2001
From: Balazs Benics 
Date: Fri, 19 Apr 2024 15:42:54 +0200
Subject: [PATCH 2/2] NFC Remove `.has_value()`

---
 clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp 
b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 2379db9fdb0576..54e88c07255db7 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2360,7 +2360,7 @@ RegionBindingsRef
 RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
   // We only care about region locations.
   auto MemRegVal = L.getAs();
-  if (!MemRegVal.has_value())
+  if (!MemRegVal)
 return B;
 
   const MemRegion *R = MemRegVal->getRegion();

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-19 Thread Balazs Benics via cfe-commits 


@@ -2358,11 +2358,12 @@ StoreRef RegionStoreManager::killBinding(Store ST, Loc 
L) {
 
 RegionBindingsRef
 RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
-  if (L.getAs())
+  // We only care about region locations.
+  auto MemRegVal = L.getAs();
+  if (!MemRegVal.has_value())

steakhal wrote:

```suggestion
  if (!MemRegVal)
```

https://github.com/llvm/llvm-project/pull/89265
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-19 Thread via cfe-commits 

https://github.com/NagyDonat approved this pull request.

LGTM nice quickfix :)

https://github.com/llvm/llvm-project/pull/89265
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-19 Thread via cfe-commits 

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/89265
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-19 Thread via cfe-commits 


@@ -2358,11 +2358,12 @@ StoreRef RegionStoreManager::killBinding(Store ST, Loc 
L) {
 
 RegionBindingsRef
 RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
-  if (L.getAs())
+  // We only care about region locations.
+  auto MemRegVal = L.getAs();
+  if (!MemRegVal.has_value())

NagyDonat wrote:

Personal style preference: In this context I would omit the `.has_value()` 
method because it doesn't add meaningful information. (If I wanted to emphasize 
that this is an `optional` and not a pointer; I would've declared the type 
explicitly instead of using `auto`. However, here the pointer/`optional` 
difference is irrelevant for this short-lived temporary variable.)

Of course this is just subjective bikeshedding, no action required.

https://github.com/llvm/llvm-project/pull/89265
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-18 Thread Balazs Benics via cfe-commits 

steakhal wrote:

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

https://github.com/llvm/llvm-project/pull/89265
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-18 Thread via cfe-commits 

llvmbot wrote:




@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
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())
+  // We only care about region locations.
+  auto MemRegVal = L.getAs();
+  if (!MemRegVal.has_value())
 return B;
 
-  // If we get here, the location should be a region.
-  const MemRegion *R = L.castAs().getRegion();
+  const MemRegion *R = MemRegVal->getRegion();
 
   // Check if the region is a struct region.
   if (const TypedValueRegion* TR = dyn_cast(R)) {
diff --git a/clang/test/Analysis/gh-issue-89185.c 
b/clang/test/Analysis/gh-issue-89185.c
new file mode 100644
index 00..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:
+  *b = 0; // no-crash
+  clang_analyzer_dump_ptr(b); // expected-warning {{&}}
+  clang_analyzer_dump(*b); // expected-warning {{Unknown}}
+  // FIXME: We should never reach here, as storing to a label is invalid.
+}

``




https://github.com/llvm/llvm-project/pull/89265
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [analyzer] Fix stores through label locations (PR #89265)

2024-04-18 Thread Balazs Benics via cfe-commits 

https://github.com/steakhal created 
https://github.com/llvm/llvm-project/pull/89265

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

>From 50964bf4f694ae21c2ba86b648b241b335e5d6e8 Mon Sep 17 00:00:00 2001
From: Balazs Benics 
Date: Thu, 18 Apr 2024 18:36:29 +0200
Subject: [PATCH] [analyzer] Fix stores through label locations

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
---
 clang/docs/ReleaseNotes.rst   |  2 ++
 clang/lib/StaticAnalyzer/Core/RegionStore.cpp |  7 ---
 clang/test/Analysis/gh-issue-89185.c  | 14 ++
 3 files changed, 20 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/Analysis/gh-issue-89185.c

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())
+  // We only care about region locations.
+  auto MemRegVal = L.getAs();
+  if (!MemRegVal.has_value())
 return B;
 
-  // If we get here, the location should be a region.
-  const MemRegion *R = L.castAs().getRegion();
+  const MemRegion *R = MemRegVal->getRegion();
 
   // Check if the region is a struct region.
   if (const TypedValueRegion* TR = dyn_cast(R)) {
diff --git a/clang/test/Analysis/gh-issue-89185.c 
b/clang/test/Analysis/gh-issue-89185.c
new file mode 100644
index 00..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:
+  *b = 0; // no-crash
+  clang_analyzer_dump_ptr(b); // expected-warning {{&}}
+  clang_analyzer_dump(*b); // expected-warning {{Unknown}}
+  // FIXME: We should never reach here, as storing to a label is invalid.
+}

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits