NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus,
baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, szepet.
Herald added a project: clang.
NoQ added a parent revision: D71321: [analyzer] CStringChecker: Warning text
fixes..
While analyzing code
memcmp(a, NULL, n);
where `a` has an unconstrained symbolic value, the analyzer is currently
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 that 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.
This could have been fixed by reordering the checks (first check null
arguments, then check for overlap) but i went a bit further and entirely
removed the state split for "same buffer vs. different buffers". This state
split is not well-justified: we cannot deduce from the presence of `memcpy` in
the code that the buffers may in fact overlap. Instead, i conservatively assume
that they don't overlap, unless they're already known to certainly overlap on
the current execution path. This loses a bit of coverage but the lost path is
where they do actually overlap. This path is in my opinion not only rare but
also fairly useless, as it immediately introduces an aliasing problem that we
aren't quite ready to deal with.
Repository:
rC Clang
https://reviews.llvm.org/D71322
Files:
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/test/Analysis/bstring.c
Index: clang/test/Analysis/bstring.c
===================================================================
--- clang/test/Analysis/bstring.c
+++ clang/test/Analysis/bstring.c
@@ -462,6 +462,12 @@
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()
//===----------------------------------------------------------------------===
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1313,9 +1313,9 @@
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 @@
svalBuilder.makeZeroVal(CE->getType()));
C.addTransition(state);
}
+ return;
}
- // If the two arguments might be different 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 different 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);
}
}
}
Index: clang/test/Analysis/bstring.c
===================================================================
--- clang/test/Analysis/bstring.c
+++ clang/test/Analysis/bstring.c
@@ -462,6 +462,12 @@
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()
//===----------------------------------------------------------------------===
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1313,9 +1313,9 @@
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 @@
svalBuilder.makeZeroVal(CE->getType()));
C.addTransition(state);
}
+ return;
}
- // If the two arguments might be different 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 different 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);
}
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
