[clang] 2b3f207 - [analyzer] CStringChecker: Fix overly eager assumption that memcmp args overlap.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 11 11:22:46 PST 2019
Author: Artem Dergachev
Date: 2019-12-11T11:22:36-08:00
New Revision: 2b3f2071ec6561c3f10e5291289c47bb3629e354
URL: https://github.com/llvm/llvm-project/commit/2b3f2071ec6561c3f10e5291289c47bb3629e354
DIFF: https://github.com/llvm/llvm-project/commit/2b3f2071ec6561c3f10e5291289c47bb3629e354.diff
LOG: [analyzer] CStringChecker: Fix overly eager assumption that memcmp args overlap.
While analyzing code `memcmp(a, NULL, n);', where `a' has an unconstrained
symbolic value, the analyzer was emitting a warning about the *first* argument
being a null pointer, even though we'd rather have it warn about the *second*
argument.
This happens because CStringChecker first checks whether the two argument
buffers are in fact the same buffer, in order to take the fast path.
This boils down to assuming `a == NULL' to true. Then the subsequent check
for null pointer argument "discovers" that `a' is null.
Don't take the fast path unless we are *sure* that the buffers are the same.
Otherwise proceed as normal.
Differential Revision: https://reviews.llvm.org/D71322
Added:
Modified:
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/test/Analysis/bstring.c
clang/test/Analysis/string.c
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index c05a09da3df1..4203f790e211 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1313,9 +1313,9 @@ void CStringChecker::evalMemcmp(CheckerContext &C, const CallExpr *CE) const {
ProgramStateRef StSameBuf, StNotSameBuf;
std::tie(StSameBuf, StNotSameBuf) = state->assume(SameBuf);
- // If the two arguments might be the same buffer, we know the result is 0,
+ // If the two arguments are the same buffer, we know the result is 0,
// and we only need to check one size.
- if (StSameBuf) {
+ if (StSameBuf && !StNotSameBuf) {
state = StSameBuf;
state = CheckBufferAccess(C, state, Size, Left);
if (state) {
@@ -1323,20 +1323,19 @@ void CStringChecker::evalMemcmp(CheckerContext &C, const CallExpr *CE) const {
svalBuilder.makeZeroVal(CE->getType()));
C.addTransition(state);
}
+ return;
}
- // If the two arguments might be
diff erent buffers, we have to check the
- // size of both of them.
- if (StNotSameBuf) {
- state = StNotSameBuf;
- state = CheckBufferAccess(C, state, Size, Left, Right);
- if (state) {
- // The return value is the comparison result, which we don't know.
- SVal CmpV = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx,
- C.blockCount());
- state = state->BindExpr(CE, LCtx, CmpV);
- C.addTransition(state);
- }
+ // If the two arguments might be
diff erent buffers, we have to check
+ // the size of both of them.
+ assert(StNotSameBuf);
+ state = CheckBufferAccess(C, state, Size, Left, Right);
+ if (state) {
+ // The return value is the comparison result, which we don't know.
+ SVal CmpV =
+ svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
+ state = state->BindExpr(CE, LCtx, CmpV);
+ C.addTransition(state);
}
}
}
diff --git a/clang/test/Analysis/bstring.c b/clang/test/Analysis/bstring.c
index 8d8f64cebec1..2d53402a9ad3 100644
--- a/clang/test/Analysis/bstring.c
+++ b/clang/test/Analysis/bstring.c
@@ -462,6 +462,12 @@ int memcmp7 (char *a, size_t x, size_t y, size_t n) {
memcmp(&a[x*y], a, n);
}
+int memcmp8(char *a, size_t n) {
+ char *b = 0;
+ // Do not warn about the first argument!
+ return memcmp(a, b, n); // expected-warning{{Null pointer passed as 2nd argument to memory comparison function}}
+}
+
//===----------------------------------------------------------------------===
// bcopy()
//===----------------------------------------------------------------------===
diff --git a/clang/test/Analysis/string.c b/clang/test/Analysis/string.c
index d21ec11c1115..7612614bc091 100644
--- a/clang/test/Analysis/string.c
+++ b/clang/test/Analysis/string.c
@@ -867,6 +867,12 @@ void strcmp_union_function_pointer_cast(union argument a) {
fPtr(&a);
}
+int strcmp_null_argument(char *a) {
+ char *b = 0;
+ // Do not warn about the first argument!
+ return strcmp(a, b); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}
+
//===----------------------------------------------------------------------===
// strncmp()
//===----------------------------------------------------------------------===
@@ -976,6 +982,12 @@ void strncmp_embedded_null () {
clang_analyzer_eval(strncmp("ab\0zz", "ab\0yy", 4) == 0); // expected-warning{{TRUE}}
}
+int strncmp_null_argument(char *a, size_t n) {
+ char *b = 0;
+ // Do not warn about the first argument!
+ return strncmp(a, b, n); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}
+
//===----------------------------------------------------------------------===
// strcasecmp()
//===----------------------------------------------------------------------===
@@ -1067,6 +1079,12 @@ void strcasecmp_embedded_null () {
clang_analyzer_eval(strcasecmp("ab\0zz", "ab\0yy") == 0); // expected-warning{{TRUE}}
}
+int strcasecmp_null_argument(char *a) {
+ char *b = 0;
+ // Do not warn about the first argument!
+ return strcasecmp(a, b); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}
+
//===----------------------------------------------------------------------===
// strncasecmp()
//===----------------------------------------------------------------------===
@@ -1176,6 +1194,12 @@ void strncasecmp_embedded_null () {
clang_analyzer_eval(strncasecmp("ab\0zz", "ab\0yy", 4) == 0); // expected-warning{{TRUE}}
}
+int strncasecmp_null_argument(char *a, size_t n) {
+ char *b = 0;
+ // Do not warn about the first argument!
+ return strncasecmp(a, b, n); // expected-warning{{Null pointer passed as 2nd argument to string comparison function}}
+}
+
//===----------------------------------------------------------------------===
// strsep()
//===----------------------------------------------------------------------===
More information about the cfe-commits
mailing list