ASDenysPetrov updated this revision to Diff 272997.
ASDenysPetrov added a comment.
@NoQ thanks for the approval. But I'm afraid we should return the check for
`!V.getAs<nonloc::LazyCompoundVal>()` back in `CStringChecker::assumeZero`.
Look, I paid attention to the fix for https://llvm.org/PR24951 and related
commit rG480a0c00ca51d909a925120737b71289cbb79eda
<https://reviews.llvm.org/rG480a0c00ca51d909a925120737b71289cbb79eda>. I saw
that @dcoughlin added a check for `V.getAs<nonloc::LazyCompoundVal>()` to fix
and the old code used it implicitly. My patch returns this issue back. That's
why we need to add the check for `LazyCompoundVal` as well.
Another big question is why `LazyCompoundVal` can be possible here at all. As I
see according to the graph below the answer is somewhere deep in the core. Thus
I'd propose to apply this patch in scope of fix https://llvm.org/PR37503.
F11723847: tmp63ucwe5i.html <https://reviews.llvm.org/F11723847>
I updated the patch. I've also added a //FIXME// caveat.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77062/new/
https://reviews.llvm.org/D77062
Files:
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/test/Analysis/string.c
Index: clang/test/Analysis/string.c
===================================================================
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -363,6 +363,14 @@
strcpy(x, y); // no-warning
}
+void *func_strcpy_no_assertion();
+char ***ptr_strcpy_no_assertion;
+void strcpy_no_assertion() {
+ *(unsigned char **)ptr_strcpy_no_assertion = (unsigned char *)(func_strcpy_no_assertion());
+ char c;
+ strcpy(**ptr_strcpy_no_assertion, &c); // no-assertion
+}
+
//===----------------------------------------------------------------------===
// stpcpy()
//===----------------------------------------------------------------------===
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -196,9 +196,8 @@
void evalBzero(CheckerContext &C, const CallExpr *CE) const;
// Utility methods
- std::pair<ProgramStateRef , ProgramStateRef >
- static assumeZero(CheckerContext &C,
- ProgramStateRef state, SVal V, QualType Ty);
+ std::pair<ProgramStateRef, ProgramStateRef> static assumeZero(
+ ProgramStateRef state, SVal V);
static ProgramStateRef setCStringLength(ProgramStateRef state,
const MemRegion *MR,
@@ -279,16 +278,18 @@
// Individual checks and utility methods.
//===----------------------------------------------------------------------===//
-std::pair<ProgramStateRef , ProgramStateRef >
-CStringChecker::assumeZero(CheckerContext &C, ProgramStateRef state, SVal V,
- QualType Ty) {
+std::pair<ProgramStateRef, ProgramStateRef>
+CStringChecker::assumeZero(ProgramStateRef state, SVal V) {
+ auto states = std::make_pair(state, state);
+
Optional<DefinedSVal> val = V.getAs<DefinedSVal>();
- if (!val)
- return std::pair<ProgramStateRef , ProgramStateRef >(state, state);
+ // FIXME: We should understand how LazyCompoundVal can be possible here,
+ // fix the root cause and get rid of this check.
+ if (val && !V.getAs<nonloc::LazyCompoundVal>())
+ // Returned pair shall be {null, non-null} so reorder states.
+ std::tie(states.second, states.first) = state->assume(*val);
- SValBuilder &svalBuilder = C.getSValBuilder();
- DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
- return state->assume(svalBuilder.evalEQ(state, *val, zero));
+ return states;
}
ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C,
@@ -299,8 +300,7 @@
return nullptr;
ProgramStateRef stateNull, stateNonNull;
- std::tie(stateNull, stateNonNull) =
- assumeZero(C, State, l, Arg.Expression->getType());
+ std::tie(stateNull, stateNonNull) = assumeZero(State, l);
if (stateNull && !stateNonNull) {
if (Filter.CheckCStringNullArg) {
@@ -1071,8 +1071,7 @@
CharVal = svalBuilder.evalCast(CharVal, Ctx.UnsignedCharTy, Ctx.IntTy);
ProgramStateRef StateNullChar, StateNonNullChar;
- std::tie(StateNullChar, StateNonNullChar) =
- assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+ std::tie(StateNullChar, StateNonNullChar) = assumeZero(State, CharVal);
if (StateWholeReg && !StateNotWholeReg && StateNullChar &&
!StateNonNullChar) {
@@ -1133,11 +1132,9 @@
// See if the size argument is zero.
const LocationContext *LCtx = C.getLocationContext();
SVal sizeVal = state->getSVal(Size.Expression, LCtx);
- QualType sizeTy = Size.Expression->getType();
ProgramStateRef stateZeroSize, stateNonZeroSize;
- std::tie(stateZeroSize, stateNonZeroSize) =
- assumeZero(C, state, sizeVal, sizeTy);
+ std::tie(stateZeroSize, stateNonZeroSize) = assumeZero(state, sizeVal);
// Get the value of the Dest.
SVal destVal = state->getSVal(Dest.Expression, LCtx);
@@ -1287,11 +1284,9 @@
// See if the size argument is zero.
SVal sizeVal = State->getSVal(Size.Expression, LCtx);
- QualType sizeTy = Size.Expression->getType();
ProgramStateRef stateZeroSize, stateNonZeroSize;
- std::tie(stateZeroSize, stateNonZeroSize) =
- assumeZero(C, State, sizeVal, sizeTy);
+ std::tie(stateZeroSize, stateNonZeroSize) = assumeZero(State, sizeVal);
// If the size can be zero, the result will be 0 in that case, and we don't
// have to check either of the buffers.
@@ -1367,8 +1362,7 @@
SVal maxlenVal = state->getSVal(maxlenExpr, LCtx);
ProgramStateRef stateZeroSize, stateNonZeroSize;
- std::tie(stateZeroSize, stateNonZeroSize) =
- assumeZero(C, state, maxlenVal, maxlenExpr->getType());
+ std::tie(stateZeroSize, stateNonZeroSize) = assumeZero(state, maxlenVal);
// If the size can be zero, the result will be 0 in that case, and we don't
// have to check the string itself.
@@ -1706,7 +1700,7 @@
// as the last element accessed, so n == 0 is problematic.
ProgramStateRef StateZeroSize, StateNonZeroSize;
std::tie(StateZeroSize, StateNonZeroSize) =
- assumeZero(C, state, *lenValNL, sizeTy);
+ assumeZero(state, *lenValNL);
// If the size is known to be zero, we're done.
if (StateZeroSize && !StateNonZeroSize) {
@@ -2177,10 +2171,9 @@
// See if the size argument is zero.
const LocationContext *LCtx = C.getLocationContext();
SVal SizeVal = C.getSVal(Size.Expression);
- QualType SizeTy = Size.Expression->getType();
ProgramStateRef ZeroSize, NonZeroSize;
- std::tie(ZeroSize, NonZeroSize) = assumeZero(C, State, SizeVal, SizeTy);
+ std::tie(ZeroSize, NonZeroSize) = assumeZero(State, SizeVal);
// Get the value of the memory area.
SVal BufferPtrVal = C.getSVal(Buffer.Expression);
@@ -2225,11 +2218,9 @@
// See if the size argument is zero.
SVal SizeVal = C.getSVal(Size.Expression);
- QualType SizeTy = Size.Expression->getType();
ProgramStateRef StateZeroSize, StateNonZeroSize;
- std::tie(StateZeroSize, StateNonZeroSize) =
- assumeZero(C, State, SizeVal, SizeTy);
+ std::tie(StateZeroSize, StateNonZeroSize) = assumeZero(State, SizeVal);
// If the size is zero, there won't be any actual memory access,
// In this case we just return.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
