From 1b439659a8407f469dd932814df15244dee254d2 Mon Sep 17 00:00:00 2001 From: Kristof Umann Date: Tue, 3 Sep 2019 17:57:01 +0000 Subject: [PATCH] [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message There are some functions which can't be given a null pointer as parameter either because it has a nonnull attribute or it is declared to have undefined behavior (e.g. strcmp()). Sometimes it is hard to determine from the checker message which parameter is null at the invocation, so now this information is included in the message. This commit fixes https://bugs.llvm.org/show_bug.cgi?id=39358 Reviewed By: NoQ, Szelethus, whisperity Patch by Tibor Brunner! Differential Revision: https://reviews.llvm.org/D66333 llvm-svn: 370798 --- .../Checkers/CStringChecker.cpp | 40 ++++++++++--------- .../Checkers/NonNullParamChecker.cpp | 19 ++++++--- .../Inputs/expected-plists/edges-new.mm.plist | 6 +-- .../expected-plists/plist-output.m.plist | 6 +-- clang/test/Analysis/misc-ps-region-store.m | 2 +- clang/test/Analysis/null-deref-ps.c | 6 +-- 6 files changed, 45 insertions(+), 34 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index bc2b7ccbbf9a..6f1b9c3979b5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -198,7 +198,8 @@ public: ProgramStateRef checkNonNull(CheckerContext &C, ProgramStateRef state, const Expr *S, - SVal l) const; + SVal l, + unsigned IdxOfArg) const; ProgramStateRef CheckLocation(CheckerContext &C, ProgramStateRef state, const Expr *S, @@ -277,7 +278,8 @@ CStringChecker::assumeZero(CheckerContext &C, ProgramStateRef state, SVal V, ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C, ProgramStateRef state, - const Expr *S, SVal l) const { + const Expr *S, SVal l, + unsigned IdxOfArg) const { // If a previous check has failed, propagate the failure. if (!state) return nullptr; @@ -288,11 +290,13 @@ ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C, if (stateNull && !stateNonNull) { if (Filter.CheckCStringNullArg) { SmallString<80> buf; - llvm::raw_svector_ostream os(buf); + llvm::raw_svector_ostream OS(buf); assert(CurrentFunctionDescription); - os << "Null pointer argument in call to " << CurrentFunctionDescription; + OS << "Null pointer argument in call to " << CurrentFunctionDescription + << ' ' << IdxOfArg << llvm::getOrdinalSuffix(IdxOfArg) + << " parameter"; - emitNullArgBug(C, stateNull, S, os.str()); + emitNullArgBug(C, stateNull, S, OS.str()); } return nullptr; } @@ -384,7 +388,7 @@ ProgramStateRef CStringChecker::CheckBufferAccess(CheckerContext &C, // Check that the first buffer is non-null. SVal BufVal = C.getSVal(FirstBuf); - state = checkNonNull(C, state, FirstBuf, BufVal); + state = checkNonNull(C, state, FirstBuf, BufVal, 1); if (!state) return nullptr; @@ -424,7 +428,7 @@ ProgramStateRef CStringChecker::CheckBufferAccess(CheckerContext &C, // If there's a second buffer, check it as well. if (SecondBuf) { BufVal = state->getSVal(SecondBuf, LCtx); - state = checkNonNull(C, state, SecondBuf, BufVal); + state = checkNonNull(C, state, SecondBuf, BufVal, 2); if (!state) return nullptr; @@ -1163,7 +1167,7 @@ void CStringChecker::evalCopyCommon(CheckerContext &C, // Ensure the destination is not null. If it is NULL there will be a // NULL pointer dereference. - state = checkNonNull(C, state, Dest, destVal); + state = checkNonNull(C, state, Dest, destVal, 1); if (!state) return; @@ -1172,7 +1176,7 @@ void CStringChecker::evalCopyCommon(CheckerContext &C, // Ensure the source is not null. If it is NULL there will be a // NULL pointer dereference. - state = checkNonNull(C, state, Source, srcVal); + state = checkNonNull(C, state, Source, srcVal, 2); if (!state) return; @@ -1383,7 +1387,7 @@ void CStringChecker::evalstrLengthCommon(CheckerContext &C, const CallExpr *CE, const Expr *Arg = CE->getArg(0); SVal ArgVal = state->getSVal(Arg, LCtx); - state = checkNonNull(C, state, Arg, ArgVal); + state = checkNonNull(C, state, Arg, ArgVal, 1); if (!state) return; @@ -1541,14 +1545,14 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, const Expr *Dst = CE->getArg(0); SVal DstVal = state->getSVal(Dst, LCtx); - state = checkNonNull(C, state, Dst, DstVal); + state = checkNonNull(C, state, Dst, DstVal, 1); if (!state) return; // Check that the source is non-null. const Expr *srcExpr = CE->getArg(1); SVal srcVal = state->getSVal(srcExpr, LCtx); - state = checkNonNull(C, state, srcExpr, srcVal); + state = checkNonNull(C, state, srcExpr, srcVal, 2); if (!state) return; @@ -1904,14 +1908,14 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE, // Check that the first string is non-null const Expr *s1 = CE->getArg(0); SVal s1Val = state->getSVal(s1, LCtx); - state = checkNonNull(C, state, s1, s1Val); + state = checkNonNull(C, state, s1, s1Val, 1); if (!state) return; // Check that the second string is non-null. const Expr *s2 = CE->getArg(1); SVal s2Val = state->getSVal(s2, LCtx); - state = checkNonNull(C, state, s2, s2Val); + state = checkNonNull(C, state, s2, s2Val, 2); if (!state) return; @@ -2038,14 +2042,14 @@ void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const { // Check that the search string pointer is non-null (though it may point to // a null string). SVal SearchStrVal = State->getSVal(SearchStrPtr, LCtx); - State = checkNonNull(C, State, SearchStrPtr, SearchStrVal); + State = checkNonNull(C, State, SearchStrPtr, SearchStrVal, 1); if (!State) return; // Check that the delimiter string is non-null. const Expr *DelimStr = CE->getArg(1); SVal DelimStrVal = State->getSVal(DelimStr, LCtx); - State = checkNonNull(C, State, DelimStr, DelimStrVal); + State = checkNonNull(C, State, DelimStr, DelimStrVal, 2); if (!State) return; @@ -2148,7 +2152,7 @@ void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const { // Ensure the memory area is not null. // If it is NULL there will be a NULL pointer dereference. - State = checkNonNull(C, StateNonZeroSize, Mem, MemVal); + State = checkNonNull(C, StateNonZeroSize, Mem, MemVal, 1); if (!State) return; @@ -2195,7 +2199,7 @@ void CStringChecker::evalBzero(CheckerContext &C, const CallExpr *CE) const { // Ensure the memory area is not null. // If it is NULL there will be a NULL pointer dereference. - State = checkNonNull(C, StateNonZeroSize, Mem, MemVal); + State = checkNonNull(C, StateNonZeroSize, Mem, MemVal, 1); if (!State) return; diff --git a/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp index 18ba3f7fb15d..cd8aa03f9e55 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp @@ -36,7 +36,9 @@ public: void checkPreCall(const CallEvent &Call, CheckerContext &C) const; std::unique_ptr - genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE) const; + genReportNullAttrNonNull(const ExplodedNode *ErrorN, + const Expr *ArgE, + unsigned IdxOfArg) const; std::unique_ptr genReportReferenceToNullPointer(const ExplodedNode *ErrorN, const Expr *ArgE) const; @@ -143,7 +145,7 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call, std::unique_ptr R; if (haveAttrNonNull) - R = genReportNullAttrNonNull(errorNode, ArgE); + R = genReportNullAttrNonNull(errorNode, ArgE, idx + 1); else if (haveRefTypeParam) R = genReportReferenceToNullPointer(errorNode, ArgE); @@ -179,7 +181,8 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call, std::unique_ptr NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode, - const Expr *ArgE) const { + const Expr *ArgE, + unsigned IdxOfArg) const { // Lazily allocate the BugType object if it hasn't already been // created. Ownership is transferred to the BugReporter object once // the BugReport is passed to 'EmitWarning'. @@ -187,9 +190,13 @@ NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode, BTAttrNonNull.reset(new BugType( this, "Argument with 'nonnull' attribute passed null", "API")); - auto R = std::make_unique( - *BTAttrNonNull, - "Null pointer passed as an argument to a 'nonnull' parameter", ErrorNode); + llvm::SmallString<256> SBuf; + llvm::raw_svector_ostream OS(SBuf); + OS << "Null pointer passed to " + << IdxOfArg << llvm::getOrdinalSuffix(IdxOfArg) + << " parameter expecting 'nonnull'"; + + auto R = llvm::make_unique(*BTAttrNonNull, SBuf, ErrorNode); if (ArgE) bugreporter::trackExpressionValue(ErrorNode, ArgE, *R); diff --git a/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist b/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist index ab04dadbf479..b949e20ebbe8 100644 --- a/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist +++ b/clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist @@ -10853,12 +10853,12 @@ depth0 extended_message - Null pointer passed as an argument to a 'nonnull' parameter + Null pointer passed to 1st parameter expecting 'nonnull' message - Null pointer passed as an argument to a 'nonnull' parameter + Null pointer passed to 1st parameter expecting 'nonnull' - descriptionNull pointer passed as an argument to a 'nonnull' parameter + descriptionNull pointer passed to 1st parameter expecting 'nonnull' categoryAPI typeArgument with 'nonnull' attribute passed null check_namecore.NonNullParamChecker diff --git a/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist b/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist index ee8bf06e59fd..9203e48c4683 100644 --- a/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist +++ b/clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist @@ -6141,12 +6141,12 @@ depth0 extended_message - Null pointer passed as an argument to a 'nonnull' parameter + Null pointer passed to 1st parameter expecting 'nonnull' message - Null pointer passed as an argument to a 'nonnull' parameter + Null pointer passed to 1st parameter expecting 'nonnull' - descriptionNull pointer passed as an argument to a 'nonnull' parameter + descriptionNull pointer passed to 1st parameter expecting 'nonnull' categoryAPI typeArgument with 'nonnull' attribute passed null check_namecore.NonNullParamChecker diff --git a/clang/test/Analysis/misc-ps-region-store.m b/clang/test/Analysis/misc-ps-region-store.m index 1ef100563126..d1011bda1615 100644 --- a/clang/test/Analysis/misc-ps-region-store.m +++ b/clang/test/Analysis/misc-ps-region-store.m @@ -1205,7 +1205,7 @@ void rdar_8642434_funcA(rdar_8642434_typeB object); void rdar_8642434_funcB(struct rdar_8642434_typeA *x, struct rdar_8642434_typeA *y) { rdar_8642434_funcA(x); if (!y) - rdar_8642434_funcA(y); // expected-warning{{Null pointer passed as an argument to a 'nonnull' parameter}} + rdar_8642434_funcA(y); // expected-warning{{Null pointer passed to 1st parameter expecting 'nonnull'}} } // - Handle loads and stores from a symbolic index diff --git a/clang/test/Analysis/null-deref-ps.c b/clang/test/Analysis/null-deref-ps.c index d0e1f9f5cc33..d1c19e533deb 100644 --- a/clang/test/Analysis/null-deref-ps.c +++ b/clang/test/Analysis/null-deref-ps.c @@ -88,21 +88,21 @@ int f5() { int bar(int* p, int q) __attribute__((nonnull)); int f6(int *p) { - return !p ? bar(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}} + return !p ? bar(p, 1) // expected-warning {{Null pointer passed to 1st parameter expecting 'nonnull'}} : bar(p, 0); // no-warning } int bar2(int* p, int q) __attribute__((nonnull(1))); int f6b(int *p) { - return !p ? bar2(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}} + return !p ? bar2(p, 1) // expected-warning {{Null pointer passed to 1st parameter expecting 'nonnull'}} : bar2(p, 0); // no-warning } int bar3(int*p, int q, int *r) __attribute__((nonnull(1,3))); int f6c(int *p, int *q) { - return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}} + return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed to 3rd parameter expecting 'nonnull'}} : bar3(p, 2, q); // no-warning }