[clang] [analyzer] Fix crash in CStringChecker on zero-size element types (PR #191061)

[via cfe-commits]

Endre =?utf-8?q?Fülöp?= <endre.fulop at sigmatechnology.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/191061 at github.com>


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Endre Fülöp (gamesh411)

<details>
<summary>Changes</summary>

Move the null check of Offset before its dereference in checkInit. When the element type has zero size (e.g., an empty struct in C), the division returns an empty optional, which was dereferenced unconditionally.

Fixes #<!-- -->190457

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


3 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp (+3-3) 
- (modified) clang/test/Analysis/bstring.c (+14) 
- (modified) clang/test/Analysis/bstring.cpp (+9) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index fbb47348db55b..e486787bd8d7f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -507,13 +507,13 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C,
                       IdxTy)
           .getAs<NonLoc>();
 
+  if (!Offset)
+    return State;
+
   // Retrieve the index of the last element.
   const NonLoc One = SVB.makeIntVal(1, IdxTy).castAs<NonLoc>();
   SVal LastIdx = SVB.evalBinOpNN(State, BO_Sub, *Offset, One, IdxTy);
 
-  if (!Offset)
-    return State;
-
   SVal LastElementVal =
       State->getLValue(ElemTy, LastIdx, loc::MemRegionVal(SuperR));
   if (!isa<Loc>(LastElementVal))
diff --git a/clang/test/Analysis/bstring.c b/clang/test/Analysis/bstring.c
index 01f85cecfbf43..f343aaec43307 100644
--- a/clang/test/Analysis/bstring.c
+++ b/clang/test/Analysis/bstring.c
@@ -530,3 +530,17 @@ void nocrash_on_locint_offset(void *addr, void* from, struct S s) {
   size_t iAdd = (size_t) addr;
   memcpy(((void *) &(s.f)), from, iAdd);
 }
+
+// PR#190457 - Crash on memcpy with zero-size element type (empty struct).
+// In the GNU C extension, empty structs have sizeof == 0, which caused a
+// division by zero in checkInit. On MSVC targets, even in C mode, empty
+// structs have nonzero sizeof (due to ABI requirements), so the overflow
+// warnings don't fire there.
+void nocrash_on_empty_struct_memcpy(void) {
+  struct {} a[10];
+  __builtin_memcpy(&a[2], a, 2); // no-crash
+#if !defined(_WIN32)
+  // expected-warning at -2 {{'memcpy' will always overflow; destination buffer has size 0, but size argument is 2}}
+  // expected-warning at -3 {{Memory copy function overflows the destination buffer}}
+#endif
+}
diff --git a/clang/test/Analysis/bstring.cpp b/clang/test/Analysis/bstring.cpp
index 9f044c6453739..2f1712648d8e1 100644
--- a/clang/test/Analysis/bstring.cpp
+++ b/clang/test/Analysis/bstring.cpp
@@ -248,3 +248,12 @@ void memmove_uninit_without_outofbound() {
   memmove(dst, src, sizeof(src)); // uninit-warning{{The first element of the 2nd argument is undefined}}
                                   // uninit-note at -1{{Other elements might also be undefined}}
 }
+
+// #190457 - In C++ the sizeof of an empty struct is 1, so this should not
+// crash and should not warn about overflow (unlike the C case where it is 0
+// with the GNU extension).
+void nocrash_on_empty_struct_memcpy_cpp() {
+  struct {} a[10];
+  __builtin_memcpy(&a[2], a, 2); // should not crash
+  // no-warning
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/191061