[analyzer][NFC] Add tests for and refactor StackAddrEscapeChecker 1/3 #105652
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These tests and refactoring are preparatory for the upcoming changes: detection of the indirect leak via global variables and output parameters. CPP-4734
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Arseniy Zaostrovnykh (necto) ChangesThese 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 #105648 Patch is 21.45 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105652.diff 3 Files Affected:
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<std::string> printReferrer(const MemRegion *Referrer) {
+ assert(Referrer);
+ const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
+ if (isa<StaticGlobalSpaceRegion>(Space))
+ return "static";
+ if (isa<GlobalsSpaceRegion>(Space))
+ return "global";
+ assert(isa<StackSpaceRegion>(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<VarRegion>(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<CXXTempObjectRegion, CXXLifetimeExtendedObjectRegion>(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<PathSensitiveBugReport>(*BT_stackleak, Out.str(), N);
@@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
return;
}
- const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
- if (isa<StaticGlobalSpaceRegion>(Space))
- return "static";
- if (isa<GlobalsSpaceRegion>(Space))
- return "global";
- assert(isa<StackSpaceRegion>(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<VarRegion>(Referrer);
- if (!ReferrerVar) {
- assert(false && "We should have a VarRegion here");
- continue; // Defensively skip this one.
+ auto ReferrerVariable = printReferrer(Referrer);
+ if (!ReferrerVariable) {
+ continue;
}
- const std::string ReferrerVarName =
- ReferrerVar->getDecl()->getDeclName().getAsString();
- Out << " is still referred to by the " << ReferrerMemorySpace
- << " variable '" << ReferrerVarName << "' " << CommonSuffix;
+ Out << " is still referred to by the " << *ReferrerVariable << CommonSuffix;
auto Report =
std::make_unique<PathSensitiveBugReport>(*BT_stackleak, Out.str(), N);
if (Range.isValid())
diff --git a/clang/test/Analysis/stack-addr-ps.c b/clang/test/Analysis/stack-addr-ps.c
index e69ab4189b524f..2e14b7820be136 100644
--- a/clang/test/Analysis/stack-addr-ps.c
+++ b/clang/test/Analysis/stack-addr-ps.c
@@ -95,3 +95,34 @@ void callTestRegister(void) {
char buf[20];
testRegister(buf); // no-warning
}
+
+void top_level_leaking(int **out) {
+ int local = 42;
+ *out = &local; // no-warning FIXME
+}
+
+void callee_leaking_via_param(int **out) {
+ int local = 1;
+ *out = &local;
+ // expected-warning@-1{{Address of stack memory associated with local variable 'local' is still referred to by the stack variable 'ptr'}}
+}
+
+void caller_for_leaking_callee() {
+ int *ptr = 0;
+ callee_leaking_via_param(&ptr);
+}
+
+void callee_nested_leaking(int **out) {
+ int local = 1;
+ *out = &local;
+ // expected-warning@-1{{Address of stack memory associated with local variable 'local' is still referred to by the stack variable 'ptr'}}
+}
+
+void caller_mid_for_nested_leaking(int **mid) {
+ callee_nested_leaking(mid);
+}
+
+void caller_for_nested_leaking() {
+ int *ptr = 0;
+ caller_mid_for_nested_leaking(&ptr);
+}
diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp
index bd856be2b8d690..68ccc322bf2ef2 100644
--- a/clang/test/Analysis/stack-addr-ps.cpp
+++ b/clang/test/Analysis/stack-addr-ps.cpp
@@ -161,3 +161,599 @@ 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<typename Callable>
+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<typename Callback>
+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; // no-warning FIXME
+}
+
+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; // no-warning FIXME
+ };
+ 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'}}
+}
+
+void param_ptr_to_ptr_fptr_caller(void (*fptr)(int**)) {
+ int* p = nullptr;
+ fptr(&p);
+}
+
+void param_ptr_to_ptr_caller_caller() {
+ void (*fptr)(int**) = param_ptr_to_ptr_fptr;
+ param_ptr_to_ptr_fptr_caller(fptr);
+}
+} // namespace leaking_via_ptr_to_ptr
+
+namespace leaking_via_ref_to_ptr {
+void** make_ptr_to_ptr();
+void*& global_rtp = *make_ptr_to_ptr();
+
+void global_ref_to_ptr() {
+ int local = 42;
+ int* p = &local;
+ global_rtp = p; // no-warning FIXME
+}
+
+void static_ref_to_ptr() {
+ int local = 42;
+ static void*& p = *make_ptr_to_ptr();
+ p = &local;
+ (void)p;
+} // no-warning False Negative, requires relating multiple bindings to cross the invented pointer.
+
+void param_ref_to_ptr_top(void*& rp) {
+ int local = 42;
+ int* p = &local;
+ rp = p; // no-warning FIXME
+}
+
+void param_ref_to_ptr_callee(void*& rp) {
+ int local = 42;
+ int* p = &local;
+ rp = p; // expected-warning{{local variable 'local' is still referred to by the stack variable 'p'}}
+}
+
+void param_ref_to_ptr_caller() {
+ void* p = nullptr;
+ param_ref_to_ptr_callee(p);
+}
+} // namespace leaking_via_ref_to_ptr
+
+namespace leaking_via_arr_of_ptr_static_idx {
+void** returned_arr_of_ptr_top() {
+ int local = 42;
+ int* p = &local;
+ void** arr = new void*[2];
+ arr[1] = p;
+ return arr;
+} // no-warning False Negative
+
+void** returned_arr_of_ptr_callee() {
+ int local = 42;
+ int* p = &local;
+ void** arr = new void*[2];
+ arr[1] = p;
+ return arr;
+} // no-warning False Negative
+
+void returned_arr_of_ptr_caller() {
+ void** arr = returned_arr_of_ptr_callee();
+ (void)arr[1];
+}
+
+void* global_aop[2];
+
+void global_arr_of_ptr() {
+ int local = 42;
+ int* p = &local;
+ global_aop[1] = p;
+} // expected-warning{{local variable 'local' is still referred to by the global variable 'global_aop'}}
+
+void static_arr_of_ptr() {
+ int local = 42;
+ static void* arr[2];
+ arr[1] = &local;
+ (void)arr[1];
+} // expected-warning{{local variable 'local' is still referred to by the static variable 'arr'}}
+
+void param_arr_of_ptr_top(void* arr[2]) {
+ int local = 42;
+ int* p = &local;
+ arr[1] = p; // no-warning FIXME
+}
+
+void param_arr_of_ptr_callee(void* arr[2]) {
+ int local = 42;
+ int* p = &local;
+ arr[1] = p; // expected-warning{{local variable 'local' is still referred to by the stack variable 'arrStack'}}
+}
+
+void param_arr_of_ptr_caller() {
+ void* arrStack[2];
+ param_arr_of_ptr_callee(arrStack);
+ (void)arrStack[1];
+}
+} // namespace leaking_via_arr_of_ptr_static_idx
+
+namespace leaking_via_arr_of_ptr_dynamic_idx {
+void** returned_arr_of_ptr_top(int idx) {
+ int local = 42;
+ int* p = &local;
+ void** arr = new void*[2];
+ arr[idx] = p;
+ return arr;
+} // no-warning False Negative
+
+void** returned_arr_of_ptr_callee(int idx) {
+ int local = 42;
+ int* p = &local;
+ void** arr = new void*[2];
+ arr[idx] = p;
+ return arr;
+} // no-warning False Negative
+
+void returned_arr_of_ptr_caller(int idx) {
+ void** arr = returned_arr_of_ptr_callee(idx);
+ (void)arr[idx];
+}
+
+void* global_aop[2];
+
+void global_arr_of_ptr(int idx) {
+ int local = 42;
+ int* p = &local;
+ global_aop[idx] = p;
+} // expected-warning{{local variable 'local' is still referred to by the global variable 'global_aop'}}
+
+void static_arr_of_ptr(int idx) {
+ int local = 42;
+ static void* arr[2];
+ arr[idx] = &local;
+ (void)arr[idx];
+} // expected-warning{{local variable 'local' is still referred to by the static variable 'arr'}}
+
+void param_arr_of_ptr_top(void* arr[2], int idx) {
+ int local = 42;
+ int* p = &local;
+ arr[idx] = p; // no-warning FIXME
+}
+
+void param_arr_of_ptr_callee(void* arr[2], int idx) {
+ int local = 42;
+ int* p = &local;
+ arr[idx] = p; // expected-warning{{local variable 'local' is still referred to by the stack variable 'arrStack'}}
+}
+
+void param_arr_of_ptr_caller(int idx) {
+ void* arrStack[2];
+ param_arr_of_ptr_callee(arrStack, idx);
+ (void)arrStack[idx];
+}
+} // namespace leaking_via_arr_of_ptr_dynamic_idx
+
+namespace leaking_via_struct_with_ptr {
+struct S {
+ int* p;
+};
+
+S returned_struct_with_ptr_top() {
+ int local = 42;
+ S s;
+ s.p = &local;
+ return s;
+} // no-warning False Negative, requires traversing returned LazyCompoundVals
+
+S returned_struct_with_ptr_callee() {
+ int local = 42;
+ S s;
+ s.p = &local;
+ return s; // expected-warning{{'local' is still referred to by the stack variable 's'}}
+}
+
+void returned_struct_with_ptr_caller() {
+ S s = returned_struct_with_ptr_callee();
+ (void)s.p;
+}
+
+S global_s;
+
+void global_struct_with_ptr() {
+ int local = 42;
+ global_s.p = &local;
+} // expected-warning{{'local' is still referred to by the global variable 'global_s'}}
+
+void static_struct_with_ptr() {
+ int local = 42;
+ static S s;
+ s.p = &local;
+ (void)s.p;
+} // expected-warning{{'local' is still referred to by the static variable 's'}}
+} // namespace leaking_via_struct_with_ptr
+
+namespace leaking_via_ref_to_struct_with_ptr {
+struct S {
+ int* p;
+};
+
+S &global_s = *(new S);
+
+void global_ref_to_struct_with_ptr() {
+ int local = 42;
+ global_s.p = &local; // no-warning FIXME
+}
+
+void static_ref_to_struct_with_ptr() {
+ int local = 42;
+ static S &s = *(new S);
+ s.p = &local;
+ (void)s.p;
+} // no-warning False Negative, requires relating multiple bindings to cross a heap region.
+
+void param_ref_to_struct_with_ptr_top(S &s) {
+ int local = 42;
+ s.p = &local; // no-warning FIXME
+}
+
+void param_ref_to_struct_with_ptr_callee(S &s) {
+ int local = 42;
+ s.p = &local; // expected-warning{{'local' is still referred to by the stack variable 'sStack'}}
+}
+
+void param_ref_to_struct_with_ptr_caller() {
+ S sStack;
+ param_ref_to_struct_with_ptr_callee(sStack);
+}
+
+template<typename Callable>
+void lambda_param_capture_callee(Callable& callee) {
+ int local = 42;
+ callee(local); // expected-warning{{'local' is still referred to by the stack variable 'p'}}
+}
+
+void lambda_param_capture_caller() {
+ int* p = nullptr;
+ auto capt = [&p](int& param) {
+ p = ¶m;
+ };
+ lambda_param_capture_callee(capt);
+}
+} // namespace leaking_via_ref_to_struct_with_ptr
+
+namespace leaking_via_ptr_to_struct_with_ptr {
+struct S {
+ int* p;
+};
+
+S* returned_ptr_to_struct_with_ptr_top() {
+ int local = 42;
+ S* s = new S;
+ s->p = &local;
+ return s;
+} // no-warning False Negative
+
+S* returned_ptr_to_struct_with_ptr_callee() {
+ int local = 42;
+ S* s = new S;
+ s->p = &local;
+ return s;
+} // no-warning False Negative
+
+void returned_ptr_to_struct_with_ptr_caller() {
+ S* s = returned_ptr_to_struct_with_ptr_callee();
+ (void)s->p;
+}
+
+S* global_s;
+
+void global_ptr_to_struct_with_ptr() {
+ int local = 42;
+ global_s->p = &local; // no-warning FIXME
+}
+
+void static_ptr_to_struct_with_ptr_new() {
+ int local = 42;
+ static S* s = new S;
+ s->p = &local;
+ (void)s->p;
+} // no-warning False Negative, requires relating multiple bindings to cross a heap region.
+
+S* get_some_s();
+
+void static_ptr_to_struct_with_ptr_generated() {
+ int local = 42;
+ static S* s = get_some_s();
+ s->p = &local;
+} // no-warning False Negative, requires relating multiple bindings to cross the invented pointer.
+
+void param_ptr_to_struct_with_ptr_top(S* s) {
+ int local = 42;
+ s->p = &local; // no-warning FIXME
+}
+
+void param_ptr_to_struct_with_ptr_callee(S* s) {
+ int local = 42;
+ s->p = &local; // expected-warning{{'local' is still referred to by the stack variable 's'}}
+}
+
+void param_ptr_to_struct_with_ptr_caller() {
+ S s;
+ param_ptr_to_struct_with_ptr_callee(&s);
+ (void)s.p;
+}
+} // namespace leaking_via_ptr_to_struct_with_ptr
+
+namespace leaking_via_arr_of_struct_with_ptr {
+struct S {
+ int* p;
+};
+
+S* returned_ptr_to_struct_with_ptr_top() {
+ int local = 42;
+ S* s = new S[2];
+ s[1].p = &local;
+ return s;
+} // no-warning False Negative
+
+S* returned_ptr_to_struct_with_ptr_callee() {
+ int local = 42;
+ S* s = new S[2];
+ s[1].p = &local;
+ return s;
+} // no-warning False Negative
+
+void returned_ptr_to_struct_with_ptr_caller() {
+ S* s = returned_ptr_to_struct_with_ptr_callee();
+ (void)s[1].p;
+}
+
+S global_s[2];
+
+void global_ptr_to_struct_with_ptr() {
+ int local = 42;
+ global_s[1].p = &local;
+} // expected-warning{{'local' is still referred to by the global variable 'global_s'}}
+
+void static_ptr_to_struct_with_ptr_new() {
+ int local = 42;
+ static S* s = new S[2];
+ s[1].p = &local;
+ (void)s[1].p;
+}
+
+S* get_some_s();
+
+void static_ptr_to_struct_with_ptr_generated() {
+ int local = 42;
+ static S* s = get_some_s();
+ s[1].p = &local;
+} // no-warning False Negative, requires relating multiple bindings to cross the invented pointer.
+
+void param_ptr_to_struct_with_ptr_top(S s[2]) {
+ int local = 42;
+ s[1].p = &local; // no-...
[truncated]
|
This was referenced Aug 22, 2024
steakhal
changed the title
steakhal
changed the title
NagyDonat
approved these changes
Aug 22, 2024
steakhal
pushed a commit
that referenced
this pull request
Aug 26, 2024
#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 #105648 it must not be merged before #105652
Xazax-hun
reviewed
Aug 27, 2024
| @@ -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"; | |||
There was a problem hiding this comment.
Is having two spaces after a dot a common convention we have? This is not a change in this PR, I'm just wondering if this was intentional or a typo.
There was a problem hiding this comment.
It is an older typographical convention that was historically common: https://en.wikipedia.org/wiki/Sentence_spacing
However, in the last 10-20 years it became obsolete almost everywhere, and I don't recall any other message where the static analyzer uses it. I'd guess that it was intentional (perhaps the personally preferred style of some contributor?), but we should probably return to single spacing.
There was a problem hiding this comment.
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'?
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
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
Function 'rand' is obsolete because it implements a poor random number generator. Use 'arc4random' instead
Overall, if lit-test messages stats is of any indication, it is relatively balanced:
grep -R '\(warning\|note\){.*\. [A-Za-z]' clang/test/ | wc
-> 23 (double space after dot)
grep -R '\(warning\|note\){.*\. [A-Za-z]' clang/test/ | wc
-> 37 (single space after dot)
steakhal
pushed a commit
that referenced
this pull request
Aug 28, 2024
…t globals 3/3 (#105648) 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 --------- This is the last of the 3 stacked PRs, it must not be merged before #105652 and #105653
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 the first of three commits constituting #105648