[clang] fix wrong result of pointers comparison between unknown and stack

[mzyKi]

Related Issue #122403

[llvmbot]

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

@llvm/pr-subscribers-clang

Author: Exile (mzyKi)

Changes

Related Issue #122403

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp (+6)
• (added) clang/test/Analysis/stream_issue122403.c (+48)

diff --git a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
index 455621739f6935..1fb51ef403fa12 100644
--- a/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -952,6 +952,12 @@ SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state,
     const MemSpaceRegion *RightMS = RightBase->getMemorySpace();
     const MemSpaceRegion *UnknownMS = MemMgr.getUnknownRegion();
 
+    if (LeftMS != RightMS &&
+        ((isa<UnknownSpaceRegion>(LeftMS) && isa<StackSpaceRegion>(RightMS)) ||
+         (isa<StackSpaceRegion>(LeftMS) && isa<UnknownSpaceRegion>(RightMS)))) {
+      return UnknownVal();
+    }
+
     // If the two regions are from different known memory spaces they cannot be
     // equal. Also, assume that no symbolic region (whose memory space is
     // unknown) is on the stack.
diff --git a/clang/test/Analysis/stream_issue122403.c b/clang/test/Analysis/stream_issue122403.c
new file mode 100644
index 00000000000000..b9582a1cf7e95e
--- /dev/null
+++ b/clang/test/Analysis/stream_issue122403.c
@@ -0,0 +1,48 @@
+// RUN: %clang_analyze_cc1 -triple=x86_64-pc-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
+// RUN: %clang_analyze_cc1 -triple=armv8-none-linux-eabi -analyzer-checker=core,unix.Stream,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
+// RUN: %clang_analyze_cc1 -triple=aarch64-linux-gnu -analyzer-checker=core,unix.Stream,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
+// RUN: %clang_analyze_cc1 -triple=hexagon -analyzer-checker=core,unix.Stream,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false,unix.Stream:Pedantic=true -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+char *get_str(char *Input);
+
+void check_f_leak() {
+  FILE *fp = fopen("test", "rb");
+  if (NULL == fp) {
+    return;
+  }
+  char str[64];
+  if (get_str(str) != str) {
+    fclose(fp);
+  }
+}// expected-warning {{Opened stream never closed. Potential resource leak}}
+
+void check_f_leak_2() {
+  FILE *fp = fopen("test", "rb");
+  if (NULL == fp) {
+    return;
+  }
+  char str[64];
+  if (get_str(str) != NULL) {
+    fclose(fp);
+  }
+}// expected-warning {{Opened stream never closed. Potential resource leak}}
+
+
+char *get_str_other(char *Input) {return Input;}
+
+void check_f_leak_3() {
+  FILE *fp = fopen("test", "rb");
+  if (NULL == fp) {
+    return;
+  }
+  char str[64];
+  if (get_str_other(str) != str) {
+    fclose(fp);
+  }
+}// expected-warning {{Opened stream never closed. Potential resource leak}}
\ No newline at end of file

[steakhal]

In general, the concept of having the memory space directly embedded into a memory region is flawed. And that is the root cause of the problem.

Ideally, the property of "in what memory space does this region live in" is a trait, that could be changed as we learn about aliasing properties in the program.
As such, we should be able to say, we didn't know exactly what is the memory space of a symbol, but now that we learned that this is equal to some other memory region that lives on the stack, we must also live on the stack.

So ideally, we would purge the whole memoryspace portion of the memregion class hierarchy.

As an easier workaround, we could probably get away with having this program trait defined only for memregions that have UnknownMemorySpace. This way we could fix these issues while saving some work of completely rewriting the whole memregion class hierarchy. (I tried that, and it's not trivial.)

@Flandini could you please chim in and collaborate with him/her?

[steakhal]

One usually calls this identity.

[steakhal]

This is dead code.

[steakhal]

Does it matter if this takes any parameters? I think we are better off not having any parameters, because those would be just confusing at the call site.

[steakhal]

In the current form, it's likely that the subsequent check could be simplified, that checks something really similar like this one.

Please consider hoisting common subexpressions, and simplifying the subsequent check as well.