[clang] [analyzer] Model constructor initializer for an array member (PR #107537)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/107537
Bind the array member to the compound region associated with the initializer
list, e.g.:
class C {
int arr[2];
C() : arr{1, 2} {}
};
C c;
This change enables correct values in `c.arr[0]` and `c.arr[1]`
CPP-5647
>From 63c856732aeda977786534d66597d0aba12cb0d4 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 3 Sep 2024 18:01:02 +0200
Subject: [PATCH] [analyzer] Model constructor initializer for an array member
Bind the array member to the compound region associated with the
initializer list
CPP-5647
---
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 9 ++-
clang/test/Analysis/ctor-array.cpp | 66
clang/test/Analysis/nullptr.cpp | 16 +
3 files changed, 77 insertions(+), 14 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index dfb7111b512552..a577c0b02c83f0 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1207,9 +1207,14 @@ void ExprEngine::ProcessInitializer(const CFGInitializer
CFGInit,
Init = ASE->getBase()->IgnoreImplicit();
SVal LValue = State->getSVal(Init, stackFrame);
-if (!Field->getType()->isReferenceType())
- if (std::optional LValueLoc = LValue.getAs())
+if (!Field->getType()->isReferenceType()) {
+ if (std::optional LValueLoc = LValue.getAs()) {
InitVal = State->getSVal(*LValueLoc);
+ } else if (auto CV = LValue.getAs()) {
+// initializer list for an array
+InitVal = *CV;
+ }
+}
// If we fail to get the value for some reason, use a symbolic value.
if (InitVal.isUnknownOrUndef()) {
diff --git a/clang/test/Analysis/ctor-array.cpp
b/clang/test/Analysis/ctor-array.cpp
index 49412ee5a68c70..40c479cd39e31e 100644
--- a/clang/test/Analysis/ctor-array.cpp
+++ b/clang/test/Analysis/ctor-array.cpp
@@ -234,16 +234,58 @@ struct Parent {
void member() {
Parent arr[2];
- // FIXME: Ideally these are TRUE, but at the moment InitListExpr has no
- // knowledge about where the initializer list is used, so we can't bind
- // the initializer list to the required region.
- clang_analyzer_eval(arr[0].arr[0].x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[0].arr[0].y == 2); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[0].arr[1].x == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[0].arr[1].y == 4); // expected-warning{{UNKNOWN}}
-
- clang_analyzer_eval(arr[1].arr[0].x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[1].arr[0].y == 2); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[1].arr[1].x == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[1].arr[1].y == 4); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(arr[0].arr[0].x == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[0].arr[0].y == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[0].arr[1].x == 3); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[0].arr[1].y == 4); // expected-warning{{TRUE}}
+
+ clang_analyzer_eval(arr[1].arr[0].x == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[1].arr[0].y == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[1].arr[1].x == 3); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[1].arr[1].y == 4); // expected-warning{{TRUE}}
+}
+
+struct HasArr {
+ int arrDefault[2] = {1, 2};
+ int arr[2];
+ HasArr(int x, int y) : arr{x, y} {}
+};
+
+struct ArrCombination : public HasArr {
+HasArr membDefault = {5, 6};
+HasArr memb;
+ArrCombination(int x) : HasArr(3, 4), memb{7, x} {}
+};
+
+void derived_and_member() {
+ ArrCombination a{8};
+ // FIXME: default initializers for array members are not modelled
+ clang_analyzer_eval(a.arrDefault[0] == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.arrDefault[1] == 2); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.arr[0] == 3); // expected-warning{{TRUE}}
+ clang_analyzer_eval(a.arr[1] == 4); // expected-warning{{TRUE}}
+ clang_analyzer_eval(a.membDefault.arrDefault[0] == 1); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.membDefault.arrDefault[1] == 2); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.membDefault.arr[0] == 5); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.membDefault.arr[1] == 6); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.memb.arrDefault[0] == 1); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.memb.arrDefault[1] == 2); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.memb.arr[0] == 7); // expected-warning{{TRUE}}
+ clang_analyzer_eval(a.memb.arr[1] == 8); // expected-warning{{TRUE}}
+
+}
+
+struct IncompleteArrInit {
+ int arr[2];
+ int arrDefault[3] = {1, 2, 3};
+ Incompl
[clang] [analyzer] Model constructor initializer for an array member (PR #107537)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/107537
>From 63c856732aeda977786534d66597d0aba12cb0d4 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 3 Sep 2024 18:01:02 +0200
Subject: [PATCH 1/2] [analyzer] Model constructor initializer for an array
member
Bind the array member to the compound region associated with the
initializer list
CPP-5647
---
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 9 ++-
clang/test/Analysis/ctor-array.cpp | 66
clang/test/Analysis/nullptr.cpp | 16 +
3 files changed, 77 insertions(+), 14 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index dfb7111b512552..a577c0b02c83f0 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1207,9 +1207,14 @@ void ExprEngine::ProcessInitializer(const CFGInitializer
CFGInit,
Init = ASE->getBase()->IgnoreImplicit();
SVal LValue = State->getSVal(Init, stackFrame);
-if (!Field->getType()->isReferenceType())
- if (std::optional LValueLoc = LValue.getAs())
+if (!Field->getType()->isReferenceType()) {
+ if (std::optional LValueLoc = LValue.getAs()) {
InitVal = State->getSVal(*LValueLoc);
+ } else if (auto CV = LValue.getAs()) {
+// initializer list for an array
+InitVal = *CV;
+ }
+}
// If we fail to get the value for some reason, use a symbolic value.
if (InitVal.isUnknownOrUndef()) {
diff --git a/clang/test/Analysis/ctor-array.cpp
b/clang/test/Analysis/ctor-array.cpp
index 49412ee5a68c70..40c479cd39e31e 100644
--- a/clang/test/Analysis/ctor-array.cpp
+++ b/clang/test/Analysis/ctor-array.cpp
@@ -234,16 +234,58 @@ struct Parent {
void member() {
Parent arr[2];
- // FIXME: Ideally these are TRUE, but at the moment InitListExpr has no
- // knowledge about where the initializer list is used, so we can't bind
- // the initializer list to the required region.
- clang_analyzer_eval(arr[0].arr[0].x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[0].arr[0].y == 2); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[0].arr[1].x == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[0].arr[1].y == 4); // expected-warning{{UNKNOWN}}
-
- clang_analyzer_eval(arr[1].arr[0].x == 1); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[1].arr[0].y == 2); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[1].arr[1].x == 3); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(arr[1].arr[1].y == 4); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(arr[0].arr[0].x == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[0].arr[0].y == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[0].arr[1].x == 3); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[0].arr[1].y == 4); // expected-warning{{TRUE}}
+
+ clang_analyzer_eval(arr[1].arr[0].x == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[1].arr[0].y == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[1].arr[1].x == 3); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr[1].arr[1].y == 4); // expected-warning{{TRUE}}
+}
+
+struct HasArr {
+ int arrDefault[2] = {1, 2};
+ int arr[2];
+ HasArr(int x, int y) : arr{x, y} {}
+};
+
+struct ArrCombination : public HasArr {
+HasArr membDefault = {5, 6};
+HasArr memb;
+ArrCombination(int x) : HasArr(3, 4), memb{7, x} {}
+};
+
+void derived_and_member() {
+ ArrCombination a{8};
+ // FIXME: default initializers for array members are not modelled
+ clang_analyzer_eval(a.arrDefault[0] == 1); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.arrDefault[1] == 2); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.arr[0] == 3); // expected-warning{{TRUE}}
+ clang_analyzer_eval(a.arr[1] == 4); // expected-warning{{TRUE}}
+ clang_analyzer_eval(a.membDefault.arrDefault[0] == 1); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.membDefault.arrDefault[1] == 2); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.membDefault.arr[0] == 5); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.membDefault.arr[1] == 6); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.memb.arrDefault[0] == 1); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.memb.arrDefault[1] == 2); //
expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a.memb.arr[0] == 7); // expected-warning{{TRUE}}
+ clang_analyzer_eval(a.memb.arr[1] == 8); // expected-warning{{TRUE}}
+
+}
+
+struct IncompleteArrInit {
+ int arr[2];
+ int arrDefault[3] = {1, 2, 3};
+ IncompleteArrInit() : arr{1}, arrDefault{2, 3} {}
+};
+
+void incomplete_array_init() {
+ IncompleteArrInit a;
+ clang_analyzer_eval(a.arr[0] == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(a.arr[1] == 0); // expected-warning{{TRU
[clang] [analyzer] Model constructor initializer for an array member (PR #107537)
necto wrote: > LGTM. FYI "modelled" should contain only 1 "l" if I'm not mistaken. Also llvm > style suggests capitalizing and punctuating comments. None of these are > blockers. fixed eecf42ac8fc4 https://github.com/llvm/llvm-project/pull/107537 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Keep alive short-circuiting condition subexpressions in a conditional (PR #100745)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/100745 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+return false;
+ }
+
+ if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
+ }
+ if (isa(ReferrerMemSpace) &&
+ ReferrerStackSpace->getStackFrame() == PoppedFrame && TopFrame) {
+// Output parameter of a top-level function
V.emplace_back(Referrer, Referred);
return true;
}
return false;
}
+void updateInvalidatedRegions(const MemRegion *Region) {
+ if (const auto *SymReg = Region->getAs()) {
+SymbolRef Symbol = SymReg->getSymbol();
+if (const auto *DerS = dyn_cast(Symbol);
+DerS && isa_and_nonnull(DerS->getParentSymbol())) {
+
InvalidatedRegions.insert(Symbol->getOriginRegion()->getBaseRegion());
+}
+ }
+}
necto wrote:
I chose the variable name poorly. Here `InvalidatedRegions` are not invalidated
in general, they are only excluded from the reports of this particular checker,
i.e., it is completely local. I'll rename it into something more benign.
https://github.com/llvm/llvm-project/pull/105648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106048
>From 7d5ae515f7727de98e7e8ce2f259e579a1f24463 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 17:31:11 +0200
Subject: [PATCH 1/7] [analyzer] Report violations of the "returns_nonnull"
attribute
Make sure code respects the GNU-extension
__attribute__((returns_nonnull)).
Extend the NullabilityChecker to check that a function returns_nonnull
does not return a nullptr.
CPP-4741
---
.../Checkers/NullabilityChecker.cpp | 8
clang/test/Analysis/nullability.c | 43 ++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 60934e51febe84..2035d50eea4c2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
NullConstraint Nullness = getNullConstraint(*RetSVal, State);
Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType);
+ if (const auto *FunDecl = C.getLocationContext()->getDecl();
+ FunDecl && FunDecl->getAttr() &&
+ (RequiredNullability == Nullability::Unspecified ||
+ RequiredNullability == Nullability::Nullable)) {
+// If a function is marked with the returns_nonnull attribute,
+// the return value must be non-null.
+RequiredNullability = Nullability::Nonnull;
+ }
// If the returned value is null but the type of the expression
// generating it is nonnull then we will suppress the diagnostic.
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index fbc03c864ad83f..9ddb9c8d2dc34f 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability
-Wno-deprecated-non-prototype -verify %s
+// RUN: %clang_analyze_cc1 -fblocks
-analyzer-checker=core,nullability,debug.ExprInspection
-Wno-deprecated-non-prototype -verify %s
+
+void clang_analyzer_warnIfReached();
void it_takes_two(int a, int b);
void function_pointer_arity_mismatch() {
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
+ }
+ // Regardless of the potential state split above, x cannot be nullptr
+ // according to the produce_nonnull_ptr annotation.
+ return x;
+ // False positive: expected-warning@-1 {{Null returned from a function that
is expected to return a non-null value}}
+}
+
+__attribute__((returns_nonnull)) int *passthrough(int *p) {
+ return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
+}
>From d3b30f3f31cc3a4cc71ae967ea6fc554a4764e37 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Mon, 26 Aug 2024 15:59:59 +0200
Subject: [PATCH 2/7] Update clang/test/Analysis/nullability.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-authored-by: Donát Nagy
---
clang/test/Analysis/nullability.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index 9ddb9c8d2dc34f..341d29c6e99d1e 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -39,8 +39,9 @@ int *cannot_return_null() {
int *x = produce_nonnull_ptr();
if (!x) {
clang_analyzer_warnIfReached();
-// Incorrect: expected-warning@-1 {{REACHABLE}}
-// According to produce_nonnull_ptr contract,
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
@@ -588,8 +588,8 @@ Warns when a null pointer is passed to a pointer which has a _Nonnull type. .. _nullability-NullReturnedFromNonnull: -nullability.NullReturnedFromNonnull (ObjC) -"" +nullability.NullReturnedFromNonnull +""" necto wrote: Yes, it is relevant for both. Thanks for the fix! Applied https://github.com/llvm/llvm-project/pull/106048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106048
>From 7d5ae515f7727de98e7e8ce2f259e579a1f24463 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 17:31:11 +0200
Subject: [PATCH 1/8] [analyzer] Report violations of the "returns_nonnull"
attribute
Make sure code respects the GNU-extension
__attribute__((returns_nonnull)).
Extend the NullabilityChecker to check that a function returns_nonnull
does not return a nullptr.
CPP-4741
---
.../Checkers/NullabilityChecker.cpp | 8
clang/test/Analysis/nullability.c | 43 ++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 60934e51febe84..2035d50eea4c2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
NullConstraint Nullness = getNullConstraint(*RetSVal, State);
Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType);
+ if (const auto *FunDecl = C.getLocationContext()->getDecl();
+ FunDecl && FunDecl->getAttr() &&
+ (RequiredNullability == Nullability::Unspecified ||
+ RequiredNullability == Nullability::Nullable)) {
+// If a function is marked with the returns_nonnull attribute,
+// the return value must be non-null.
+RequiredNullability = Nullability::Nonnull;
+ }
// If the returned value is null but the type of the expression
// generating it is nonnull then we will suppress the diagnostic.
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index fbc03c864ad83f..9ddb9c8d2dc34f 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability
-Wno-deprecated-non-prototype -verify %s
+// RUN: %clang_analyze_cc1 -fblocks
-analyzer-checker=core,nullability,debug.ExprInspection
-Wno-deprecated-non-prototype -verify %s
+
+void clang_analyzer_warnIfReached();
void it_takes_two(int a, int b);
void function_pointer_arity_mismatch() {
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
+ }
+ // Regardless of the potential state split above, x cannot be nullptr
+ // according to the produce_nonnull_ptr annotation.
+ return x;
+ // False positive: expected-warning@-1 {{Null returned from a function that
is expected to return a non-null value}}
+}
+
+__attribute__((returns_nonnull)) int *passthrough(int *p) {
+ return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
+}
>From d3b30f3f31cc3a4cc71ae967ea6fc554a4764e37 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Mon, 26 Aug 2024 15:59:59 +0200
Subject: [PATCH 2/8] Update clang/test/Analysis/nullability.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-authored-by: Donát Nagy
---
clang/test/Analysis/nullability.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index 9ddb9c8d2dc34f..341d29c6e99d1e 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -39,8 +39,9 @@ int *cannot_return_null() {
int *x = produce_nonnull_ptr();
if (!x) {
clang_analyzer_warnIfReached();
-// Incorrect: expected-warning@-1 {{REACHABLE}}
-// According to produce_nonnull_ptr contract,
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
@@ -51,3 +54,15 @@ int *cannot_return_null() {
__attribute__((returns_nonnull)) int *passthrough(int *p) {
return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
}
+
+__attribute__((noreturn))
+void exit(int);
necto wrote:
It is not used, indeed; thanks for spotting it!
It was relevant in one of the working versions of the test case, but not in the
committed one. Removed in 41956123
https://github.com/llvm/llvm-project/pull/106048
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From 991f176c5545fedae2ba8b5c1b357734abe68ac7 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 11:16:10 +0200
Subject: [PATCH 1/2] [analyzer] Detect leaks on top-level via output params,
indirect globals
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.
The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 +-
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 13 +-
.../lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 2 +-
clang/test/Analysis/copy-elision.cpp | 20 +--
.../test/Analysis/incorrect-checker-names.cpp | 4 +-
clang/test/Analysis/loop-block-counts.c | 2 +-
clang/test/Analysis/stack-addr-ps.c | 6 +-
clang/test/Analysis/stack-addr-ps.cpp | 117 ++
.../Analysis/stack-capture-leak-no-arc.mm | 4 +-
clang/test/Analysis/stackaddrleak.c | 8 +-
10 files changed, 157 insertions(+), 83 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index dcf6801a73de2d..82f03e5f7d09d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -305,6 +305,14 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
return nullptr;
}
+const MemRegion *getOriginBaseRegion(const MemRegion *Referrer) {
+ Referrer = Referrer->getBaseRegion();
+ while (const auto *SymReg = dyn_cast(Referrer)) {
+Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+ }
+ return Referrer;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -313,7 +321,8 @@ std::optional printReferrer(const MemRegion
*Referrer) {
if (isa(Space))
return "global";
assert(isa(Space));
-return "stack";
+// This case covers top-level and inlined analyses.
+return "caller";
}(getStackOrGlobalSpaceRegion(Referrer));
while (!Referrer->canPrintPretty()) {
@@ -348,12 +357,27 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
ExplodedNode *Node = Ctx.getPredecessor();
+ bool ExitingTopFrame =
+ Ctx.getPredecessor()->getLocationContext()->inTopFrame();
+
+ if (ExitingTopFrame && Node->getLocation().getTag() &&
+ Node->getLocation().getTag()->getTagDescription() ==
+ "ExprEngine : Clean Node" &&
+ Node->getFirstPred()) {
+// When finishing analysis of a top-level function, engine proactively
+// removes dead symbols thus preventing this checker from looking through
+// the output parameters. Take 1 step back, to the node where these symbols
+// and their bindings are still present
+Node = Node->getFirstPred();
+ }
+
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
class CallBack : public StoreManager::BindingsHandler {
private:
CheckerContext &Ctx;
const StackFrameContext *PoppedFrame;
+const bool TopFrame;
/// Look for stack variables referring to popped stack variables.
/// Returns true only if it found some dangling stack variables
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+return false;
+ }
+
+ if (ReferrerStackSpace->getS
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+return false;
+ }
+
+ if (ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
+ }
+ if (isa(ReferrerMemSpace) &&
+ ReferrerStackSpace->getStackFrame() == PoppedFrame && TopFrame) {
+// Output parameter of a top-level function
V.emplace_back(Referrer, Referred);
return true;
}
return false;
}
+void updateInvalidatedRegions(const MemRegion *Region) {
+ if (const auto *SymReg = Region->getAs()) {
+SymbolRef Symbol = SymReg->getSymbol();
+if (const auto *DerS = dyn_cast(Symbol);
+DerS && isa_and_nonnull(DerS->getParentSymbol())) {
+
InvalidatedRegions.insert(Symbol->getOriginRegion()->getBaseRegion());
+}
+ }
+}
necto wrote:
Renamed as `ExcludedRegions` in 9075f2e1687d
Is it clear with the new name that the checker does not do any invalidation?
https://github.com/llvm/llvm-project/pull/105648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/106048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106048
>From 7d5ae515f7727de98e7e8ce2f259e579a1f24463 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 17:31:11 +0200
Subject: [PATCH 1/8] [analyzer] Report violations of the "returns_nonnull"
attribute
Make sure code respects the GNU-extension
__attribute__((returns_nonnull)).
Extend the NullabilityChecker to check that a function returns_nonnull
does not return a nullptr.
CPP-4741
---
.../Checkers/NullabilityChecker.cpp | 8
clang/test/Analysis/nullability.c | 43 ++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 60934e51febe84..2035d50eea4c2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
NullConstraint Nullness = getNullConstraint(*RetSVal, State);
Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType);
+ if (const auto *FunDecl = C.getLocationContext()->getDecl();
+ FunDecl && FunDecl->getAttr() &&
+ (RequiredNullability == Nullability::Unspecified ||
+ RequiredNullability == Nullability::Nullable)) {
+// If a function is marked with the returns_nonnull attribute,
+// the return value must be non-null.
+RequiredNullability = Nullability::Nonnull;
+ }
// If the returned value is null but the type of the expression
// generating it is nonnull then we will suppress the diagnostic.
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index fbc03c864ad83f..9ddb9c8d2dc34f 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability
-Wno-deprecated-non-prototype -verify %s
+// RUN: %clang_analyze_cc1 -fblocks
-analyzer-checker=core,nullability,debug.ExprInspection
-Wno-deprecated-non-prototype -verify %s
+
+void clang_analyzer_warnIfReached();
void it_takes_two(int a, int b);
void function_pointer_arity_mismatch() {
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
+ }
+ // Regardless of the potential state split above, x cannot be nullptr
+ // according to the produce_nonnull_ptr annotation.
+ return x;
+ // False positive: expected-warning@-1 {{Null returned from a function that
is expected to return a non-null value}}
+}
+
+__attribute__((returns_nonnull)) int *passthrough(int *p) {
+ return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
+}
>From d3b30f3f31cc3a4cc71ae967ea6fc554a4764e37 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Mon, 26 Aug 2024 15:59:59 +0200
Subject: [PATCH 2/8] Update clang/test/Analysis/nullability.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-authored-by: Donát Nagy
---
clang/test/Analysis/nullability.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index 9ddb9c8d2dc34f..341d29c6e99d1e 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -39,8 +39,9 @@ int *cannot_return_null() {
int *x = produce_nonnull_ptr();
if (!x) {
clang_analyzer_warnIfReached();
-// Incorrect: expected-warning@-1 {{REACHABLE}}
-// According to produce_nonnull_ptr contract,
[clang] [analyzer][NFC] Add tests for and refactor StackAddrEscapeChecker 1/3 (PR #105652)
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
necto wrote:
I found this style in a few other messages:
> Call to 'dispatch_once' uses the local variable 'once' for the predicate
> value. Using such transient memory for the predicate is potentially
> dangerous. Perhaps you intended to declare the variable as 'static'?
[link](https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/dispatch-once.m#L24)
> Object leaked: object allocated and stored into 'object' is returned from a
> function whose name ('CFGetRuleViolation') does not contain 'Copy' or
> 'Create'. This violates the naming convention rules given in the Memory
> Management Guide for Core Foundation
[link](https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/retain-release-path-notes.m#L115C61-L115C316)
> The return value from the call to 'setuid' is not checked. If an error
> occurs in 'setuid', the following code may execute with unexpected privileges
[link](https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/security-syntax-checks.m#L103)
> Function 'rand' is obsolete because it implements a poor random number
> generator. Use 'arc4random' instead
[link](https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/security-syntax-checks.m#L138)
Overall, if lit-test messages stats is of any indication, it is pretty balanced:
grep -R 'warning{.*\. [A-Za-z]' clang/test/ | wc -l
-> 20 (double space after dot)
grep -R 'warning{.*\. [A-Za-z]' clang/test/ | wc -l
-> 26 (single space after dot)
https://github.com/llvm/llvm-project/pull/105652
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Add tests for and refactor StackAddrEscapeChecker 1/3 (PR #105652)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/105652 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Add tests for and refactor StackAddrEscapeChecker 1/3 (PR #105652)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/105652 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From 991f176c5545fedae2ba8b5c1b357734abe68ac7 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 11:16:10 +0200
Subject: [PATCH 1/3] [analyzer] Detect leaks on top-level via output params,
indirect globals
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.
The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 +-
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 13 +-
.../lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 2 +-
clang/test/Analysis/copy-elision.cpp | 20 +--
.../test/Analysis/incorrect-checker-names.cpp | 4 +-
clang/test/Analysis/loop-block-counts.c | 2 +-
clang/test/Analysis/stack-addr-ps.c | 6 +-
clang/test/Analysis/stack-addr-ps.cpp | 117 ++
.../Analysis/stack-capture-leak-no-arc.mm | 4 +-
clang/test/Analysis/stackaddrleak.c | 8 +-
10 files changed, 157 insertions(+), 83 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index dcf6801a73de2d..82f03e5f7d09d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -305,6 +305,14 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
return nullptr;
}
+const MemRegion *getOriginBaseRegion(const MemRegion *Referrer) {
+ Referrer = Referrer->getBaseRegion();
+ while (const auto *SymReg = dyn_cast(Referrer)) {
+Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+ }
+ return Referrer;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -313,7 +321,8 @@ std::optional printReferrer(const MemRegion
*Referrer) {
if (isa(Space))
return "global";
assert(isa(Space));
-return "stack";
+// This case covers top-level and inlined analyses.
+return "caller";
}(getStackOrGlobalSpaceRegion(Referrer));
while (!Referrer->canPrintPretty()) {
@@ -348,12 +357,27 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
ExplodedNode *Node = Ctx.getPredecessor();
+ bool ExitingTopFrame =
+ Ctx.getPredecessor()->getLocationContext()->inTopFrame();
+
+ if (ExitingTopFrame && Node->getLocation().getTag() &&
+ Node->getLocation().getTag()->getTagDescription() ==
+ "ExprEngine : Clean Node" &&
+ Node->getFirstPred()) {
+// When finishing analysis of a top-level function, engine proactively
+// removes dead symbols thus preventing this checker from looking through
+// the output parameters. Take 1 step back, to the node where these symbols
+// and their bindings are still present
+Node = Node->getFirstPred();
+ }
+
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
class CallBack : public StoreManager::BindingsHandler {
private:
CheckerContext &Ctx;
const StackFrameContext *PoppedFrame;
+const bool TopFrame;
/// Look for stack variables referring to popped stack variables.
/// Returns true only if it found some dangling stack variables
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+return false;
+ }
+
+ if (ReferrerStackSpace->getS
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
@@ -424,6 +481,9 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
for (const auto &P : Cb.V) {
const MemRegion *Referrer = P.first->getBaseRegion();
const MemRegion *Referred = P.second;
+if (Cb.ExcludedRegions.contains(getOriginBaseRegion(Referrer))) {
necto wrote:
Fixed in 9e048c8d43fd
https://github.com/llvm/llvm-project/pull/105648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From 991f176c5545fedae2ba8b5c1b357734abe68ac7 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 11:16:10 +0200
Subject: [PATCH 1/4] [analyzer] Detect leaks on top-level via output params,
indirect globals
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.
The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 +-
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 13 +-
.../lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 2 +-
clang/test/Analysis/copy-elision.cpp | 20 +--
.../test/Analysis/incorrect-checker-names.cpp | 4 +-
clang/test/Analysis/loop-block-counts.c | 2 +-
clang/test/Analysis/stack-addr-ps.c | 6 +-
clang/test/Analysis/stack-addr-ps.cpp | 117 ++
.../Analysis/stack-capture-leak-no-arc.mm | 4 +-
clang/test/Analysis/stackaddrleak.c | 8 +-
10 files changed, 157 insertions(+), 83 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index dcf6801a73de2d..82f03e5f7d09d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -305,6 +305,14 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
return nullptr;
}
+const MemRegion *getOriginBaseRegion(const MemRegion *Referrer) {
+ Referrer = Referrer->getBaseRegion();
+ while (const auto *SymReg = dyn_cast(Referrer)) {
+Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+ }
+ return Referrer;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -313,7 +321,8 @@ std::optional printReferrer(const MemRegion
*Referrer) {
if (isa(Space))
return "global";
assert(isa(Space));
-return "stack";
+// This case covers top-level and inlined analyses.
+return "caller";
}(getStackOrGlobalSpaceRegion(Referrer));
while (!Referrer->canPrintPretty()) {
@@ -348,12 +357,27 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
ExplodedNode *Node = Ctx.getPredecessor();
+ bool ExitingTopFrame =
+ Ctx.getPredecessor()->getLocationContext()->inTopFrame();
+
+ if (ExitingTopFrame && Node->getLocation().getTag() &&
+ Node->getLocation().getTag()->getTagDescription() ==
+ "ExprEngine : Clean Node" &&
+ Node->getFirstPred()) {
+// When finishing analysis of a top-level function, engine proactively
+// removes dead symbols thus preventing this checker from looking through
+// the output parameters. Take 1 step back, to the node where these symbols
+// and their bindings are still present
+Node = Node->getFirstPred();
+ }
+
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
class CallBack : public StoreManager::BindingsHandler {
private:
CheckerContext &Ctx;
const StackFrameContext *PoppedFrame;
+const bool TopFrame;
/// Look for stack variables referring to popped stack variables.
/// Returns true only if it found some dangling stack variables
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+return false;
+ }
+
+ if (ReferrerStackSpace->getS
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
@@ -305,6 +305,14 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
return nullptr;
}
+const MemRegion *getOriginBaseRegion(const MemRegion *Referrer) {
+ Referrer = Referrer->getBaseRegion();
+ while (const auto *SymReg = dyn_cast(Referrer)) {
+Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+ }
+ return Referrer;
necto wrote:
renamed in bc41e99d6d34
https://github.com/llvm/llvm-project/pull/105648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From 991f176c5545fedae2ba8b5c1b357734abe68ac7 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 11:16:10 +0200
Subject: [PATCH 1/5] [analyzer] Detect leaks on top-level via output params,
indirect globals
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.
The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 +-
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 13 +-
.../lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 2 +-
clang/test/Analysis/copy-elision.cpp | 20 +--
.../test/Analysis/incorrect-checker-names.cpp | 4 +-
clang/test/Analysis/loop-block-counts.c | 2 +-
clang/test/Analysis/stack-addr-ps.c | 6 +-
clang/test/Analysis/stack-addr-ps.cpp | 117 ++
.../Analysis/stack-capture-leak-no-arc.mm | 4 +-
clang/test/Analysis/stackaddrleak.c | 8 +-
10 files changed, 157 insertions(+), 83 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index dcf6801a73de2d..82f03e5f7d09d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -305,6 +305,14 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
return nullptr;
}
+const MemRegion *getOriginBaseRegion(const MemRegion *Referrer) {
+ Referrer = Referrer->getBaseRegion();
+ while (const auto *SymReg = dyn_cast(Referrer)) {
+Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+ }
+ return Referrer;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -313,7 +321,8 @@ std::optional printReferrer(const MemRegion
*Referrer) {
if (isa(Space))
return "global";
assert(isa(Space));
-return "stack";
+// This case covers top-level and inlined analyses.
+return "caller";
}(getStackOrGlobalSpaceRegion(Referrer));
while (!Referrer->canPrintPretty()) {
@@ -348,12 +357,27 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
ExplodedNode *Node = Ctx.getPredecessor();
+ bool ExitingTopFrame =
+ Ctx.getPredecessor()->getLocationContext()->inTopFrame();
+
+ if (ExitingTopFrame && Node->getLocation().getTag() &&
+ Node->getLocation().getTag()->getTagDescription() ==
+ "ExprEngine : Clean Node" &&
+ Node->getFirstPred()) {
+// When finishing analysis of a top-level function, engine proactively
+// removes dead symbols thus preventing this checker from looking through
+// the output parameters. Take 1 step back, to the node where these symbols
+// and their bindings are still present
+Node = Node->getFirstPred();
+ }
+
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
class CallBack : public StoreManager::BindingsHandler {
private:
CheckerContext &Ctx;
const StackFrameContext *PoppedFrame;
+const bool TopFrame;
/// Look for stack variables referring to popped stack variables.
/// Returns true only if it found some dangling stack variables
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+return false;
+ }
+
+ if (ReferrerStackSpace->getS
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
@@ -348,12 +357,27 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, ExplodedNode *Node = Ctx.getPredecessor(); + bool ExitingTopFrame = + Ctx.getPredecessor()->getLocationContext()->inTopFrame(); + + if (ExitingTopFrame && Node->getLocation().getTag() && + Node->getLocation().getTag()->getTagDescription() == necto wrote: I've rewritten it with a direct pointer comparison: 532eea2 I think it is not worse and it won't trigger the string-typing thoughts when one reads to code. Do you agree? I can revert it if it isn't going in the right direction https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From 991f176c5545fedae2ba8b5c1b357734abe68ac7 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 11:16:10 +0200
Subject: [PATCH 1/6] [analyzer] Detect leaks on top-level via output params,
indirect globals
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.
The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 +-
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 13 +-
.../lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 2 +-
clang/test/Analysis/copy-elision.cpp | 20 +--
.../test/Analysis/incorrect-checker-names.cpp | 4 +-
clang/test/Analysis/loop-block-counts.c | 2 +-
clang/test/Analysis/stack-addr-ps.c | 6 +-
clang/test/Analysis/stack-addr-ps.cpp | 117 ++
.../Analysis/stack-capture-leak-no-arc.mm | 4 +-
clang/test/Analysis/stackaddrleak.c | 8 +-
10 files changed, 157 insertions(+), 83 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index dcf6801a73de2d..82f03e5f7d09d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -305,6 +305,14 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
return nullptr;
}
+const MemRegion *getOriginBaseRegion(const MemRegion *Referrer) {
+ Referrer = Referrer->getBaseRegion();
+ while (const auto *SymReg = dyn_cast(Referrer)) {
+Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+ }
+ return Referrer;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -313,7 +321,8 @@ std::optional printReferrer(const MemRegion
*Referrer) {
if (isa(Space))
return "global";
assert(isa(Space));
-return "stack";
+// This case covers top-level and inlined analyses.
+return "caller";
}(getStackOrGlobalSpaceRegion(Referrer));
while (!Referrer->canPrintPretty()) {
@@ -348,12 +357,27 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
ExplodedNode *Node = Ctx.getPredecessor();
+ bool ExitingTopFrame =
+ Ctx.getPredecessor()->getLocationContext()->inTopFrame();
+
+ if (ExitingTopFrame && Node->getLocation().getTag() &&
+ Node->getLocation().getTag()->getTagDescription() ==
+ "ExprEngine : Clean Node" &&
+ Node->getFirstPred()) {
+// When finishing analysis of a top-level function, engine proactively
+// removes dead symbols thus preventing this checker from looking through
+// the output parameters. Take 1 step back, to the node where these symbols
+// and their bindings are still present
+Node = Node->getFirstPred();
+ }
+
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
class CallBack : public StoreManager::BindingsHandler {
private:
CheckerContext &Ctx;
const StackFrameContext *PoppedFrame;
+const bool TopFrame;
/// Look for stack variables referring to popped stack variables.
/// Returns true only if it found some dangling stack variables
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+return false;
+ }
+
+ if (ReferrerStackSpace->getS
[clang] [analyzer][NFC] Remove a non-actionable dump (PR #106232)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/106232
This dump, if it is ever executed, is not actionable by the user and might
produce unwanted noise in the stderr.
The original intention behind this dump, to provide maximum information in an
unexpected situation, does not outweigh the potential annoyance caused to users
who might not even realize that they witnessed an unexpected situation.
>From 3daf20657c7e87f24595fbcca530c539b6d13cf6 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 16:51:59 +0200
Subject: [PATCH] [analyzer][NFC] Remove a non-actionable dump
---
clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index dcf6801a73de2d..cdbede6981aa09 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -326,7 +326,6 @@ std::optional printReferrer(const MemRegion
*Referrer) {
// warn_init_ptr_member_to_parameter_addr
return std::nullopt;
} else {
- Referrer->dump();
assert(false && "Unexpected referrer region type.");
return std::nullopt;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
@@ -297,20 +314,31 @@ std::optional printReferrer(const MemRegion
*Referrer) {
return "global";
assert(isa(Space));
return "stack";
- }(Referrer->getMemorySpace());
-
- // We should really only have VarRegions here.
- // Anything else is really surprising, and we should get notified if such
- // ever happens.
- const auto *ReferrerVar = dyn_cast(Referrer);
- if (!ReferrerVar) {
-assert(false && "We should have a VarRegion here");
-return std::nullopt; // Defensively skip this one.
+ }(getStackOrGlobalSpaceRegion(Referrer));
+
+ while (!Referrer->canPrintPretty()) {
+if (const auto *SymReg = dyn_cast(Referrer);
+SymReg && SymReg->getSymbol()->getOriginRegion()) {
+ Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+} else if (isa(Referrer)) {
+ // Skip members of a class, it is handled in CheckExprLifetime.cpp as
+ // warn_bind_ref_member_to_parameter or
+ // warn_init_ptr_member_to_parameter_addr
+ return std::nullopt;
+} else {
+ Referrer->dump();
necto wrote:
Makes sense. Here is [the fix](https://github.com/llvm/llvm-project/pull/106232)
https://github.com/llvm/llvm-project/pull/105653
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/106240
If a mutex interface is split in inheritance chain, e.g. struct mutex has
`unlock` and inherits `lock` from __mutex_base then calls m.lock() and
m.unlock() have different "this" targets: m and the __mutex_base of m, which
used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical section
and enables search in ActiveCritSections list regardless of which class the
callee is the member of.
This possibly fixes #104241
CPP-5541
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH] [analyzer] Fix false positive for mutexes inheriting
mutex_base
If a mutex interface is split in inheritance chain, e.g. struct mutex
has `unlock` and inherits `lock` from __mutex_base then calls m.lock()
and m.unlock() have different "this" targets: m and the __mutex_base of
m, which used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical
section and enables search in ActiveCritSections list regardless of
which class the callee is the member of.
This possibly fixes #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 --
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
-return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical
section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH 1/2] [analyzer] Fix false positive for mutexes inheriting
mutex_base
If a mutex interface is split in inheritance chain, e.g. struct mutex
has `unlock` and inherits `lock` from __mutex_base then calls m.lock()
and m.unlock() have different "this" targets: m and the __mutex_base of
m, which used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical
section and enables search in ActiveCritSections list regardless of
which class the callee is the member of.
This possibly fixes #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 --
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
-return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical
section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
>From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:27:50 +0200
Subject: [PATCH 2/2] Record a false negative associated with the new mutex
canonicalization
---
.../block-in-critical-section-inheritance.cpp | 14 ++
1 file changed, 14 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index c60ba2632cee25..5f6fa565f42fb9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -39,3 +39,17 @@ void no_false_positive_gh_104241() {
m.unlock();
sleep(10); // no-warning
}
+
+struct TwoMutexes {
+ std::mutex m1;
+ std::mutex m2;
+};
+
+void two_mutexes_false_negative(TwoMutexes &tm) {
+ tm.m1.lock();
+ tm.m2.unlock();
+ // Critical section is associated with tm now so tm.m1 and tm.m2 are
+ // undistinguishiable
+ sleep(10); // False-negative
+ tm.m1.unlock();
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH 1/3] [analyzer] Fix false positive for mutexes inheriting
mutex_base
If a mutex interface is split in inheritance chain, e.g. struct mutex
has `unlock` and inherits `lock` from __mutex_base then calls m.lock()
and m.unlock() have different "this" targets: m and the __mutex_base of
m, which used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical
section and enables search in ActiveCritSections list regardless of
which class the callee is the member of.
This possibly fixes #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 --
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
-return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical
section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
>From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:27:50 +0200
Subject: [PATCH 2/3] Record a false negative associated with the new mutex
canonicalization
---
.../block-in-critical-section-inheritance.cpp | 14 ++
1 file changed, 14 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index c60ba2632cee25..5f6fa565f42fb9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -39,3 +39,17 @@ void no_false_positive_gh_104241() {
m.unlock();
sleep(10); // no-warning
}
+
+struct TwoMutexes {
+ std::mutex m1;
+ std::mutex m2;
+};
+
+void two_mutexes_false_negative(TwoMutexes &tm) {
+ tm.m1.lock();
+ tm.m2.unlock();
+ // Critical section is associated with tm now so tm.m1 and tm.m2 are
+ // undistinguishiable
+ sleep(10); // False-negative
+ tm.m1.unlock();
+}
>From 744272e0f1ccd5217c0d456a7c499a9bcae84679 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:33:02 +0200
Subject: [PATCH 3/3] Fix false negative for field regions
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 9 -
.../Analysis/block-in-critical-section-inheritance.cpp | 9 +
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 52ff639c6b8022..fd8700902c1835 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -241,13 +241,20 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const
CallEvent &Call,
return std::nullopt;
}
+static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
+ while (const auto *BaseClassRegion = dyn_cast(Reg)) {
+Reg = BaseClassRegion->getSuperRegion();
+ }
+ return Reg;
+}
+
static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
[&Call, IsLock]
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
necto wrote: > Uh that FN seems really bad. Have you measured this change? Can we relax the > canonicalization to only unwrap base class regions, or only apply to classes > within the stdlib? relaxed in 744272e https://github.com/llvm/llvm-project/pull/106240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH 1/6] [analyzer] Fix false positive for mutexes inheriting
mutex_base
If a mutex interface is split in inheritance chain, e.g. struct mutex
has `unlock` and inherits `lock` from __mutex_base then calls m.lock()
and m.unlock() have different "this" targets: m and the __mutex_base of
m, which used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical
section and enables search in ActiveCritSections list regardless of
which class the callee is the member of.
This possibly fixes #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 --
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
-return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical
section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
>From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:27:50 +0200
Subject: [PATCH 2/6] Record a false negative associated with the new mutex
canonicalization
---
.../block-in-critical-section-inheritance.cpp | 14 ++
1 file changed, 14 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index c60ba2632cee25..5f6fa565f42fb9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -39,3 +39,17 @@ void no_false_positive_gh_104241() {
m.unlock();
sleep(10); // no-warning
}
+
+struct TwoMutexes {
+ std::mutex m1;
+ std::mutex m2;
+};
+
+void two_mutexes_false_negative(TwoMutexes &tm) {
+ tm.m1.lock();
+ tm.m2.unlock();
+ // Critical section is associated with tm now so tm.m1 and tm.m2 are
+ // undistinguishiable
+ sleep(10); // False-negative
+ tm.m1.unlock();
+}
>From 744272e0f1ccd5217c0d456a7c499a9bcae84679 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:33:02 +0200
Subject: [PATCH 3/6] Fix false negative for field regions
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 9 -
.../Analysis/block-in-critical-section-inheritance.cpp | 9 +
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 52ff639c6b8022..fd8700902c1835 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -241,13 +241,20 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const
CallEvent &Call,
return std::nullopt;
}
+static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
+ while (const auto *BaseClassRegion = dyn_cast(Reg)) {
+Reg = BaseClassRegion->getSuperRegion();
+ }
+ return Reg;
+}
+
static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
[&Call, IsLock]
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
necto wrote:
> Could you also add this test case?
>
> ```c++
> ```
>
> Or is it already implied by other tests?
My first test
``` C++
void no_false_positive_gh_104241() {
std::mutex m;
m.lock();
// If inheritance not handled properly, this unlock might not match the lock
// above because technically they act on different memory regions:
// __mutex_base and mutex.
m.unlock();
sleep(10); // no-warning
}
```
Was derived from your example, so I guess it is implied, but I added your
example as is with 0d95583 to make it extra clear.
https://github.com/llvm/llvm-project/pull/106240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH 1/7] [analyzer] Fix false positive for mutexes inheriting
mutex_base
If a mutex interface is split in inheritance chain, e.g. struct mutex
has `unlock` and inherits `lock` from __mutex_base then calls m.lock()
and m.unlock() have different "this" targets: m and the __mutex_base of
m, which used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical
section and enables search in ActiveCritSections list regardless of
which class the callee is the member of.
This possibly fixes #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 --
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
-return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical
section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
>From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:27:50 +0200
Subject: [PATCH 2/7] Record a false negative associated with the new mutex
canonicalization
---
.../block-in-critical-section-inheritance.cpp | 14 ++
1 file changed, 14 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index c60ba2632cee25..5f6fa565f42fb9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -39,3 +39,17 @@ void no_false_positive_gh_104241() {
m.unlock();
sleep(10); // no-warning
}
+
+struct TwoMutexes {
+ std::mutex m1;
+ std::mutex m2;
+};
+
+void two_mutexes_false_negative(TwoMutexes &tm) {
+ tm.m1.lock();
+ tm.m2.unlock();
+ // Critical section is associated with tm now so tm.m1 and tm.m2 are
+ // undistinguishiable
+ sleep(10); // False-negative
+ tm.m1.unlock();
+}
>From 744272e0f1ccd5217c0d456a7c499a9bcae84679 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:33:02 +0200
Subject: [PATCH 3/7] Fix false negative for field regions
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 9 -
.../Analysis/block-in-critical-section-inheritance.cpp | 9 +
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 52ff639c6b8022..fd8700902c1835 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -241,13 +241,20 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const
CallEvent &Call,
return std::nullopt;
}
+static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
+ while (const auto *BaseClassRegion = dyn_cast(Reg)) {
+Reg = BaseClassRegion->getSuperRegion();
+ }
+ return Reg;
+}
+
static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
[&Call, IsLock]
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH 1/8] [analyzer] Fix false positive for mutexes inheriting
mutex_base
If a mutex interface is split in inheritance chain, e.g. struct mutex
has `unlock` and inherits `lock` from __mutex_base then calls m.lock()
and m.unlock() have different "this" targets: m and the __mutex_base of
m, which used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical
section and enables search in ActiveCritSections list regardless of
which class the callee is the member of.
This possibly fixes #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 --
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
-return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical
section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
>From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:27:50 +0200
Subject: [PATCH 2/8] Record a false negative associated with the new mutex
canonicalization
---
.../block-in-critical-section-inheritance.cpp | 14 ++
1 file changed, 14 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index c60ba2632cee25..5f6fa565f42fb9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -39,3 +39,17 @@ void no_false_positive_gh_104241() {
m.unlock();
sleep(10); // no-warning
}
+
+struct TwoMutexes {
+ std::mutex m1;
+ std::mutex m2;
+};
+
+void two_mutexes_false_negative(TwoMutexes &tm) {
+ tm.m1.lock();
+ tm.m2.unlock();
+ // Critical section is associated with tm now so tm.m1 and tm.m2 are
+ // undistinguishiable
+ sleep(10); // False-negative
+ tm.m1.unlock();
+}
>From 744272e0f1ccd5217c0d456a7c499a9bcae84679 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:33:02 +0200
Subject: [PATCH 3/8] Fix false negative for field regions
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 9 -
.../Analysis/block-in-critical-section-inheritance.cpp | 9 +
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 52ff639c6b8022..fd8700902c1835 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -241,13 +241,20 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const
CallEvent &Call,
return std::nullopt;
}
+static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
+ while (const auto *BaseClassRegion = dyn_cast(Reg)) {
+Reg = BaseClassRegion->getSuperRegion();
+ }
+ return Reg;
+}
+
static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
[&Call, IsLock]
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
necto wrote: @steakhal I added a fix for multiple-inheritance fn, please take another look https://github.com/llvm/llvm-project/pull/106240 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH 1/9] [analyzer] Fix false positive for mutexes inheriting
mutex_base
If a mutex interface is split in inheritance chain, e.g. struct mutex
has `unlock` and inherits `lock` from __mutex_base then calls m.lock()
and m.unlock() have different "this" targets: m and the __mutex_base of
m, which used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical
section and enables search in ActiveCritSections list regardless of
which class the callee is the member of.
This possibly fixes #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 --
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
-return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical
section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
>From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:27:50 +0200
Subject: [PATCH 2/9] Record a false negative associated with the new mutex
canonicalization
---
.../block-in-critical-section-inheritance.cpp | 14 ++
1 file changed, 14 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index c60ba2632cee25..5f6fa565f42fb9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -39,3 +39,17 @@ void no_false_positive_gh_104241() {
m.unlock();
sleep(10); // no-warning
}
+
+struct TwoMutexes {
+ std::mutex m1;
+ std::mutex m2;
+};
+
+void two_mutexes_false_negative(TwoMutexes &tm) {
+ tm.m1.lock();
+ tm.m2.unlock();
+ // Critical section is associated with tm now so tm.m1 and tm.m2 are
+ // undistinguishiable
+ sleep(10); // False-negative
+ tm.m1.unlock();
+}
>From 744272e0f1ccd5217c0d456a7c499a9bcae84679 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:33:02 +0200
Subject: [PATCH 3/9] Fix false negative for field regions
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 9 -
.../Analysis/block-in-critical-section-inheritance.cpp | 9 +
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 52ff639c6b8022..fd8700902c1835 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -241,13 +241,20 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const
CallEvent &Call,
return std::nullopt;
}
+static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
+ while (const auto *BaseClassRegion = dyn_cast(Reg)) {
+Reg = BaseClassRegion->getSuperRegion();
+ }
+ return Reg;
+}
+
static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
[&Call, IsLock]
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH 01/10] [analyzer] Fix false positive for mutexes inheriting
mutex_base
If a mutex interface is split in inheritance chain, e.g. struct mutex
has `unlock` and inherits `lock` from __mutex_base then calls m.lock()
and m.unlock() have different "this" targets: m and the __mutex_base of
m, which used to confuse the `ActiveCritSections` list.
Taking base region canonicalizes the region used to identify a critical
section and enables search in ActiveCritSections list regardless of
which class the callee is the member of.
This possibly fixes #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 --
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
-return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical
section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
>From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:27:50 +0200
Subject: [PATCH 02/10] Record a false negative associated with the new mutex
canonicalization
---
.../block-in-critical-section-inheritance.cpp | 14 ++
1 file changed, 14 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index c60ba2632cee25..5f6fa565f42fb9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -39,3 +39,17 @@ void no_false_positive_gh_104241() {
m.unlock();
sleep(10); // no-warning
}
+
+struct TwoMutexes {
+ std::mutex m1;
+ std::mutex m2;
+};
+
+void two_mutexes_false_negative(TwoMutexes &tm) {
+ tm.m1.lock();
+ tm.m2.unlock();
+ // Critical section is associated with tm now so tm.m1 and tm.m2 are
+ // undistinguishiable
+ sleep(10); // False-negative
+ tm.m1.unlock();
+}
>From 744272e0f1ccd5217c0d456a7c499a9bcae84679 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 28 Aug 2024 08:33:02 +0200
Subject: [PATCH 03/10] Fix false negative for field regions
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 9 -
.../Analysis/block-in-critical-section-inheritance.cpp | 9 +
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 52ff639c6b8022..fd8700902c1835 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -241,13 +241,20 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const
CallEvent &Call,
return std::nullopt;
}
+static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
+ while (const auto *BaseClassRegion = dyn_cast(Reg)) {
+Reg = BaseClassRegion->getSuperRegion();
+ }
+ return Reg;
+}
+
static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
[&Call, I
[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
@@ -241,10 +241,14 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const
CallEvent &Call,
return std::nullopt;
}
-static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
- while (const auto *BaseClassRegion = dyn_cast(Reg)) {
+static const MemRegion *skipStdBaseClassRegion(const MemRegion *Reg) {
+ do {
+assert(Reg);
+const auto *BaseClassRegion = dyn_cast(Reg);
+if (!BaseClassRegion || !BaseClassRegion->getDecl()->isInStdNamespace())
necto wrote:
fixed with e94327b
https://github.com/llvm/llvm-project/pull/106240
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix nullptr dereference for symbols from pointer invalidation (PR #106568)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/106568
As reported in
https://github.com/llvm/llvm-project/pull/105648#issuecomment-2317144635 commit
08ad8dc7154bf3ab79f750e6d5fb7df597c7601a
introduced a nullptr dereference in the case when store contains a binding to a
symbol that has no origin region associated with it, such as the symbol
generated when a pointer is passed to an opaque function.
>From 71aae8d0cc96d389da95c2231b1145b7ffeb2132 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Thu, 29 Aug 2024 16:39:12 +0200
Subject: [PATCH] [analyzer] Fix nullptr dereference for symbols from pointer
invalidation
As reported in
https://github.com/llvm/llvm-project/pull/105648#issuecomment-2317144635
commit 08ad8dc7154bf3ab79f750e6d5fb7df597c7601a
introduced a nullptr dereference in the case when store contains a binding
to a symbol that has no origin region associated with it,
such as the symbol generated when a pointer is passed to an opaque function.
---
.../Checkers/StackAddrEscapeChecker.cpp | 5 -
clang/test/Analysis/stack-addr-ps.c | 19 +++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index 20232405d572d2..d3b185541729d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -308,7 +308,10 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
const MemRegion *getOriginBaseRegion(const MemRegion *Reg) {
Reg = Reg->getBaseRegion();
while (const auto *SymReg = dyn_cast(Reg)) {
-Reg = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+const auto* OriginReg = SymReg->getSymbol()->getOriginRegion();
+if (!OriginReg)
+ break;
+Reg = OriginReg->getBaseRegion();
}
return Reg;
}
diff --git a/clang/test/Analysis/stack-addr-ps.c
b/clang/test/Analysis/stack-addr-ps.c
index 138b8c16b02bde..f47529623a6f57 100644
--- a/clang/test/Analysis/stack-addr-ps.c
+++ b/clang/test/Analysis/stack-addr-ps.c
@@ -126,3 +126,22 @@ void caller_for_nested_leaking() {
int *ptr = 0;
caller_mid_for_nested_leaking(&ptr);
}
+
+// This used to crash StackAddrEscapeChecker because
+// it features a symbol conj_$1{struct c *, LC1, S763, #1}
+// that has no origin region.
+// bbi-98571
+struct a {
+ int member;
+};
+
+struct c {
+ struct a *nested_ptr;
+};
+long global_var;
+void opaque(struct c*);
+void bbi_98571_no_crash() {
+ struct c *ptr = (struct c *)global_var;
+ opaque(ptr);
+ ptr->nested_ptr->member++;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
necto wrote: > Hello, > > The following starts crashing with this patch: > > ``` > clang -cc1 -analyze -analyzer-checker=core bbi-98571.c > ``` > > Result: > > ``` > (...) > ``` Thank you for the report! Here is the fix: https://github.com/llvm/llvm-project/pull/106568 https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix nullptr dereference for symbols from pointer invalidation (PR #106568)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106568
>From 71aae8d0cc96d389da95c2231b1145b7ffeb2132 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Thu, 29 Aug 2024 16:39:12 +0200
Subject: [PATCH 1/2] [analyzer] Fix nullptr dereference for symbols from
pointer invalidation
As reported in
https://github.com/llvm/llvm-project/pull/105648#issuecomment-2317144635
commit 08ad8dc7154bf3ab79f750e6d5fb7df597c7601a
introduced a nullptr dereference in the case when store contains a binding
to a symbol that has no origin region associated with it,
such as the symbol generated when a pointer is passed to an opaque function.
---
.../Checkers/StackAddrEscapeChecker.cpp | 5 -
clang/test/Analysis/stack-addr-ps.c | 19 +++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index 20232405d572d2..d3b185541729d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -308,7 +308,10 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
const MemRegion *getOriginBaseRegion(const MemRegion *Reg) {
Reg = Reg->getBaseRegion();
while (const auto *SymReg = dyn_cast(Reg)) {
-Reg = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+const auto* OriginReg = SymReg->getSymbol()->getOriginRegion();
+if (!OriginReg)
+ break;
+Reg = OriginReg->getBaseRegion();
}
return Reg;
}
diff --git a/clang/test/Analysis/stack-addr-ps.c
b/clang/test/Analysis/stack-addr-ps.c
index 138b8c16b02bde..f47529623a6f57 100644
--- a/clang/test/Analysis/stack-addr-ps.c
+++ b/clang/test/Analysis/stack-addr-ps.c
@@ -126,3 +126,22 @@ void caller_for_nested_leaking() {
int *ptr = 0;
caller_mid_for_nested_leaking(&ptr);
}
+
+// This used to crash StackAddrEscapeChecker because
+// it features a symbol conj_$1{struct c *, LC1, S763, #1}
+// that has no origin region.
+// bbi-98571
+struct a {
+ int member;
+};
+
+struct c {
+ struct a *nested_ptr;
+};
+long global_var;
+void opaque(struct c*);
+void bbi_98571_no_crash() {
+ struct c *ptr = (struct c *)global_var;
+ opaque(ptr);
+ ptr->nested_ptr->member++;
+}
>From dcd5ade96e45c4a2e86b7886a430e12f2b6e096f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Thu, 29 Aug 2024 17:23:16 +0200
Subject: [PATCH 2/2] [NFC] Fix the code format
---
clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index d3b185541729d3..ec577c36188e6c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -308,7 +308,7 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
const MemRegion *getOriginBaseRegion(const MemRegion *Reg) {
Reg = Reg->getBaseRegion();
while (const auto *SymReg = dyn_cast(Reg)) {
-const auto* OriginReg = SymReg->getSymbol()->getOriginRegion();
+const auto *OriginReg = SymReg->getSymbol()->getOriginRegion();
if (!OriginReg)
break;
Reg = OriginReg->getBaseRegion();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for stack-addr leak on simple param ptr (PR #107003)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/107003
Assigning to a pointer parameter does not leak the stack address because it
stays within the function and is not shared with the caller.
Previous implementation reported any association of a pointer parameter with a
local address, which is too broad.
This fix enforces that the pointer to a stack variable is related by at least
one level of indirection.
CPP-5642
Fixes #106834
>From da5671efccd0ba56a0dd983b04d1f798c5c35d0d Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Mon, 2 Sep 2024 17:13:14 +0200
Subject: [PATCH] [analyzer] Fix false positive for stack-addr leak on simple
param ptr
Assigning to a pointer parameter does not leak the stack address because
it stays within the function and is not shared with the caller.
Previous implementation reported any association of a pointer parameter
with a local address, which is too broad.
This fix enforces that the pointer to a stack variable is related by at
least one level of indirection.
CPP-5642
Fixes #106834
---
.../Checkers/StackAddrEscapeChecker.cpp | 2 ++
clang/test/Analysis/stack-addr-ps.cpp | 27 +++
2 files changed, 29 insertions(+)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ec577c36188e6c..5394c2257514dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -420,6 +420,8 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return true;
}
if (isa(ReferrerMemSpace) &&
+ // Not a simple ptr (int*) but something deeper, e.g. int**
+ isa(Referrer->getBaseRegion()) &&
ReferrerStackSpace->getStackFrame() == PoppedFrame && TopFrame) {
// Output parameter of a top-level function
V.emplace_back(Referrer, Referred);
diff --git a/clang/test/Analysis/stack-addr-ps.cpp
b/clang/test/Analysis/stack-addr-ps.cpp
index 88bf6512165201..3c922dfb0ed454 100644
--- a/clang/test/Analysis/stack-addr-ps.cpp
+++ b/clang/test/Analysis/stack-addr-ps.cpp
@@ -791,3 +791,30 @@ void global_ptr_to_ptr() {
*global_pp = nullptr;
}
} // namespace leaking_via_indirect_global_invalidated
+
+namespace not_leaking_via_simple_ptr {
+void top(const char *p) {
+char tmp;
+p = &tmp;
+}
+
+extern void copy(char *output, const char *input, unsigned size);
+extern bool foo(const char *input);
+extern void bar(char *output, unsigned count);
+extern bool baz(char *output, const char *input);
+
+void repo(const char *input, char *output) {
+ char temp[64];
+ copy(temp, input, sizeof(temp));
+
+ char result[64];
+ input = temp;
+ if (foo(temp)) {
+bar(result, sizeof(result));
+input = result;
+ }
+ if (!baz(output, input)) {
+copy(output, input, sizeof(result));
+ }
+}
+} // namespace not_leaking_via_simple_ptr
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for stack-addr leak on simple param ptr (PR #107003)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/107003
>From da5671efccd0ba56a0dd983b04d1f798c5c35d0d Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Mon, 2 Sep 2024 17:13:14 +0200
Subject: [PATCH 1/2] [analyzer] Fix false positive for stack-addr leak on
simple param ptr
Assigning to a pointer parameter does not leak the stack address because
it stays within the function and is not shared with the caller.
Previous implementation reported any association of a pointer parameter
with a local address, which is too broad.
This fix enforces that the pointer to a stack variable is related by at
least one level of indirection.
CPP-5642
Fixes #106834
---
.../Checkers/StackAddrEscapeChecker.cpp | 2 ++
clang/test/Analysis/stack-addr-ps.cpp | 27 +++
2 files changed, 29 insertions(+)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ec577c36188e6c..5394c2257514dc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -420,6 +420,8 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return true;
}
if (isa(ReferrerMemSpace) &&
+ // Not a simple ptr (int*) but something deeper, e.g. int**
+ isa(Referrer->getBaseRegion()) &&
ReferrerStackSpace->getStackFrame() == PoppedFrame && TopFrame) {
// Output parameter of a top-level function
V.emplace_back(Referrer, Referred);
diff --git a/clang/test/Analysis/stack-addr-ps.cpp
b/clang/test/Analysis/stack-addr-ps.cpp
index 88bf6512165201..3c922dfb0ed454 100644
--- a/clang/test/Analysis/stack-addr-ps.cpp
+++ b/clang/test/Analysis/stack-addr-ps.cpp
@@ -791,3 +791,30 @@ void global_ptr_to_ptr() {
*global_pp = nullptr;
}
} // namespace leaking_via_indirect_global_invalidated
+
+namespace not_leaking_via_simple_ptr {
+void top(const char *p) {
+char tmp;
+p = &tmp;
+}
+
+extern void copy(char *output, const char *input, unsigned size);
+extern bool foo(const char *input);
+extern void bar(char *output, unsigned count);
+extern bool baz(char *output, const char *input);
+
+void repo(const char *input, char *output) {
+ char temp[64];
+ copy(temp, input, sizeof(temp));
+
+ char result[64];
+ input = temp;
+ if (foo(temp)) {
+bar(result, sizeof(result));
+input = result;
+ }
+ if (!baz(output, input)) {
+copy(output, input, sizeof(result));
+ }
+}
+} // namespace not_leaking_via_simple_ptr
>From 8a9a76a3d1b19d8d2434e8c6de7fe41fe8cd8756 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 3 Sep 2024 04:20:56 +0200
Subject: [PATCH 2/2] Add tests
---
clang/test/Analysis/stack-addr-ps.cpp | 36 ---
1 file changed, 32 insertions(+), 4 deletions(-)
diff --git a/clang/test/Analysis/stack-addr-ps.cpp
b/clang/test/Analysis/stack-addr-ps.cpp
index 3c922dfb0ed454..35f38fbbfbefdc 100644
--- a/clang/test/Analysis/stack-addr-ps.cpp
+++ b/clang/test/Analysis/stack-addr-ps.cpp
@@ -137,7 +137,7 @@ namespace rdar13296133 {
ConvertsToPointer obj;
return obj; // no-warning
}
-}
+} // namespace rdar13296133
void write_stack_address_to(char **q) {
char local;
@@ -793,9 +793,28 @@ void global_ptr_to_ptr() {
} // namespace leaking_via_indirect_global_invalidated
namespace not_leaking_via_simple_ptr {
-void top(const char *p) {
-char tmp;
-p = &tmp;
+void simple_ptr(const char *p) {
+ char tmp;
+ p = &tmp; // no-warning
+}
+
+void ref_ptr(const char *&p) {
+ char tmp;
+ p = &tmp; // expected-warning{{variable 'tmp' is still referred to by the
caller variable 'p'}}
+}
+
+struct S {
+ const char *p;
+};
+
+void struct_ptr(S s) {
+ char tmp;
+ s.p = &tmp; // no-warning
+}
+
+void array(const char arr[2]) {
+ char tmp;
+ arr = &tmp; // no-warning
}
extern void copy(char *output, const char *input, unsigned size);
@@ -818,3 +837,12 @@ void repo(const char *input, char *output) {
}
}
} // namespace not_leaking_via_simple_ptr
+
+namespace early_reclaim_dead_limitation {
+void foo();
+void top(char **p) {
+ char local;
+ *p = &local;
+ foo(); // no-warning FIXME: p binding is reclaimed before the function end
+}
+} // namespace early_reclaim_dead_limitation
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for stack-addr leak on simple param ptr (PR #107003)
@@ -791,3 +791,30 @@ void global_ptr_to_ptr() {
*global_pp = nullptr;
}
} // namespace leaking_via_indirect_global_invalidated
+
+namespace not_leaking_via_simple_ptr {
+void top(const char *p) {
necto wrote:
I am not sure what you are referring to by the "ref case". There is already
`param_ref_to_struct_with_ptr_top` and `param_ref_to_ptr_top`.
I added other cases in 8a9a76a3d1b1
https://github.com/llvm/llvm-project/pull/107003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for stack-addr leak on simple param ptr (PR #107003)
@@ -791,3 +791,58 @@ void global_ptr_to_ptr() {
*global_pp = nullptr;
}
} // namespace leaking_via_indirect_global_invalidated
+
+namespace not_leaking_via_simple_ptr {
+void simple_ptr(const char *p) {
+ char tmp;
+ p = &tmp; // no-warning
+}
+
+void ref_ptr(const char *&p) {
+ char tmp;
+ p = &tmp; // expected-warning{{variable 'tmp' is still referred to by the
caller variable 'p'}}
+}
+
+struct S {
+ const char *p;
necto wrote:
I do not understand what you mean.
My best interpretation
```C++
struct Ref {
char &p;
};
void struct_ref(Ref s) {
char tmp;
s = {tmp}; // no-warning
}
```
does not compile
https://github.com/llvm/llvm-project/pull/107003
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix false positive for stack-addr leak on simple param ptr (PR #107003)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/107003 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto created https://github.com/llvm/llvm-project/pull/92266
None
>From eeb24ddbf261fd7667dd05feee14637bc379d182 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:02:07 +0200
Subject: [PATCH] Fix CXXNewExpr end source location for 'new struct S'
---
clang/lib/Parse/ParseDeclCXX.cpp | 1 +
clang/test/Misc/ast-source-ranges.cpp | 5 +
2 files changed, 6 insertions(+)
create mode 100644 clang/test/Misc/ast-source-ranges.cpp
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 65ddebca49bc6..32c4e923243a9 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -1883,6 +1883,7 @@ void Parser::ParseClassSpecifier(tok::TokenKind
TagTokKind,
if (Tok.is(tok::identifier)) {
Name = Tok.getIdentifierInfo();
NameLoc = ConsumeToken();
+DS.SetRangeEnd(NameLoc);
if (Tok.is(tok::less) && getLangOpts().CPlusPlus) {
// The name was supposed to refer to a template, but didn't.
diff --git a/clang/test/Misc/ast-source-ranges.cpp
b/clang/test/Misc/ast-source-ranges.cpp
new file mode 100644
index 0..9c0ab9449a6f5
--- /dev/null
+++ b/clang/test/Misc/ast-source-ranges.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s
+
+struct Sock {};
+void leakNewFn() { new struct Sock; }
+// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/92266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/92266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
necto wrote: Disclaimer: I've never touched the Clang parser before, so the fix might not be in the right place. Please advise. https://github.com/llvm/llvm-project/pull/92266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
necto wrote: The invalid end location affects the CSA diagnostics, as you can [see on CE](https://compiler-explorer.com/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGe1wAyeAyYAHI%2BAEaYxCDSAA6oCoRODB7evnoJSY4CQSHhLFExXLaY9jkMQgRMxARpPn4ldpgOKVU1BHlhkdGxttW19RlNA53B3YW9XACUtqhexMjsHAoExF4OANRCmyYA7FYaAIL7ACImAMyHRwBuqHjom0RxENO7B5shAO6bq%2BtbQkuFne52OHFmtE4AFZeH5uLxUJw3NZrL95otMLszBceKQCJpwbMANYgC4ADgAdGTqTTaTSAJz6TiSWEE0iIji8BQgDR4gmzOCwGCIEBMVbkShoFhxOjRUKsZaqMkANgAtMrJJtkAYjJsIGsvAwidNeJh8ERiA89PxBCIxOwpDJBIoVOoOFpSLommUWhUXAx3J4Gv4A10CkVMolkgIho1SFlowwwz1iqVym1RrG9M1WgJ2rVk5NU6sOlmRh1CxGZnjiJhMFaNBDoaz3QjOEdVpsAGp4TBfaKbJVqjVanXAPUGo1vCC4QgkLE40ibDzS2XEBczXj41vTWYITBMLAxV5Mjgs0gsWJmClQqQXSRQi44sySZV7STSOEejlcnl8nekIKIpSjK9BkBQEAgWuKBjgA%2BpORJ8HQBDRNyEARGyETBDUACenC4lhzDEDhADyETaL6%2BG8FKbCCCRDC0HhrakFgEReMAbhiLQ3LwixmAsIYwDiMx%2BC1q0NyYDxHqYKoLReChVHkIIZRsrQeARMQuEeFgbJrHgl68RJxARIkmCnPxglqUY/J8AYwAKD2fYkXEjCKTawiiOIjruS6ahsl6%2BiCSgKKWPo6ncpAsyoHEFQ8aqqqGGIOEAF6YqqAAaiWMal64Jcw2XRKq8wEHE8mXKcKGqAQCJGZaWARSeOZ%2BhArhlqQgTjOGUzxlGFRtQmFSVt1TUZqWQbDGmvqjQWnUptmmbjXGJYzfkc3Vgo6JLBITYcDCpBfm2HCDiq6qatqgkTusU56rOFobia/5aLupAkmY15vR9n1fVCp7npeL4UlwULYmYXBPlCkhcPS9IXPtbI/rYf7bk9gHChASDFaVBASpBqCrmB8psJwQ6naOF1cHsFIaIDprmiQVolO5dpedIPlKH5zEBV8mlxFRO17Qd7KcCR8lY5sqBUMdw5nWOl2GsaeorqBA7mDiD3I4Sv28JeT4Uk%2B%2BsG4bcPMQj3K8hrz0kpI14XMqt4XBokgaBcexA8qZinhcLbfpwW42UB6MgEQng4zU9m%2B6QYcKMohhlEICCoF8cK4lBBMKsTJ0jsAyDIJsYMUmYtNznV1qyMzDqs7Ivluh6AUsAITBoLdJAABJihYdYMEc8moM5jBmop9fME3dPEH3yfaw3aBVPgE/9LPEerLPMchLQ8eJ3Pwe0AAkugIBCA3ZDc0wvPwvz3uHQfRFS6T526vqV0KzOo8bkuStrvdfs7sSEiUu%2BUL0jJBoPYwCLj0ghvSZUWtjY%2B05Ijc2/IdqFxgYdL%2BT1ZhGSSM4SQQA%3D%3D) https://github.com/llvm/llvm-project/pull/92266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/92266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/92266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto updated https://github.com/llvm/llvm-project/pull/92266
>From eeb24ddbf261fd7667dd05feee14637bc379d182 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:02:07 +0200
Subject: [PATCH 1/2] Fix CXXNewExpr end source location for 'new struct S'
---
clang/lib/Parse/ParseDeclCXX.cpp | 1 +
clang/test/Misc/ast-source-ranges.cpp | 5 +
2 files changed, 6 insertions(+)
create mode 100644 clang/test/Misc/ast-source-ranges.cpp
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 65ddebca49bc6..32c4e923243a9 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -1883,6 +1883,7 @@ void Parser::ParseClassSpecifier(tok::TokenKind
TagTokKind,
if (Tok.is(tok::identifier)) {
Name = Tok.getIdentifierInfo();
NameLoc = ConsumeToken();
+DS.SetRangeEnd(NameLoc);
if (Tok.is(tok::less) && getLangOpts().CPlusPlus) {
// The name was supposed to refer to a template, but didn't.
diff --git a/clang/test/Misc/ast-source-ranges.cpp
b/clang/test/Misc/ast-source-ranges.cpp
new file mode 100644
index 0..9c0ab9449a6f5
--- /dev/null
+++ b/clang/test/Misc/ast-source-ranges.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s
+
+struct Sock {};
+void leakNewFn() { new struct Sock; }
+// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
>From 28d5f458542d1fed2b3d82543c93e61a2768637c Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:53:53 +0200
Subject: [PATCH 2/2] Fix clang-tidy:make-unique
---
.../test/clang-tidy/checkers/modernize/make-unique.cpp | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
index 7934c6e93ffbd..fe512a8f3bf32 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
@@ -606,11 +606,8 @@ void invoke_template() {
template_fun(foo);
}
-void no_fix_for_invalid_new_loc() {
- // FIXME: Although the code is valid, the end location of `new struct Base`
is
- // invalid. Correct it once https://bugs.llvm.org/show_bug.cgi?id=35952 is
- // fixed.
+void fix_for_c_style_struct() {
auto T = std::unique_ptr(new struct Base);
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use std::make_unique instead
- // CHECK-FIXES: auto T = std::unique_ptr(new struct Base);
+ // CHECK-FIXES: auto T = std::make_unique();
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto updated https://github.com/llvm/llvm-project/pull/92266
>From eeb24ddbf261fd7667dd05feee14637bc379d182 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:02:07 +0200
Subject: [PATCH 1/3] Fix CXXNewExpr end source location for 'new struct S'
---
clang/lib/Parse/ParseDeclCXX.cpp | 1 +
clang/test/Misc/ast-source-ranges.cpp | 5 +
2 files changed, 6 insertions(+)
create mode 100644 clang/test/Misc/ast-source-ranges.cpp
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 65ddebca49bc6..32c4e923243a9 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -1883,6 +1883,7 @@ void Parser::ParseClassSpecifier(tok::TokenKind
TagTokKind,
if (Tok.is(tok::identifier)) {
Name = Tok.getIdentifierInfo();
NameLoc = ConsumeToken();
+DS.SetRangeEnd(NameLoc);
if (Tok.is(tok::less) && getLangOpts().CPlusPlus) {
// The name was supposed to refer to a template, but didn't.
diff --git a/clang/test/Misc/ast-source-ranges.cpp
b/clang/test/Misc/ast-source-ranges.cpp
new file mode 100644
index 0..9c0ab9449a6f5
--- /dev/null
+++ b/clang/test/Misc/ast-source-ranges.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s
+
+struct Sock {};
+void leakNewFn() { new struct Sock; }
+// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
>From 28d5f458542d1fed2b3d82543c93e61a2768637c Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:53:53 +0200
Subject: [PATCH 2/3] Fix clang-tidy:make-unique
---
.../test/clang-tidy/checkers/modernize/make-unique.cpp | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
index 7934c6e93ffbd..fe512a8f3bf32 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
@@ -606,11 +606,8 @@ void invoke_template() {
template_fun(foo);
}
-void no_fix_for_invalid_new_loc() {
- // FIXME: Although the code is valid, the end location of `new struct Base`
is
- // invalid. Correct it once https://bugs.llvm.org/show_bug.cgi?id=35952 is
- // fixed.
+void fix_for_c_style_struct() {
auto T = std::unique_ptr(new struct Base);
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use std::make_unique instead
- // CHECK-FIXES: auto T = std::unique_ptr(new struct Base);
+ // CHECK-FIXES: auto T = std::make_unique();
}
>From 21077adfefd5a255735cb7f6f0bb690a1e880e62 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Thu, 16 May 2024 09:27:25 +0200
Subject: [PATCH 3/3] Move the AST dump test to the conventional file
---
clang/test/AST/ast-dump-expr.cpp | 5 +
clang/test/Misc/ast-source-ranges.cpp | 5 -
2 files changed, 5 insertions(+), 5 deletions(-)
delete mode 100644 clang/test/Misc/ast-source-ranges.cpp
diff --git a/clang/test/AST/ast-dump-expr.cpp b/clang/test/AST/ast-dump-expr.cpp
index 69e65e22d61d0..9b29ab1bbb228 100644
--- a/clang/test/AST/ast-dump-expr.cpp
+++ b/clang/test/AST/ast-dump-expr.cpp
@@ -583,3 +583,8 @@ void NonADLCall3() {
f(x);
}
} // namespace test_adl_call_three
+
+struct Sock {};
+void leakNewFn() { new struct Sock; }
+// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
+
diff --git a/clang/test/Misc/ast-source-ranges.cpp
b/clang/test/Misc/ast-source-ranges.cpp
deleted file mode 100644
index 9c0ab9449a6f5..0
--- a/clang/test/Misc/ast-source-ranges.cpp
+++ /dev/null
@@ -1,5 +0,0 @@
-// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s
-
-struct Sock {};
-void leakNewFn() { new struct Sock; }
-// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
@@ -0,0 +1,5 @@ +// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s necto wrote: thanks for the pointer, moved! https://github.com/llvm/llvm-project/pull/92266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
necto wrote: > `clang-tidy/checkers/modernize/make-unique.cpp` is failing for whatever > reason. Otherwise LGTM. Indeed, turns out this PR fixes #35300 https://github.com/llvm/llvm-project/pull/92266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto updated https://github.com/llvm/llvm-project/pull/92266
>From eeb24ddbf261fd7667dd05feee14637bc379d182 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:02:07 +0200
Subject: [PATCH 1/3] Fix CXXNewExpr end source location for 'new struct S'
---
clang/lib/Parse/ParseDeclCXX.cpp | 1 +
clang/test/Misc/ast-source-ranges.cpp | 5 +
2 files changed, 6 insertions(+)
create mode 100644 clang/test/Misc/ast-source-ranges.cpp
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 65ddebca49bc6..32c4e923243a9 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -1883,6 +1883,7 @@ void Parser::ParseClassSpecifier(tok::TokenKind
TagTokKind,
if (Tok.is(tok::identifier)) {
Name = Tok.getIdentifierInfo();
NameLoc = ConsumeToken();
+DS.SetRangeEnd(NameLoc);
if (Tok.is(tok::less) && getLangOpts().CPlusPlus) {
// The name was supposed to refer to a template, but didn't.
diff --git a/clang/test/Misc/ast-source-ranges.cpp
b/clang/test/Misc/ast-source-ranges.cpp
new file mode 100644
index 0..9c0ab9449a6f5
--- /dev/null
+++ b/clang/test/Misc/ast-source-ranges.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s
+
+struct Sock {};
+void leakNewFn() { new struct Sock; }
+// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
>From 28d5f458542d1fed2b3d82543c93e61a2768637c Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:53:53 +0200
Subject: [PATCH 2/3] Fix clang-tidy:make-unique
---
.../test/clang-tidy/checkers/modernize/make-unique.cpp | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
index 7934c6e93ffbd..fe512a8f3bf32 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
@@ -606,11 +606,8 @@ void invoke_template() {
template_fun(foo);
}
-void no_fix_for_invalid_new_loc() {
- // FIXME: Although the code is valid, the end location of `new struct Base`
is
- // invalid. Correct it once https://bugs.llvm.org/show_bug.cgi?id=35952 is
- // fixed.
+void fix_for_c_style_struct() {
auto T = std::unique_ptr(new struct Base);
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use std::make_unique instead
- // CHECK-FIXES: auto T = std::unique_ptr(new struct Base);
+ // CHECK-FIXES: auto T = std::make_unique();
}
>From 21077adfefd5a255735cb7f6f0bb690a1e880e62 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Thu, 16 May 2024 09:27:25 +0200
Subject: [PATCH 3/3] Move the AST dump test to the conventional file
---
clang/test/AST/ast-dump-expr.cpp | 5 +
clang/test/Misc/ast-source-ranges.cpp | 5 -
2 files changed, 5 insertions(+), 5 deletions(-)
delete mode 100644 clang/test/Misc/ast-source-ranges.cpp
diff --git a/clang/test/AST/ast-dump-expr.cpp b/clang/test/AST/ast-dump-expr.cpp
index 69e65e22d61d0..9b29ab1bbb228 100644
--- a/clang/test/AST/ast-dump-expr.cpp
+++ b/clang/test/AST/ast-dump-expr.cpp
@@ -583,3 +583,8 @@ void NonADLCall3() {
f(x);
}
} // namespace test_adl_call_three
+
+struct Sock {};
+void leakNewFn() { new struct Sock; }
+// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
+
diff --git a/clang/test/Misc/ast-source-ranges.cpp
b/clang/test/Misc/ast-source-ranges.cpp
deleted file mode 100644
index 9c0ab9449a6f5..0
--- a/clang/test/Misc/ast-source-ranges.cpp
+++ /dev/null
@@ -1,5 +0,0 @@
-// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s
-
-struct Sock {};
-void leakNewFn() { new struct Sock; }
-// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
https://github.com/necto updated https://github.com/llvm/llvm-project/pull/92266
>From eeb24ddbf261fd7667dd05feee14637bc379d182 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:02:07 +0200
Subject: [PATCH 1/4] Fix CXXNewExpr end source location for 'new struct S'
---
clang/lib/Parse/ParseDeclCXX.cpp | 1 +
clang/test/Misc/ast-source-ranges.cpp | 5 +
2 files changed, 6 insertions(+)
create mode 100644 clang/test/Misc/ast-source-ranges.cpp
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 65ddebca49bc6..32c4e923243a9 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -1883,6 +1883,7 @@ void Parser::ParseClassSpecifier(tok::TokenKind
TagTokKind,
if (Tok.is(tok::identifier)) {
Name = Tok.getIdentifierInfo();
NameLoc = ConsumeToken();
+DS.SetRangeEnd(NameLoc);
if (Tok.is(tok::less) && getLangOpts().CPlusPlus) {
// The name was supposed to refer to a template, but didn't.
diff --git a/clang/test/Misc/ast-source-ranges.cpp
b/clang/test/Misc/ast-source-ranges.cpp
new file mode 100644
index 0..9c0ab9449a6f5
--- /dev/null
+++ b/clang/test/Misc/ast-source-ranges.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s
+
+struct Sock {};
+void leakNewFn() { new struct Sock; }
+// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
>From 28d5f458542d1fed2b3d82543c93e61a2768637c Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Wed, 15 May 2024 16:53:53 +0200
Subject: [PATCH 2/4] Fix clang-tidy:make-unique
---
.../test/clang-tidy/checkers/modernize/make-unique.cpp | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
index 7934c6e93ffbd..fe512a8f3bf32 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-unique.cpp
@@ -606,11 +606,8 @@ void invoke_template() {
template_fun(foo);
}
-void no_fix_for_invalid_new_loc() {
- // FIXME: Although the code is valid, the end location of `new struct Base`
is
- // invalid. Correct it once https://bugs.llvm.org/show_bug.cgi?id=35952 is
- // fixed.
+void fix_for_c_style_struct() {
auto T = std::unique_ptr(new struct Base);
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use std::make_unique instead
- // CHECK-FIXES: auto T = std::unique_ptr(new struct Base);
+ // CHECK-FIXES: auto T = std::make_unique();
}
>From 21077adfefd5a255735cb7f6f0bb690a1e880e62 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Thu, 16 May 2024 09:27:25 +0200
Subject: [PATCH 3/4] Move the AST dump test to the conventional file
---
clang/test/AST/ast-dump-expr.cpp | 5 +
clang/test/Misc/ast-source-ranges.cpp | 5 -
2 files changed, 5 insertions(+), 5 deletions(-)
delete mode 100644 clang/test/Misc/ast-source-ranges.cpp
diff --git a/clang/test/AST/ast-dump-expr.cpp b/clang/test/AST/ast-dump-expr.cpp
index 69e65e22d61d0..9b29ab1bbb228 100644
--- a/clang/test/AST/ast-dump-expr.cpp
+++ b/clang/test/AST/ast-dump-expr.cpp
@@ -583,3 +583,8 @@ void NonADLCall3() {
f(x);
}
} // namespace test_adl_call_three
+
+struct Sock {};
+void leakNewFn() { new struct Sock; }
+// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
+
diff --git a/clang/test/Misc/ast-source-ranges.cpp
b/clang/test/Misc/ast-source-ranges.cpp
deleted file mode 100644
index 9c0ab9449a6f5..0
--- a/clang/test/Misc/ast-source-ranges.cpp
+++ /dev/null
@@ -1,5 +0,0 @@
-// RUN: %clang_cc1 -ast-dump %s 2>&1 | FileCheck %s
-
-struct Sock {};
-void leakNewFn() { new struct Sock; }
-// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
>From 4eeb467d612ccf184399df95172311e6f68ea60d Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Thu, 16 May 2024 10:33:12 +0200
Subject: [PATCH 4/4] wrap test into a GH35300 namespace
---
clang/test/AST/ast-dump-expr.cpp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/clang/test/AST/ast-dump-expr.cpp b/clang/test/AST/ast-dump-expr.cpp
index e848cee2637ed..604868103dab8 100644
--- a/clang/test/AST/ast-dump-expr.cpp
+++ b/clang/test/AST/ast-dump-expr.cpp
@@ -584,7 +584,9 @@ void NonADLCall3() {
}
} // namespace test_adl_call_three
+namespace GH35300 {
struct Sock {};
void leakNewFn() { new struct Sock; }
// CHECK: CXXNewExpr {{.*}} 'struct Sock *'
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang] Fix CXXNewExpr end source location for 'new struct S' (PR #92266)
@@ -583,3 +583,8 @@ void NonADLCall3() {
f(x);
}
} // namespace test_adl_call_three
+
necto wrote:
Done
https://github.com/llvm/llvm-project/pull/92266
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals (PR #105648)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/105648
Note, I prepared this PR to be rebased and merged with three commits that are
self-sufficient and build on each other.
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function with
an output parameter is analyzed as top-level or as a callee. I took special
care to make sure the reports point to the same primary location and, in many
cases, feature the same primary message. That is the motivation to modify
Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is assigned a
local address, invalidated, and then reset, I rely on the fact that the
invalidation symbol will be a DerivedSymbol of a ConjuredSymbol that refers to
the same memory region.
The checker still has a false negative for non-trivial escaping via a returned
value. It requires an active
https://sonarsource.atlassian.net/browse/CPP-4734
>From 9219884befe8e3c7b4ac8bcbfce9ecf9063713ec Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH 1/3] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +3
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals (PR #105648)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals (PR #105648)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals (PR #105648)
https://github.com/necto converted_to_draft https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals (PR #105648)
necto wrote: It turns out that you cannot rebase&merge in llvm-project repo, so I'll create two more PRs stacked PRs - one per commit https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH 1/3] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
}
-const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa(Space))
-return "static";
- if (isa(Space))
-return "global";
- assert(isa(Space));
- return "stack";
-}(Referrer->getMemorySpace());
-
-// We should really only have VarRegions here.
-// Anything else is reall
[clang] [analyzer] [NFC] Add tests for and refactor StackAddrEscapeChecker (PR #105652)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/105652
These tests and refactoring are preparatory for the upcoming changes: detection
of the indirect leak via global variables and output parameters.
CPP-4734
---
This is 1 of three commits constituting
https://github.com/llvm/llvm-project/pull/105648
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
}
-const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa(
[clang] [analyzer] [NFC] Add tests for and refactor StackAddrEscapeChecker (PR #105652)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/105652 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leak of a stack address through output arguments (PR #105653)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/105653
At this point, only functions called from other functions (i.e., not
top-level) are covered. Top-level functions have a different exit
sequence and will be handled by a subsequent change.
CPP-4734
---
This is the second of three commits constituting
https://github.com/llvm/llvm-project/pull/105648
it must not be merged before https://github.com/llvm/llvm-project/pull/105652
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH 1/2] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
[clang] [analyzer] Detect leak of a stack address through output arguments (PR #105653)
https://github.com/necto converted_to_draft https://github.com/llvm/llvm-project/pull/105653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals (PR #105648)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
necto wrote: > It turns out that you cannot rebase&merge in llvm-project repo, so I'll > create two more PRs stacked PRs - one per commit Here are the two PRs that promote the first two commits of this branch: https://github.com/llvm/llvm-project/pull/105652 and https://github.com/llvm/llvm-project/pull/105653 https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
@@ -297,20 +314,29 @@ std::optional printReferrer(const MemRegion
*Referrer) {
return "global";
assert(isa(Space));
return "stack";
- }(Referrer->getMemorySpace());
-
- // We should really only have VarRegions here.
- // Anything else is really surprising, and we should get notified if such
- // ever happens.
- const auto *ReferrerVar = dyn_cast(Referrer);
- if (!ReferrerVar) {
-assert(false && "We should have a VarRegion here");
-return std::nullopt; // Defensively skip this one.
+ }(getStackOrGlobalSpaceRegion(Referrer));
+
+ while (!Referrer->canPrintPretty()) {
+if (const auto *SymReg = dyn_cast(Referrer)) {
+ Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+} else if (isa(Referrer)) {
+ // Skip members of a class, it is handled by
+ // warn_bind_ref_member_to_parameter_addr
necto wrote:
I was referring to this diagnostic by Sema:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CheckExprLifetime.cpp#L1115
Should I better refer to `sema::checkExprLifetime`?
https://github.com/llvm/llvm-project/pull/105653
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105653
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH 1/4] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
}
-const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa(Space))
-return "static";
- if (isa(Space))
-return "global";
- assert(isa(Space));
- return "stack";
-}(Referrer->getMemorySpace());
-
-// We should really only have VarRegions here.
-// Anything else is reall
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
@@ -297,20 +314,29 @@ std::optional printReferrer(const MemRegion
*Referrer) {
return "global";
assert(isa(Space));
return "stack";
- }(Referrer->getMemorySpace());
-
- // We should really only have VarRegions here.
- // Anything else is really surprising, and we should get notified if such
- // ever happens.
- const auto *ReferrerVar = dyn_cast(Referrer);
- if (!ReferrerVar) {
-assert(false && "We should have a VarRegion here");
-return std::nullopt; // Defensively skip this one.
+ }(getStackOrGlobalSpaceRegion(Referrer));
+
+ while (!Referrer->canPrintPretty()) {
+if (const auto *SymReg = dyn_cast(Referrer)) {
+ Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
necto wrote:
> Consider adding a testcase which shows this limitation.
Added the test case: f223714
> Be careful with getOriginRegion(), it will return null if the symbol is not a
> SymbolRegionValue or a SymbolDerived (e.g. a SymbolConjured returned by an
> opaque function call)!
Added a defensive check: 4afbb63
However, I suspect it would never trigger, because I think a region containing
a conjured symbol with no origin region associated would also have no known
memory space so it would not reach this point because I discard such region
[here](https://github.com/necto/llvm-project/blob/az/CPP-4734-stack-leak-output-arg/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp#L302)
https://github.com/llvm/llvm-project/pull/105653
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH 1/5] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
}
-const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa(Space))
-return "static";
- if (isa(Space))
-return "global";
- assert(isa(Space));
- return "stack";
-}(Referrer->getMemorySpace());
-
-// We should really only have VarRegions here.
-// Anything else is reall
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
@@ -297,20 +314,29 @@ std::optional printReferrer(const MemRegion
*Referrer) {
return "global";
assert(isa(Space));
return "stack";
- }(Referrer->getMemorySpace());
-
- // We should really only have VarRegions here.
- // Anything else is really surprising, and we should get notified if such
- // ever happens.
- const auto *ReferrerVar = dyn_cast(Referrer);
- if (!ReferrerVar) {
-assert(false && "We should have a VarRegion here");
-return std::nullopt; // Defensively skip this one.
+ }(getStackOrGlobalSpaceRegion(Referrer));
+
+ while (!Referrer->canPrintPretty()) {
+if (const auto *SymReg = dyn_cast(Referrer)) {
+ Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+} else if (isa(Referrer)) {
+ // Skip members of a class, it is handled by
+ // warn_bind_ref_member_to_parameter_addr
necto wrote:
Indeed the diagnostic name is off (and in fact both of diags you mentioned
fit). Corrected clarified in 47b5deecc and 4081a03fb
https://github.com/llvm/llvm-project/pull/105653
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH 1/6] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
}
-const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa(Space))
-return "static";
- if (isa(Space))
-return "global";
- assert(isa(Space));
- return "stack";
-}(Referrer->getMemorySpace());
-
-// We should really only have VarRegions here.
-// Anything else is reall
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105653
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH 1/6] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
}
-const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa(Space))
-return "static";
- if (isa(Space))
-return "global";
- assert(isa(Space));
- return "stack";
-}(Referrer->getMemorySpace());
-
-// We should really only have VarRegions here.
-// Anything else is reall
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
@@ -297,20 +314,29 @@ std::optional printReferrer(const MemRegion
*Referrer) {
return "global";
assert(isa(Space));
return "stack";
- }(Referrer->getMemorySpace());
-
- // We should really only have VarRegions here.
- // Anything else is really surprising, and we should get notified if such
- // ever happens.
- const auto *ReferrerVar = dyn_cast(Referrer);
- if (!ReferrerVar) {
-assert(false && "We should have a VarRegion here");
-return std::nullopt; // Defensively skip this one.
+ }(getStackOrGlobalSpaceRegion(Referrer));
+
+ while (!Referrer->canPrintPretty()) {
+if (const auto *SymReg = dyn_cast(Referrer)) {
+ Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+} else if (isa(Referrer)) {
+ // Skip members of a class, it is handled by
+ // warn_bind_ref_member_to_parameter_addr
necto wrote:
Oups, indeed, I pushed them to the wrong branch. thank you for warning me!
they are on this branch too now:
24bd8bffc6bb clarify reference further
9b7e3d6a80a8 Clarify reference
https://github.com/llvm/llvm-project/pull/105653
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105653
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH 1/7] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
}
-const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa(Space))
-return "static";
- if (isa(Space))
-return "global";
- assert(isa(Space));
- return "stack";
-}(Referrer->getMemorySpace());
-
-// We should really only have VarRegions here.
-// Anything else is reall
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
@@ -161,3 +164,619 @@ C make1() {
void test_copy_elision() {
C c1 = make1();
}
+
+namespace leaking_via_direct_pointer {
+void* returned_direct_pointer_top() {
+ int local = 42;
+ int* p = &local;
+ return p; // expected-warning{{associated with local variable 'local'
returned}}
+}
+
+int* returned_direct_pointer_callee() {
+ int local = 42;
+ int* p = &local;
+ return p; // expected-warning{{associated with local variable 'local'
returned}}
+}
+
+void returned_direct_pointer_caller() {
+ int* loc_ptr = nullptr;
+ loc_ptr = returned_direct_pointer_callee();
+ (void)loc_ptr;
+}
+
+void* global_ptr;
+
+void global_direct_pointer() {
+ int local = 42;
+ global_ptr = &local;
+} // expected-warning{{local variable 'local' is still referred to by the
global variable 'global_ptr'}}
+
+void static_direct_pointer_top() {
+ int local = 42;
+ static int* p = &local;
+ (void)p;
+} // expected-warning{{local variable 'local' is still referred to by the
static variable 'p'}}
+
+void static_direct_pointer_callee() {
+ int local = 42;
+ static int* p = &local;
+ (void)p; // expected-warning{{local variable 'local' is still referred to by
the static variable 'p'}}
+}
+
+void static_direct_pointer_caller() {
+ static_direct_pointer_callee();
+}
+
+void lambda_to_global_direct_pointer() {
+ auto lambda = [&] {
+int local = 42;
+global_ptr = &local; // expected-warning{{local variable 'local' is still
referred to by the global variable 'global_ptr'}}
+ };
+ lambda();
+}
+
+void lambda_to_context_direct_pointer() {
+ int *p = nullptr;
+ auto lambda = [&] {
+int local = 42;
+p = &local; // expected-warning{{local variable 'local' is still referred
to by the stack variable 'p'}}
+ };
+ lambda();
+ (void)p;
+}
+
+template
+class MyFunction {
+ Callable* fptr;
+ public:
+ MyFunction(Callable* callable) :fptr(callable) {}
+};
+
+void* lambda_to_context_direct_pointer_uncalled() {
+ int *p = nullptr;
+ auto lambda = [&] {
+int local = 42;
+p = &local; // no-warning: analyzed only as top-level, ignored explicitly
by the checker
+ };
+ return new MyFunction(&lambda);
+}
+
+void lambda_to_context_direct_pointer_lifetime_extended() {
+ int *p = nullptr;
+ auto lambda = [&] {
+int&& local = 42;
+p = &local; // expected-warning{{'int' lifetime extended by local variable
'local' is still referred to by the stack variable 'p'}}
+ };
+ lambda();
+ (void)p;
+}
+
+template
+void lambda_param_capture_direct_pointer_callee(Callback& callee) {
+ int local = 42;
+ callee(local); // expected-warning{{'local' is still referred to by the
stack variable 'p'}}
+}
+
+void lambda_param_capture_direct_pointer_caller() {
+ int* p = nullptr;
+ auto capt = [&p](int& param) {
+p = ¶m;
+ };
+ lambda_param_capture_direct_pointer_callee(capt);
+}
+} // namespace leaking_via_direct_pointer
+
+namespace leaking_via_ptr_to_ptr {
+void** returned_ptr_to_ptr_top() {
+ int local = 42;
+ int* p = &local;
+ void** pp = (void**)&p;
+ return pp; // expected-warning{{associated with local variable 'p' returned}}
+}
+
+void** global_pp;
+
+void global_ptr_local_to_ptr() {
+ int local = 42;
+ int* p = &local;
+ global_pp = (void**)&p;
+} // expected-warning{{local variable 'p' is still referred to by the global
variable 'global_pp'}}
+
+void global_ptr_to_ptr() {
+ int local = 42;
+ *global_pp = &local; // no-warning FIXME
+}
+
+void *** global_ppp;
+
+void global_ptr_to_ptr_to_ptr() {
+ int local = 42;
+ **global_ppp = &local; // no-warning FIXME
+}
+
+void** get_some_pp();
+
+void static_ptr_to_ptr() {
+ int local = 42;
+ static void** pp = get_some_pp();
+ *pp = &local;
+} // no-warning False Negative, requires relating multiple bindings to cross
the invented pointer.
+
+void param_ptr_to_ptr_top(void** pp) {
+ int local = 42;
+ *pp = &local; // no-warning FIXME
+}
+
+void param_ptr_to_ptr_callee(void** pp) {
+ int local = 42;
+ *pp = &local; // expected-warning{{local variable 'local' is still referred
to by the stack variable 'p'}}
+}
+
+void param_ptr_to_ptr_caller() {
+ void* p = nullptr;
+ param_ptr_to_ptr_callee((void**)&p);
+}
+
+void param_ptr_to_ptr_to_ptr_top(void*** ppp) {
+ int local = 42;
+ **ppp = &local; // no-warning FIXME
+}
+
+void param_ptr_to_ptr_to_ptr_callee(void*** ppp) {
+ int local = 42;
+ **ppp = &local; // expected-warning{{local variable 'local' is still
referred to by the stack variable 'pp'}}
+}
+
+void param_ptr_to_ptr_to_ptr_caller(void** pp) {
+ param_ptr_to_ptr_to_ptr_callee(&pp);
+}
+
+void lambda_to_context_ptr_to_ptr(int **pp) {
+ auto lambda = [&] {
+int local = 42;
+*pp = &local; // expected-warning{{local variable 'local' is still
referred to by the stack variable 'pp'}}
+ };
+ lambda();
+ (void)*pp;
+}
+
+void param_ptr_to_ptr_fptr(int **pp) {
+ int local = 42;
+ *pp = &local; // expected-warning{{local variable 'local' is still referred
to by the stack variable 'p'}}
+}
+
+vo
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/105653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto created
https://github.com/llvm/llvm-project/pull/106048
Make sure code respects the GNU-extension __attribute__((returns_nonnull)).
Extend the NullabilityChecker to check that a function returns_nonnull does not
return a nullptr.
CPP-4741
>From 2618fc762a4913eaf3dd3df5bac9ddbfed27affe Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 17:31:11 +0200
Subject: [PATCH] [analyzer] Report violations of the "returns_nonnull"
attribute
Make sure code respects the GNU-extension
__attribute__((returns_nonnull)).
Extend the NullabilityChecker to check that a function returns_nonnull
does not return a nullptr.
CPP-4741
---
.../Checkers/NullabilityChecker.cpp | 8
clang/test/Analysis/nullability.c | 43 ++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 60934e51febe84..2035d50eea4c2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
NullConstraint Nullness = getNullConstraint(*RetSVal, State);
Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType);
+ if (const auto *FunDecl = C.getLocationContext()->getDecl();
+ FunDecl && FunDecl->getAttr() &&
+ (RequiredNullability == Nullability::Unspecified ||
+ RequiredNullability == Nullability::Nullable)) {
+// If a function is marked with the returns_nonnull attribute,
+// the return value must be non-null.
+RequiredNullability = Nullability::Nonnull;
+ }
// If the returned value is null but the type of the expression
// generating it is nonnull then we will suppress the diagnostic.
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index fbc03c864ad83f..9ddb9c8d2dc34f 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability
-Wno-deprecated-non-prototype -verify %s
+// RUN: %clang_analyze_cc1 -fblocks
-analyzer-checker=core,nullability,debug.ExprInspection
-Wno-deprecated-non-prototype -verify %s
+
+void clang_analyzer_warnIfReached();
void it_takes_two(int a, int b);
void function_pointer_arity_mismatch() {
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
+ }
+ // Regardless of the potential state split above, x cannot be nullptr
+ // according to the produce_nonnull_ptr annotation.
+ return x;
+ // False positive: expected-warning@-1 {{Null returned from a function that
is expected to return a non-null value}}
+}
+
+__attribute__((returns_nonnull)) int *passthrough(int *p) {
+ return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto edited https://github.com/llvm/llvm-project/pull/106048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Add tests for and refactor StackAddrEscapeChecker 1/3 (PR #105652)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105652
>From db68dcfba96bfbf9367ba4159b6bf179c8c56f4f Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:26:38 +0200
Subject: [PATCH] [analyzer] [NFC] Add tests for and refactor
StackAddrEscapeChecker
These tests and refactoring are preparatory for the upcoming changes:
detection of the indirect leak via global variables and output parameters.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 71 ++-
clang/test/Analysis/stack-addr-ps.c | 31 +
clang/test/Analysis/stack-addr-ps.cpp | 596 ++
3 files changed, 665 insertions(+), 33 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index ea09c43cc5ce90..2bd4ca4528de8b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const
ReturnStmt *RS,
EmitStackError(C, R, RetE);
}
+std::optional printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+if (isa(Space))
+ return "static";
+if (isa(Space))
+ return "global";
+assert(isa(Space));
+return "stack";
+ }(Referrer->getMemorySpace());
+
+ // We should really only have VarRegions here.
+ // Anything else is really surprising, and we should get notified if such
+ // ever happens.
+ const auto *ReferrerVar = dyn_cast(Referrer);
+ if (!ReferrerVar) {
+assert(false && "We should have a VarRegion here");
+return std::nullopt; // Defensively skip this one.
+ }
+ const std::string ReferrerVarName =
+ ReferrerVar->getDecl()->getDeclName().getAsString();
+
+ return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+}
+
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
- ProgramStateRef State = Ctx.getState();
+ ExplodedNode *Node = Ctx.getPredecessor();
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
@@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
- const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
- const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
-
- if (ReferrerMemSpace && ReferredMemSpace) {
-if (ReferredFrame == PoppedFrame &&
-ReferrerFrame->isParentOf(PoppedFrame)) {
- V.emplace_back(Referrer, Referred);
- return true;
-}
+ if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
+ ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+V.emplace_back(Referrer, Referred);
+return true;
}
return false;
}
@@ -352,6 +372,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
};
CallBack Cb(Ctx);
+ ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);
@@ -359,7 +380,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
// Generate an error node.
- ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
+ ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;
@@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
// Generate a report for this bug.
const StringRef CommonSuffix =
-"upon returning to the caller. This will be a dangling reference";
+" upon returning to the caller. This will be a dangling reference";
SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());
if (isa(Referrer)) {
- Out << " is still referred to by a temporary object on the stack "
+ Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique(*BT_stackleak, Out.str(),
N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
return;
}
-const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa(Space))
-return "static";
- if (isa(Space))
-return "global";
- assert(isa(Space));
- return "stack";
-}(Referrer->getMemorySpace());
-
-// We should really only have VarRegions here.
-// Anything else is really su
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105653
>From 902e1d63b436db3ca9e21b022e821a0182bf992c Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:53:25 +0200
Subject: [PATCH] [analyzer] Detect leak of a stack address through output
arguments
At this point only functions called from other functions (i.e., not
top-level) are covered. Top-level functions have a different exit
sequence, and will be handled by a subsequent change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 ++-
clang/test/Analysis/stack-addr-ps.cpp | 31 +++--
2 files changed, 75 insertions(+), 20 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index 2bd4ca4528de8b..dcf6801a73de2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,6 +288,23 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt
*RS,
EmitStackError(C, R, RetE);
}
+static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {
+ assert(R);
+ if (const auto *MemSpace = R->getMemorySpace()) {
+if (const auto *SSR = MemSpace->getAs())
+ return SSR;
+if (const auto *GSR = MemSpace->getAs())
+ return GSR;
+ }
+ // If R describes a lambda capture, it will be a symbolic region
+ // referring to a field region of another symbolic region.
+ if (const auto *SymReg = R->getBaseRegion()->getAs()) {
+if (const auto *OriginReg = SymReg->getSymbol()->getOriginRegion())
+ return getStackOrGlobalSpaceRegion(OriginReg);
+ }
+ return nullptr;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -297,20 +314,31 @@ std::optional printReferrer(const MemRegion
*Referrer) {
return "global";
assert(isa(Space));
return "stack";
- }(Referrer->getMemorySpace());
-
- // We should really only have VarRegions here.
- // Anything else is really surprising, and we should get notified if such
- // ever happens.
- const auto *ReferrerVar = dyn_cast(Referrer);
- if (!ReferrerVar) {
-assert(false && "We should have a VarRegion here");
-return std::nullopt; // Defensively skip this one.
+ }(getStackOrGlobalSpaceRegion(Referrer));
+
+ while (!Referrer->canPrintPretty()) {
+if (const auto *SymReg = dyn_cast(Referrer);
+SymReg && SymReg->getSymbol()->getOriginRegion()) {
+ Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+} else if (isa(Referrer)) {
+ // Skip members of a class, it is handled in CheckExprLifetime.cpp as
+ // warn_bind_ref_member_to_parameter or
+ // warn_init_ptr_member_to_parameter_addr
+ return std::nullopt;
+} else {
+ Referrer->dump();
+ assert(false && "Unexpected referrer region type.");
+ return std::nullopt;
+}
}
- const std::string ReferrerVarName =
- ReferrerVar->getDecl()->getDeclName().getAsString();
+ assert(Referrer);
+ assert(Referrer->canPrintPretty());
- return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+ std::string buf;
+ llvm::raw_string_ostream os(buf);
+ os << ReferrerMemorySpace << " variable ";
+ Referrer->printPretty(os);
+ return buf;
}
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
@@ -332,16 +360,20 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
/// referred by an other stack variable from different stack frame.
bool checkForDanglingStackVariable(const MemRegion *Referrer,
const MemRegion *Referred) {
- const auto *ReferrerMemSpace =
- Referrer->getMemorySpace()->getAs();
+ const auto *ReferrerMemSpace = getStackOrGlobalSpaceRegion(Referrer);
const auto *ReferredMemSpace =
Referred->getMemorySpace()->getAs();
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
+ const auto *ReferrerStackSpace =
+ ReferrerMemSpace->getAs();
+ if (!ReferrerStackSpace)
+return false;
+
if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
V.emplace_back(Referrer, Referred);
return true;
}
@@ -387,7 +419,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!BT_stackleak)
BT_stackleak =
std::make_unique(CheckNames[CK_StackAddrEscapeChecker],
- "Stack address stored into global variable");
+ "Stack address leaks outside of stack
frame");
for (const auto &P : Cb.V) {
const M
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105653
>From 902e1d63b436db3ca9e21b022e821a0182bf992c Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:53:25 +0200
Subject: [PATCH] [analyzer] Detect leak of a stack address through output
arguments
At this point only functions called from other functions (i.e., not
top-level) are covered. Top-level functions have a different exit
sequence, and will be handled by a subsequent change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 ++-
clang/test/Analysis/stack-addr-ps.cpp | 31 +++--
2 files changed, 75 insertions(+), 20 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index 2bd4ca4528de8b..dcf6801a73de2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,6 +288,23 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt
*RS,
EmitStackError(C, R, RetE);
}
+static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {
+ assert(R);
+ if (const auto *MemSpace = R->getMemorySpace()) {
+if (const auto *SSR = MemSpace->getAs())
+ return SSR;
+if (const auto *GSR = MemSpace->getAs())
+ return GSR;
+ }
+ // If R describes a lambda capture, it will be a symbolic region
+ // referring to a field region of another symbolic region.
+ if (const auto *SymReg = R->getBaseRegion()->getAs()) {
+if (const auto *OriginReg = SymReg->getSymbol()->getOriginRegion())
+ return getStackOrGlobalSpaceRegion(OriginReg);
+ }
+ return nullptr;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -297,20 +314,31 @@ std::optional printReferrer(const MemRegion
*Referrer) {
return "global";
assert(isa(Space));
return "stack";
- }(Referrer->getMemorySpace());
-
- // We should really only have VarRegions here.
- // Anything else is really surprising, and we should get notified if such
- // ever happens.
- const auto *ReferrerVar = dyn_cast(Referrer);
- if (!ReferrerVar) {
-assert(false && "We should have a VarRegion here");
-return std::nullopt; // Defensively skip this one.
+ }(getStackOrGlobalSpaceRegion(Referrer));
+
+ while (!Referrer->canPrintPretty()) {
+if (const auto *SymReg = dyn_cast(Referrer);
+SymReg && SymReg->getSymbol()->getOriginRegion()) {
+ Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+} else if (isa(Referrer)) {
+ // Skip members of a class, it is handled in CheckExprLifetime.cpp as
+ // warn_bind_ref_member_to_parameter or
+ // warn_init_ptr_member_to_parameter_addr
+ return std::nullopt;
+} else {
+ Referrer->dump();
+ assert(false && "Unexpected referrer region type.");
+ return std::nullopt;
+}
}
- const std::string ReferrerVarName =
- ReferrerVar->getDecl()->getDeclName().getAsString();
+ assert(Referrer);
+ assert(Referrer->canPrintPretty());
- return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+ std::string buf;
+ llvm::raw_string_ostream os(buf);
+ os << ReferrerMemorySpace << " variable ";
+ Referrer->printPretty(os);
+ return buf;
}
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
@@ -332,16 +360,20 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
/// referred by an other stack variable from different stack frame.
bool checkForDanglingStackVariable(const MemRegion *Referrer,
const MemRegion *Referred) {
- const auto *ReferrerMemSpace =
- Referrer->getMemorySpace()->getAs();
+ const auto *ReferrerMemSpace = getStackOrGlobalSpaceRegion(Referrer);
const auto *ReferredMemSpace =
Referred->getMemorySpace()->getAs();
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
+ const auto *ReferrerStackSpace =
+ ReferrerMemSpace->getAs();
+ if (!ReferrerStackSpace)
+return false;
+
if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
V.emplace_back(Referrer, Referred);
return true;
}
@@ -387,7 +419,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!BT_stackleak)
BT_stackleak =
std::make_unique(CheckNames[CK_StackAddrEscapeChecker],
- "Stack address stored into global variable");
+ "Stack address leaks outside of stack
frame");
for (const auto &P : Cb.V) {
const M
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
https://github.com/necto ready_for_review https://github.com/llvm/llvm-project/pull/105653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From 902e1d63b436db3ca9e21b022e821a0182bf992c Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 10:53:25 +0200
Subject: [PATCH 1/2] [analyzer] Detect leak of a stack address through output
arguments
At this point only functions called from other functions (i.e., not
top-level) are covered. Top-level functions have a different exit
sequence, and will be handled by a subsequent change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 ++-
clang/test/Analysis/stack-addr-ps.cpp | 31 +++--
2 files changed, 75 insertions(+), 20 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index 2bd4ca4528de8b..dcf6801a73de2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -288,6 +288,23 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt
*RS,
EmitStackError(C, R, RetE);
}
+static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {
+ assert(R);
+ if (const auto *MemSpace = R->getMemorySpace()) {
+if (const auto *SSR = MemSpace->getAs())
+ return SSR;
+if (const auto *GSR = MemSpace->getAs())
+ return GSR;
+ }
+ // If R describes a lambda capture, it will be a symbolic region
+ // referring to a field region of another symbolic region.
+ if (const auto *SymReg = R->getBaseRegion()->getAs()) {
+if (const auto *OriginReg = SymReg->getSymbol()->getOriginRegion())
+ return getStackOrGlobalSpaceRegion(OriginReg);
+ }
+ return nullptr;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -297,20 +314,31 @@ std::optional printReferrer(const MemRegion
*Referrer) {
return "global";
assert(isa(Space));
return "stack";
- }(Referrer->getMemorySpace());
-
- // We should really only have VarRegions here.
- // Anything else is really surprising, and we should get notified if such
- // ever happens.
- const auto *ReferrerVar = dyn_cast(Referrer);
- if (!ReferrerVar) {
-assert(false && "We should have a VarRegion here");
-return std::nullopt; // Defensively skip this one.
+ }(getStackOrGlobalSpaceRegion(Referrer));
+
+ while (!Referrer->canPrintPretty()) {
+if (const auto *SymReg = dyn_cast(Referrer);
+SymReg && SymReg->getSymbol()->getOriginRegion()) {
+ Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+} else if (isa(Referrer)) {
+ // Skip members of a class, it is handled in CheckExprLifetime.cpp as
+ // warn_bind_ref_member_to_parameter or
+ // warn_init_ptr_member_to_parameter_addr
+ return std::nullopt;
+} else {
+ Referrer->dump();
+ assert(false && "Unexpected referrer region type.");
+ return std::nullopt;
+}
}
- const std::string ReferrerVarName =
- ReferrerVar->getDecl()->getDeclName().getAsString();
+ assert(Referrer);
+ assert(Referrer->canPrintPretty());
- return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
+ std::string buf;
+ llvm::raw_string_ostream os(buf);
+ os << ReferrerMemorySpace << " variable ";
+ Referrer->printPretty(os);
+ return buf;
}
void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
@@ -332,16 +360,20 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
/// referred by an other stack variable from different stack frame.
bool checkForDanglingStackVariable(const MemRegion *Referrer,
const MemRegion *Referred) {
- const auto *ReferrerMemSpace =
- Referrer->getMemorySpace()->getAs();
+ const auto *ReferrerMemSpace = getStackOrGlobalSpaceRegion(Referrer);
const auto *ReferredMemSpace =
Referred->getMemorySpace()->getAs();
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;
+ const auto *ReferrerStackSpace =
+ ReferrerMemSpace->getAs();
+ if (!ReferrerStackSpace)
+return false;
+
if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
V.emplace_back(Referrer, Referred);
return true;
}
@@ -387,7 +419,7 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
if (!BT_stackleak)
BT_stackleak =
std::make_unique(CheckNames[CK_StackAddrEscapeChecker],
- "Stack address stored into global variable");
+ "Stack address leaks outside of stack
frame");
for (const auto &P : Cb.V) {
con
[clang] [analyzer] Detect leak of a stack address through output arguments 2/3 (PR #105653)
necto wrote: @steakhal I've rebased atop of `main` and squashed. CI is green. Could you, please, merge this PR (following https://github.com/llvm/llvm-project/pull/105652)? https://github.com/llvm/llvm-project/pull/105653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/105648
>From 991f176c5545fedae2ba8b5c1b357734abe68ac7 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 11:16:10 +0200
Subject: [PATCH] [analyzer] Detect leaks on top-level via output params,
indirect globals
Fix some false negatives of StackAddrEscapeChecker:
- Output parameters
```
void top(int **out) {
int local = 42;
*out = &local; // Noncompliant
}
```
- Indirect global pointers
```
int **global;
void top() {
int local = 42;
*global = &local; // Noncompliant
}
```
Note that now StackAddrEscapeChecker produces a diagnostic if a function
with an output parameter is analyzed as top-level or as a callee. I took
special care to make sure the reports point to the same primary location
and, in many cases, feature the same primary message. That is the
motivation to modify Core/BugReporter.cpp and Core/ExplodedGraph.cpp
To avoid false positive reports when a global indirect pointer is
assigned a local address, invalidated, and then reset, I rely on the
fact that the invalidation symbol will be a DerivedSymbol of a
ConjuredSymbol that refers to the same memory region.
The checker still has a false negative for non-trivial escaping via a
returned value. It requires a more sophisticated traversal akin to
scanReachableSymbols, which out of the scope of this change.
CPP-4734
---
.../Checkers/StackAddrEscapeChecker.cpp | 64 +-
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 13 +-
.../lib/StaticAnalyzer/Core/ExplodedGraph.cpp | 2 +-
clang/test/Analysis/copy-elision.cpp | 20 +--
.../test/Analysis/incorrect-checker-names.cpp | 4 +-
clang/test/Analysis/loop-block-counts.c | 2 +-
clang/test/Analysis/stack-addr-ps.c | 6 +-
clang/test/Analysis/stack-addr-ps.cpp | 117 ++
.../Analysis/stack-capture-leak-no-arc.mm | 4 +-
clang/test/Analysis/stackaddrleak.c | 8 +-
10 files changed, 157 insertions(+), 83 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index dcf6801a73de2d..82f03e5f7d09d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -305,6 +305,14 @@ static const MemSpaceRegion
*getStackOrGlobalSpaceRegion(const MemRegion *R) {
return nullptr;
}
+const MemRegion *getOriginBaseRegion(const MemRegion *Referrer) {
+ Referrer = Referrer->getBaseRegion();
+ while (const auto *SymReg = dyn_cast(Referrer)) {
+Referrer = SymReg->getSymbol()->getOriginRegion()->getBaseRegion();
+ }
+ return Referrer;
+}
+
std::optional printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
@@ -313,7 +321,8 @@ std::optional printReferrer(const MemRegion
*Referrer) {
if (isa(Space))
return "global";
assert(isa(Space));
-return "stack";
+// This case covers top-level and inlined analyses.
+return "caller";
}(getStackOrGlobalSpaceRegion(Referrer));
while (!Referrer->canPrintPretty()) {
@@ -348,12 +357,27 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
ExplodedNode *Node = Ctx.getPredecessor();
+ bool ExitingTopFrame =
+ Ctx.getPredecessor()->getLocationContext()->inTopFrame();
+
+ if (ExitingTopFrame && Node->getLocation().getTag() &&
+ Node->getLocation().getTag()->getTagDescription() ==
+ "ExprEngine : Clean Node" &&
+ Node->getFirstPred()) {
+// When finishing analysis of a top-level function, engine proactively
+// removes dead symbols thus preventing this checker from looking through
+// the output parameters. Take 1 step back, to the node where these symbols
+// and their bindings are still present
+Node = Node->getFirstPred();
+ }
+
// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
class CallBack : public StoreManager::BindingsHandler {
private:
CheckerContext &Ctx;
const StackFrameContext *PoppedFrame;
+const bool TopFrame;
/// Look for stack variables referring to popped stack variables.
/// Returns true only if it found some dangling stack variables
@@ -369,24 +393,48 @@ void StackAddrEscapeChecker::checkEndFunction(const
ReturnStmt *RS,
const auto *ReferrerStackSpace =
ReferrerMemSpace->getAs();
+
if (!ReferrerStackSpace)
return false;
- if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
- ReferrerStackSpace->getStackFrame()->isParentOf(PoppedFrame)) {
+ if (const auto *ReferredFrame = ReferredMemSpace->getStackFrame();
+ ReferredFrame != PoppedFrame) {
+return false;
+ }
+
+ if (ReferrerStackSpace->getStack
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
https://github.com/necto ready_for_review https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Detect leaks of stack addresses via output params, indirect globals 3/3 (PR #105648)
necto wrote: > Please ping me when this commit is in a clean state that can be reviewed > (e.g. updates on earlier commits are incorporated). Thanks! @NagyDonat , the earlier commits are now merged and I rebased this PR. Feel free to have a look https://github.com/llvm/llvm-project/pull/105648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106048
>From 7d5ae515f7727de98e7e8ce2f259e579a1f24463 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 17:31:11 +0200
Subject: [PATCH] [analyzer] Report violations of the "returns_nonnull"
attribute
Make sure code respects the GNU-extension
__attribute__((returns_nonnull)).
Extend the NullabilityChecker to check that a function returns_nonnull
does not return a nullptr.
CPP-4741
---
.../Checkers/NullabilityChecker.cpp | 8
clang/test/Analysis/nullability.c | 43 ++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 60934e51febe84..2035d50eea4c2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
NullConstraint Nullness = getNullConstraint(*RetSVal, State);
Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType);
+ if (const auto *FunDecl = C.getLocationContext()->getDecl();
+ FunDecl && FunDecl->getAttr() &&
+ (RequiredNullability == Nullability::Unspecified ||
+ RequiredNullability == Nullability::Nullable)) {
+// If a function is marked with the returns_nonnull attribute,
+// the return value must be non-null.
+RequiredNullability = Nullability::Nonnull;
+ }
// If the returned value is null but the type of the expression
// generating it is nonnull then we will suppress the diagnostic.
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index fbc03c864ad83f..9ddb9c8d2dc34f 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability
-Wno-deprecated-non-prototype -verify %s
+// RUN: %clang_analyze_cc1 -fblocks
-analyzer-checker=core,nullability,debug.ExprInspection
-Wno-deprecated-non-prototype -verify %s
+
+void clang_analyzer_warnIfReached();
void it_takes_two(int a, int b);
void function_pointer_arity_mismatch() {
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
+ }
+ // Regardless of the potential state split above, x cannot be nullptr
+ // according to the produce_nonnull_ptr annotation.
+ return x;
+ // False positive: expected-warning@-1 {{Null returned from a function that
is expected to return a non-null value}}
+}
+
+__attribute__((returns_nonnull)) int *passthrough(int *p) {
+ return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106048
>From 7d5ae515f7727de98e7e8ce2f259e579a1f24463 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 17:31:11 +0200
Subject: [PATCH 1/2] [analyzer] Report violations of the "returns_nonnull"
attribute
Make sure code respects the GNU-extension
__attribute__((returns_nonnull)).
Extend the NullabilityChecker to check that a function returns_nonnull
does not return a nullptr.
CPP-4741
---
.../Checkers/NullabilityChecker.cpp | 8
clang/test/Analysis/nullability.c | 43 ++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 60934e51febe84..2035d50eea4c2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
NullConstraint Nullness = getNullConstraint(*RetSVal, State);
Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType);
+ if (const auto *FunDecl = C.getLocationContext()->getDecl();
+ FunDecl && FunDecl->getAttr() &&
+ (RequiredNullability == Nullability::Unspecified ||
+ RequiredNullability == Nullability::Nullable)) {
+// If a function is marked with the returns_nonnull attribute,
+// the return value must be non-null.
+RequiredNullability = Nullability::Nonnull;
+ }
// If the returned value is null but the type of the expression
// generating it is nonnull then we will suppress the diagnostic.
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index fbc03c864ad83f..9ddb9c8d2dc34f 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability
-Wno-deprecated-non-prototype -verify %s
+// RUN: %clang_analyze_cc1 -fblocks
-analyzer-checker=core,nullability,debug.ExprInspection
-Wno-deprecated-non-prototype -verify %s
+
+void clang_analyzer_warnIfReached();
void it_takes_two(int a, int b);
void function_pointer_arity_mismatch() {
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
+ }
+ // Regardless of the potential state split above, x cannot be nullptr
+ // according to the produce_nonnull_ptr annotation.
+ return x;
+ // False positive: expected-warning@-1 {{Null returned from a function that
is expected to return a non-null value}}
+}
+
+__attribute__((returns_nonnull)) int *passthrough(int *p) {
+ return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
+}
>From d3b30f3f31cc3a4cc71ae967ea6fc554a4764e37 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Mon, 26 Aug 2024 15:59:59 +0200
Subject: [PATCH 2/2] Update clang/test/Analysis/nullability.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-authored-by: Donát Nagy
---
clang/test/Analysis/nullability.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index 9ddb9c8d2dc34f..341d29c6e99d1e 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -39,8 +39,9 @@ int *cannot_return_null() {
int *x = produce_nonnull_ptr();
if (!x) {
clang_analyzer_warnIfReached();
-// Incorrect: expected-warning@-1 {{REACHABLE}}
-// According to produce_nonnull_ptr contract,
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
necto wrote:
We've entered it as a Jira ticket:
https://sonarsource.atlassian.net/browse/CPP-5614
But we have no immediate plans to implement it.
https://github.com/llvm/llvm-project/pull/106048
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
https://github.com/necto updated
https://github.com/llvm/llvm-project/pull/106048
>From 7d5ae515f7727de98e7e8ce2f259e579a1f24463 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Tue, 20 Aug 2024 17:31:11 +0200
Subject: [PATCH 1/5] [analyzer] Report violations of the "returns_nonnull"
attribute
Make sure code respects the GNU-extension
__attribute__((returns_nonnull)).
Extend the NullabilityChecker to check that a function returns_nonnull
does not return a nullptr.
CPP-4741
---
.../Checkers/NullabilityChecker.cpp | 8
clang/test/Analysis/nullability.c | 43 ++-
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 60934e51febe84..2035d50eea4c2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S,
NullConstraint Nullness = getNullConstraint(*RetSVal, State);
Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType);
+ if (const auto *FunDecl = C.getLocationContext()->getDecl();
+ FunDecl && FunDecl->getAttr() &&
+ (RequiredNullability == Nullability::Unspecified ||
+ RequiredNullability == Nullability::Nullable)) {
+// If a function is marked with the returns_nonnull attribute,
+// the return value must be non-null.
+RequiredNullability = Nullability::Nonnull;
+ }
// If the returned value is null but the type of the expression
// generating it is nonnull then we will suppress the diagnostic.
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index fbc03c864ad83f..9ddb9c8d2dc34f 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability
-Wno-deprecated-non-prototype -verify %s
+// RUN: %clang_analyze_cc1 -fblocks
-analyzer-checker=core,nullability,debug.ExprInspection
-Wno-deprecated-non-prototype -verify %s
+
+void clang_analyzer_warnIfReached();
void it_takes_two(int a, int b);
void function_pointer_arity_mismatch() {
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
+ }
+ // Regardless of the potential state split above, x cannot be nullptr
+ // according to the produce_nonnull_ptr annotation.
+ return x;
+ // False positive: expected-warning@-1 {{Null returned from a function that
is expected to return a non-null value}}
+}
+
+__attribute__((returns_nonnull)) int *passthrough(int *p) {
+ return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
+}
>From d3b30f3f31cc3a4cc71ae967ea6fc554a4764e37 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh
Date: Mon, 26 Aug 2024 15:59:59 +0200
Subject: [PATCH 2/5] Update clang/test/Analysis/nullability.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-authored-by: Donát Nagy
---
clang/test/Analysis/nullability.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/test/Analysis/nullability.c
b/clang/test/Analysis/nullability.c
index 9ddb9c8d2dc34f..341d29c6e99d1e 100644
--- a/clang/test/Analysis/nullability.c
+++ b/clang/test/Analysis/nullability.c
@@ -39,8 +39,9 @@ int *cannot_return_null() {
int *x = produce_nonnull_ptr();
if (!x) {
clang_analyzer_warnIfReached();
-// Incorrect: expected-warning@-1 {{REACHABLE}}
-// According to produce_nonnull_ptr contract,
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
@@ -1,4 +1,6 @@ -// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability -Wno-deprecated-non-prototype -verify %s +// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability,debug.ExprInspection -Wno-deprecated-non-prototype -verify %s necto wrote: Done in a9c489f683fc https://github.com/llvm/llvm-project/pull/106048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
@@ -10,3 +12,42 @@ void block_arity_mismatch() {
void(^b)() = ^(int a, int b) { };
b(1); // no-crash expected-warning {{Block taking 2 arguments is called
with fewer (1)}}
}
+
+int *nonnull_return_annotation_indirect() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_indirect() {
+ int *x = 0;
+ return x; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *nonnull_return_annotation_direct() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_direct() {
+ return 0; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+} // expected-warning@-1 {{null returned from function that requires a
non-null return value}}
+
+int *nonnull_return_annotation_assumed() __attribute__((returns_nonnull));
+int *nonnull_return_annotation_assumed(int* ptr) {
+ if (ptr) {
+return ptr;
+ }
+ return ptr; // expected-warning {{Null returned from a function that is
expected to return a non-null value}}
+}
+
+int *produce_nonnull_ptr() __attribute__((returns_nonnull));
+
+__attribute__((returns_nonnull))
+int *cannot_return_null() {
+ int *x = produce_nonnull_ptr();
+ if (!x) {
+clang_analyzer_warnIfReached();
+// Incorrect: expected-warning@-1 {{REACHABLE}}
+// According to produce_nonnull_ptr contract, x cannot be null.
+ }
+ // Regardless of the potential state split above, x cannot be nullptr
+ // according to the produce_nonnull_ptr annotation.
+ return x;
+ // False positive: expected-warning@-1 {{Null returned from a function that
is expected to return a non-null value}}
+}
+
+__attribute__((returns_nonnull)) int *passthrough(int *p) {
+ return p; // no-warning: we have no evidence that `p` is null, i.e.,
violating the contract
+}
necto wrote:
Added in 3d5e750aec0d
However, it did not trigger because of an explicit suppression of the reports
in inlined functions:
https://github.com/llvm/llvm-project/commit/49bd58f1ebe28d97e4949e9c757bc5dfd8b2d72f
I believe the reasoning for that suppression no longer holds, so I lifted the
suppression: 598c574e0703
Do you agree? If so, do you think it should be done in a separate change set?
https://github.com/llvm/llvm-project/pull/106048
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Report violations of the "returns_nonnull" attribute (PR #106048)
@@ -692,6 +692,14 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, NullConstraint Nullness = getNullConstraint(*RetSVal, State); Nullability RequiredNullability = getNullabilityAnnotation(RequiredRetType); + if (const auto *FunDecl = C.getLocationContext()->getDecl(); necto wrote: `getNullabilityAnnotation` is concerned only with the type. It is used both for return types and function parameters by multiple checkers. Handling the function annotation there would mean I have to pass the function declaration (which might not be available in the client of the API), which is only relevant in one case (and maybe another couple in TrustNonnullChecker). That means I would need to complicate the function definition and potentially increase the PR footprint by touching all call sites. https://github.com/llvm/llvm-project/pull/106048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
