Skip to content

[analyzer] Fix false positive for stack-addr leak on simple param ptr #107003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

necto
Copy link
Contributor

@necto necto commented Sep 2, 2024

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

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 llvm#106834
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Sep 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 2, 2024

@llvm/pr-subscribers-clang

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

Author: Arseniy Zaostrovnykh (necto)

Changes

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


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

2 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp (+2)
  • (modified) clang/test/Analysis/stack-addr-ps.cpp (+27)
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<StackArgumentsSpaceRegion>(ReferrerMemSpace) &&
+          // Not a simple ptr (int*) but something deeper, e.g. int**
+          isa<SymbolicRegion>(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

@necto necto requested a review from steakhal September 3, 2024 02:21
Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

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

You nailed the tests. You inspired one more test, and that's it I think.

Once you added that, lets merge this.

@steakhal steakhal merged commit aa4f81e into llvm:main Sep 3, 2024
8 checks passed
@necto necto deleted the az/stack-addr-fp branch September 3, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clang-analyzer-core.StackAddressEscape false-positive
3 participants