False negatives clang-analyzer-core.StackAddressEscape when storing pointers/references in container #123459
Comments
|
Sorry, what's the issue here? You are taking a reference/pointer to a local variable and the |
|
Yes, but it should be possible to find the issues in static analysis, |
|
@llvm/issue-subscribers-clang-static-analyzer Author: None (chrchr-github)
~~~c++
#include <string>
#include <vector>
struct S { std::string* s; }; struct T { std::string& s; }; int main() { |
|
Does it reproduce the FN if you avoid using the Thank you. |
steakhal
added
the
needs-reduction
|
Here's a reduction of the pointer case: struct S { int* p; };
S f() {
S s;
{
int a = 1;
s = S(&a);
}
return s;
}
int main() {
return *f().p;
}https://godbolt.org/z/MGzohEheE This is caught: |
|
Oddly, this is caught as C++ but not as C code: struct S { int* p; };
struct S f() {
struct S s;
{
int a = 1;
s = (struct S){ &a };
}
return s;
}
int main() {
return *f().p;
}https://godbolt.org/z/Edjbsc8n9 |
|
More container-like: struct S { int* p; };
template <typename V>
struct T {
V* q{};
T() = default;
T(T&& rhs) { q = rhs.q; rhs.q = nullptr;}
T& operator=(T&& rhs) { q = rhs.q; rhs.q = nullptr;}
void push_back(const V& v) { if (q == nullptr) q = new V(v); }
~T() { delete q; }
};
T<S> f() {
T<S> t;
{
int a = 1;
t.push_back({ &a });
}
return t;
}
int main() {
return *f().q->p;
} |
steakhal
removed
the
needs-reduction
|
Thank you for the several proposed reproducers. They helped a lot really. struct S { int* p; };
S top() {
int a;
S t(&a);
return t;
}This should work AFAIK the checker. So when leaving the top-level function I think this is a good first issue. Remember to use the |
steakhal
added
the
good first issue
|
Hi! This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:
If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below. |
|
@llvm/issue-subscribers-good-first-issue Author: None (chrchr-github)
~~~c++
#include <string>
#include <vector>
struct S { std::string* s; }; struct T { std::string& s; }; int main() { |
|
Looks like an issue with the callback in checkEndFunction not checking deep enough into the SVal of the binding. For this: I get only one binding checked by checkEndFunction: (edit:) and both of these have Similar, there are false negatives with other SVals where the stack capture is deeper in the structure, so these examples are also false negatives: So the fix will pretty much be inspecting the SVal of the binding more deeply for escaping stack regions. I think this is a pretty easy win to cover some more false negatives. Working on a fix while the other benchmarks run |
Sounds good on the high level. I hope you know about SValVisitor, iterBindings and ScanReachableSymbols. |
…hecker (#125638) Fixes llvm/llvm-project#123459. Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in llvm/llvm-project#123459. The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the `checkEndFunction` side of the checker also. Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from` checkEndFunction` `iterBindings` checking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop the `checkEndFunction` without breaking other `checkEndFunction` warnings that we do want. Thoughts here?
…m#125638) Fixes llvm#123459. Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in llvm#123459. The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the `checkEndFunction` side of the checker also. Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from` checkEndFunction` `iterBindings` checking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop the `checkEndFunction` without breaking other `checkEndFunction` warnings that we do want. Thoughts here?
|
The original example #123459 (comment) as well as |
|
@chrchr-github, you are right. I only picked the latest reduced case here and didn't try out the previous ones which differ. Could we reopen this issue @steakhal @Xazax-hun? I see what the issue is for the others and will take a stab at it later today. |
…m#125638) Fixes llvm#123459. Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in llvm#123459. The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the `checkEndFunction` side of the checker also. Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from` checkEndFunction` `iterBindings` checking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop the `checkEndFunction` without breaking other `checkEndFunction` warnings that we do want. Thoughts here?
…frames (#126986) Fixes #123459. This changes checking of the returned expr to also look for memory regions whose stack frame context was a child of the current stack frame context, e.g., for cases like this given in #123459: ``` struct S { int *p; }; S f() { S s; { int a = 1; s.p = &a; } return s; } ```
…m#125638) Fixes llvm#123459. Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in llvm#123459. The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the `checkEndFunction` side of the checker also. Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from` checkEndFunction` `iterBindings` checking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop the `checkEndFunction` without breaking other `checkEndFunction` warnings that we do want. Thoughts here?
…frames (llvm#126986) Fixes llvm#123459. This changes checking of the returned expr to also look for memory regions whose stack frame context was a child of the current stack frame context, e.g., for cases like this given in llvm#123459: ``` struct S { int *p; }; S f() { S s; { int a = 1; s.p = &a; } return s; } ```
https://godbolt.org/z/sMb8Ebv9j
The text was updated successfully, but these errors were encountered: