vlad.tsyrklevich created this revision. vlad.tsyrklevich added reviewers: cfe-commits, zaks.anna, dcoughlin, NoQ.
CStringChecker assumes that SVals are not undefined at two points with comments stating that other checkers will check for that condition first; however, it can crash if a user chooses a particular configuration. I hit such an unlucky configuration while eliminating false positive heavy checks analyzing the Linux kernel and tracked down the crashes to this assumption. Add UndefinedVal checks where appropriate. https://reviews.llvm.org/D28765 Files: lib/StaticAnalyzer/Checkers/CStringChecker.cpp test/Analysis/cstring-regressions.c Index: test/Analysis/cstring-regressions.c =================================================================== --- /dev/null +++ test/Analysis/cstring-regressions.c @@ -0,0 +1,18 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.cstring,debug.ExprInspection -analyzer-disable-checker=core.UndefinedBinaryOperatorResult,core.CallAndMessage,core.NullDereference,core.uninitialized -analyzer-store=region -verify %s +// +// CStringChecker crashes found running analyzer over the Linux kernel source +// with a very particular set of enabled/disabled checks. + +int strcmp(const char * s1, const char * s2); +int memcmp(const void *s1, const void *s2, unsigned long n); +void clang_analyzer_eval(int); + +void test1(void *ptr) { + int uninit; + clang_analyzer_eval(strcmp(ptr + uninit, "mcp_trace_meta") == 0); // expected-warning{{UNKNOWN}} +} + +void test2(char *foo, void *ptr) { + int uninit; + clang_analyzer_eval(memcmp(foo, ptr + uninit, 48) == 0); // expected-warning{{UNKNOWN}} +} Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -1179,15 +1179,17 @@ if (stateNonZeroSize) { state = stateNonZeroSize; // If we know the two buffers are the same, we know the result is 0. - // First, get the two buffers' addresses. Another checker will have already - // made sure they're not undefined. - DefinedOrUnknownSVal LV = - state->getSVal(Left, LCtx).castAs<DefinedOrUnknownSVal>(); - DefinedOrUnknownSVal RV = - state->getSVal(Right, LCtx).castAs<DefinedOrUnknownSVal>(); + // First, get the two buffers' addresses. + Optional<DefinedOrUnknownSVal> LV = + state->getSVal(Left, LCtx).getAs<DefinedOrUnknownSVal>(); + Optional<DefinedOrUnknownSVal> RV = + state->getSVal(Right, LCtx).getAs<DefinedOrUnknownSVal>(); + + if (!LV || !RV) + return; // See if they are the same. - DefinedOrUnknownSVal SameBuf = svalBuilder.evalEQ(state, LV, RV); + DefinedOrUnknownSVal SameBuf = svalBuilder.evalEQ(state, *LV, *RV); ProgramStateRef StSameBuf, StNotSameBuf; std::tie(StSameBuf, StNotSameBuf) = state->assume(SameBuf); @@ -1781,13 +1783,19 @@ // Check that the first string is non-null const Expr *s1 = CE->getArg(0); SVal s1Val = state->getSVal(s1, LCtx); + if (s1Val.isUndef()) + return; + state = checkNonNull(C, state, s1, s1Val); if (!state) return; // Check that the second string is non-null. const Expr *s2 = CE->getArg(1); SVal s2Val = state->getSVal(s2, LCtx); + if (s2Val.isUndef()) + return; + state = checkNonNull(C, state, s2, s2Val); if (!state) return; @@ -1803,8 +1811,7 @@ return; // If we know the two buffers are the same, we know the result is 0. - // First, get the two buffers' addresses. Another checker will have already - // made sure they're not undefined. + // First, get the two buffers' addresses. DefinedOrUnknownSVal LV = s1Val.castAs<DefinedOrUnknownSVal>(); DefinedOrUnknownSVal RV = s2Val.castAs<DefinedOrUnknownSVal>();
Index: test/Analysis/cstring-regressions.c =================================================================== --- /dev/null +++ test/Analysis/cstring-regressions.c @@ -0,0 +1,18 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.cstring,debug.ExprInspection -analyzer-disable-checker=core.UndefinedBinaryOperatorResult,core.CallAndMessage,core.NullDereference,core.uninitialized -analyzer-store=region -verify %s +// +// CStringChecker crashes found running analyzer over the Linux kernel source +// with a very particular set of enabled/disabled checks. + +int strcmp(const char * s1, const char * s2); +int memcmp(const void *s1, const void *s2, unsigned long n); +void clang_analyzer_eval(int); + +void test1(void *ptr) { + int uninit; + clang_analyzer_eval(strcmp(ptr + uninit, "mcp_trace_meta") == 0); // expected-warning{{UNKNOWN}} +} + +void test2(char *foo, void *ptr) { + int uninit; + clang_analyzer_eval(memcmp(foo, ptr + uninit, 48) == 0); // expected-warning{{UNKNOWN}} +} Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -1179,15 +1179,17 @@ if (stateNonZeroSize) { state = stateNonZeroSize; // If we know the two buffers are the same, we know the result is 0. - // First, get the two buffers' addresses. Another checker will have already - // made sure they're not undefined. - DefinedOrUnknownSVal LV = - state->getSVal(Left, LCtx).castAs<DefinedOrUnknownSVal>(); - DefinedOrUnknownSVal RV = - state->getSVal(Right, LCtx).castAs<DefinedOrUnknownSVal>(); + // First, get the two buffers' addresses. + Optional<DefinedOrUnknownSVal> LV = + state->getSVal(Left, LCtx).getAs<DefinedOrUnknownSVal>(); + Optional<DefinedOrUnknownSVal> RV = + state->getSVal(Right, LCtx).getAs<DefinedOrUnknownSVal>(); + + if (!LV || !RV) + return; // See if they are the same. - DefinedOrUnknownSVal SameBuf = svalBuilder.evalEQ(state, LV, RV); + DefinedOrUnknownSVal SameBuf = svalBuilder.evalEQ(state, *LV, *RV); ProgramStateRef StSameBuf, StNotSameBuf; std::tie(StSameBuf, StNotSameBuf) = state->assume(SameBuf); @@ -1781,13 +1783,19 @@ // Check that the first string is non-null const Expr *s1 = CE->getArg(0); SVal s1Val = state->getSVal(s1, LCtx); + if (s1Val.isUndef()) + return; + state = checkNonNull(C, state, s1, s1Val); if (!state) return; // Check that the second string is non-null. const Expr *s2 = CE->getArg(1); SVal s2Val = state->getSVal(s2, LCtx); + if (s2Val.isUndef()) + return; + state = checkNonNull(C, state, s2, s2Val); if (!state) return; @@ -1803,8 +1811,7 @@ return; // If we know the two buffers are the same, we know the result is 0. - // First, get the two buffers' addresses. Another checker will have already - // made sure they're not undefined. + // First, get the two buffers' addresses. DefinedOrUnknownSVal LV = s1Val.castAs<DefinedOrUnknownSVal>(); DefinedOrUnknownSVal RV = s2Val.castAs<DefinedOrUnknownSVal>();
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits