diff --git a/clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp @@ -54,15 +54,23 @@ public: /// Test if an encountered binary operator where the return value is involved /// is a valid check statement. The return value appears in one side of the - /// operator (`ChildIsLHS` indicates if it is on the LHS). If the other side - /// contains a known (mostly constant) value, it is already calculated in - /// `KnownValue`. `RetTy` is the type of the return value (return type of the - /// function call in the code to check). + /// operator (`ChildIsLHS` indicates if it is on the LHS). The other side is + /// the value to test against, its "known value" is passed in + /// `ValueToTestAgainst. `RetTy` is the return type of the function. + // testAppearanceInBinOp virtual bool testBinOpForCheckStatement(BasicValueFactory &BVF, const BinaryOperator *BinOp, - const llvm::APSInt *KnownValue, + const llvm::APSInt *ValueToTestAgainst, QualType RetTy, bool ChildIsLHS) const = 0; + + virtual bool testUnaryOp(BasicValueFactory &BVF, + const UnaryOperator *UnOp, + QualType RetTy) const = 0; + + /// Tell if the bare appearance of the return value in place of a condition (for example 'X' in 'if (X)') can be taken as a check for error return value. + // testAppearanceInCondition + virtual bool testConditionForCheckStatement() const = 0; }; /// Error return is a -1 or any negative value (both is accepted). @@ -73,28 +81,28 @@ public: bool testBinOpForCheckStatement(BasicValueFactory &BVF, const BinaryOperator *BinOp, - const llvm::APSInt *KnownValue, + const llvm::APSInt *ValueToTestAgainst, QualType RetTy, bool ChildIsLHS) const override { - if (!KnownValue) - return false; + if (!ValueToTestAgainst) + return true; - bool KnownNull = KnownValue->isNullValue(); - bool KnownEOF = ((*KnownValue) == BVF.getValue(-1, RetTy)); + bool NullFound = ValueToTestAgainst->isNullValue(); + bool EOFFound = ((*ValueToTestAgainst) == BVF.getValue(-1, RetTy)); if (ChildIsLHS) { switch (BinOp->getOpcode()) { case BO_EQ: // 'X == -1' case BO_NE: // 'X != -1' - return KnownEOF; + return EOFFound; case BO_LT: // 'X < 0' - return KnownNull; + return NullFound; case BO_GE: // 'X >= 0' - return KnownNull; + return NullFound; case BO_LE: // 'X <= -1' - return KnownEOF; + return EOFFound; case BO_GT: // 'X > -1' - return KnownEOF; + return EOFFound; default: return false; } @@ -102,21 +110,77 @@ switch (BinOp->getOpcode()) { case BO_EQ: // '-1 == X' case BO_NE: // '-1 != X' - return KnownEOF; + return EOFFound; case BO_GT: // '0 > X' - return KnownNull; + return NullFound; case BO_LE: // '0 <= X' - return KnownNull; + return NullFound; case BO_GE: // '-1 >= X' - return KnownEOF; + return EOFFound; case BO_LT: // '-1 < X' - return KnownEOF; + return EOFFound; default: return false; } } return false; } + + bool testUnaryOp(BasicValueFactory &BVF, const UnaryOperator *UnOp, + QualType RetTy) const override { + return false; + } + + bool testConditionForCheckStatement() const override { return false; } +}; + +class NullErrorReturn : public ErrorReturnCheckKind { +public: + bool testBinOpForCheckStatement(BasicValueFactory &BVF, + const BinaryOperator *BinOp, + const llvm::APSInt *ValueToTestAgainst, + QualType RetTy, + bool ChildIsLHS) const override { + clang::BinaryOperator::Opcode C = BinOp->getOpcode(); + + if (C == BO_LAnd || C == BO_LOr) + return true; + + if (!ValueToTestAgainst) + return false; + bool NullFound = ValueToTestAgainst->isNullValue(); + + if (C == BO_EQ || C == BO_NE) + return NullFound; + + /*if (ChildIsLHS) { + switch (BinOp->getOpcode()) { + case BO_EQ: // 'X == 0' + case BO_NE: // 'X != 0' + return KnownNull; + default: + return false; + } + } else { + switch (BinOp->getOpcode()) { + case BO_EQ: // '0 == X' + case BO_NE: // '0 != X' + return KnownNull; + default: + return false; + } + }*/ + return false; + } + + bool testUnaryOp(BasicValueFactory &BVF, const UnaryOperator *UnOp, + QualType RetTy) const override { + if (UnOp->getOpcode() == UnaryOperator::Opcode::UO_LNot) + return true; + return false; + } + + bool testConditionForCheckStatement() const override { return true; } }; /// Description of an API function to check. @@ -171,7 +235,7 @@ check::PointerEscape> { mutable std::unique_ptr BT_UncheckedUse; - void checkAccess(CheckerContext &C, ProgramStateRef State, const Stmt *LoadS, + ExplodedNode *checkAccess(CheckerContext &C, ProgramStateRef State, const Stmt *LoadS, SymbolRef CallSym, const CalledFunctionData *CFD) const; ProgramStateRef processEscapedParams(CheckerContext &C, const CallEvent &Call, ProgramStateRef State) const; @@ -190,11 +254,68 @@ private: EOFOrNegativeErrorReturn CheckForEOFOrNegative; + NullErrorReturn CheckForNull; + NullErrorReturn CheckForZero; CallDescriptionMap CheckedFunctions = { {{"fputs", 2}, FnInfo{&CheckForEOFOrNegative}}, {{"fputws", 2}, FnInfo{&CheckForEOFOrNegative}}, + + {{"aligned_alloc", 2}, FnInfo{&CheckForNull}}, + {{"bsearch", 5}, FnInfo{&CheckForNull}}, + {{"bsearch_s", 6}, FnInfo{&CheckForNull}}, + {{"calloc", 2}, FnInfo{&CheckForNull}}, + {{"fgets", 3}, FnInfo{&CheckForNull}}, + {{"fopen", 2}, FnInfo{&CheckForNull}}, + {{"freopen", 3}, FnInfo{&CheckForNull}}, + {{"getenv", 1}, FnInfo{&CheckForNull}}, + {{"getenv_s", 4}, FnInfo{&CheckForNull}}, + {{"gets_s", 2}, FnInfo{&CheckForNull}}, + {{"gmtime", 1}, FnInfo{&CheckForNull}}, + {{"gmtime_s", 2}, FnInfo{&CheckForNull}}, + {{"localtime", 1}, FnInfo{&CheckForNull}}, + {{"localtime_s", 2}, FnInfo{&CheckForNull}}, + {{"malloc", 1}, FnInfo{&CheckForNull}}, + {{"memchr", 3}, FnInfo{&CheckForNull}}, + {{"realloc", 2}, FnInfo{&CheckForNull}}, + {{"setlocale", 2}, FnInfo{&CheckForNull}}, + {{"strchr", 2}, FnInfo{&CheckForNull}}, + {{"strpbrk", 2}, FnInfo{&CheckForNull}}, + {{"strrchr", 2}, FnInfo{&CheckForNull}}, + {{"strstr", 2}, FnInfo{&CheckForNull}}, + {{"strtok", 2}, FnInfo{&CheckForNull}}, + {{"strtok_s", 4}, FnInfo{&CheckForNull}}, + {{"tmpfile", 0}, FnInfo{&CheckForNull}}, + {{"tmpnam", 1}, FnInfo{&CheckForNull}}, + {{"wcschr", 2}, FnInfo{&CheckForNull}}, + {{"wcspbrk", 2}, FnInfo{&CheckForNull}}, + {{"wcsrchr", 2}, FnInfo{&CheckForNull}}, + {{"wcsstr", 2}, FnInfo{&CheckForNull}}, + {{"wcstok", 3}, FnInfo{&CheckForNull}}, + {{"wcstok_s", 4}, FnInfo{&CheckForNull}}, + {{"wmemchr", 3}, FnInfo{&CheckForNull}}, + + {{"asctime_s", 3}, FnInfo{&CheckForZero}}, + {{"at_quick_exit", 1}, FnInfo{&CheckForZero}}, + {{"atexit", 1}, FnInfo{&CheckForZero}}, + {{"ctime_s", 3}, FnInfo{&CheckForZero}}, + {{"fgetpos", 2}, FnInfo{&CheckForZero}}, + {{"fopen_s", 3}, FnInfo{&CheckForZero}}, + {{"freopen_s", 4}, FnInfo{&CheckForZero}}, + {{"fsetpos", 2}, FnInfo{&CheckForZero}}, + {{"raise", 1}, FnInfo{&CheckForZero}}, + {{"remove", 1}, FnInfo{&CheckForZero}}, + {{"rename", 2}, FnInfo{&CheckForZero}}, + {{"setvbuf", 4}, FnInfo{&CheckForZero}}, + {{"strerror_s", 3}, FnInfo{&CheckForZero}}, + {{"strftime", 4}, FnInfo{&CheckForZero}}, + {{"timespec_get", 2}, FnInfo{&CheckForZero}}, + {{"tmpfile_s", 1}, FnInfo{&CheckForZero}}, + {{"tmpnam_s", 2}, FnInfo{&CheckForZero}}, + {{"wctrans", 1}, FnInfo{&CheckForZero}}, + {{"wctype", 1}, FnInfo{&CheckForZero}} }; + }; /// Result of the ErrorCheckTestStmtVisitor. @@ -277,6 +398,20 @@ return NoCheckUseFound; } + VisitResult VisitUnaryOperator(const UnaryOperator *UO) { + if (CFD->Info->ErrorReturnKind->testUnaryOp( + C.getSValBuilder().getBasicValueFactory(), UO, + CFD->Info->RetTy)) + return CheckFound; + return NoCheckUseFound; + } + + VisitResult VisitAbstractConditionalOperator(const AbstractConditionalOperator *ACO) { + if (isChildOf(ACO->getCond())) + return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound; + return ContinueAtParentStmt; + } + VisitResult VisitCallExpr(const CallExpr *CE) { const FunctionDecl *CalledF = C.getCalleeDecl(CE); SourceLocation Loc = CalledF->getLocation(); @@ -302,26 +437,27 @@ VisitResult VisitIfStmt(const IfStmt *S) { if (isChildOf(S->getCond())) - return NoCheckUseFound; + return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound; return StopExamineNoError; } VisitResult VisitForStmt(const ForStmt *S) { - if (isChildOf(S->getInit()) || isChildOf(S->getCond()) || - isChildOf(S->getInc())) - return NoCheckUseFound; +// if (isChildOf(S->getInit()) || isChildOf(S->getInc())) +// return + if (isChildOf(S->getCond())) + return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound; return StopExamineNoError; } VisitResult VisitDoStmt(const DoStmt *S) { if (isChildOf(S->getCond())) - return NoCheckUseFound; + return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound; return StopExamineNoError; } VisitResult VisitWhileStmt(const WhileStmt *S) { if (isChildOf(S->getCond())) - return NoCheckUseFound; + return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound; return StopExamineNoError; } @@ -336,7 +472,7 @@ REGISTER_MAP_WITH_PROGRAMSTATE(CalledFunctionDataMap, SymbolRef, CalledFunctionData) -void ErrorReturnChecker::checkAccess(CheckerContext &C, ProgramStateRef State, +ExplodedNode *ErrorReturnChecker::checkAccess(CheckerContext &C, ProgramStateRef State, const Stmt *LoadS, SymbolRef CallSym, const CalledFunctionData *CFD) const { const ParentMap &PM = C.getLocationContext()->getParentMap(); @@ -359,15 +495,13 @@ SourceRange CallLocation = CFD->CallLocation; State = State->remove(CallSym); - ExplodedNode *N = C.generateNonFatalErrorNode(State); - if (!N) { - C.addTransition(State); - return; - } + ExplodedNode *ErrN = C.generateNonFatalErrorNode(State); + if (!ErrN) + return C.addTransition(State); auto Report = std::make_unique( - *BT_UncheckedUse, BT_UncheckedUse->getDescription(), N); - // Report->markInteresting(CallSym); + *BT_UncheckedUse, BT_UncheckedUse->getDescription(), ErrN); + Report->markInteresting(CallSym); Report->addRange(CallLocation); C.emitReport(std::move(Report)); @@ -375,13 +509,12 @@ //Report->addRange(CallLocation); //C.emitReport(std::move(Report)); - return; + return nullptr; } case CheckFound: // A correct error check was found, remove from state. State = State->remove(CallSym); - C.addTransition(State); - return; + return C.addTransition(State); case ContinueAtParentStmt: // Continue checking at upper level (check with result of assignment). @@ -389,10 +522,10 @@ continue; case StopExamineNoError: - C.addTransition(State); - return; + return C.addTransition(State); }; } + return C.addTransition(State); } ProgramStateRef ErrorReturnChecker::processEscapedParams( @@ -446,6 +579,21 @@ return Fn; } + struct NoteFn { + const CheckerNameRef CheckerName; + SymbolRef StreamSym; + std::string Message; + + std::string operator()(PathSensitiveBugReport &BR) const { + llvm::errs()<<"NoteFn\n"; + if (BR.isInteresting(StreamSym) && + CheckerName == BR.getBugType().getCheckerName()) + return Message; + + return ""; + } + }; + void ErrorReturnChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { ProgramStateRef State = C.getState(); @@ -463,7 +611,14 @@ CalledFunctionData CFD{Fn, Call.getSourceRange()}; State = State->set(RetSym, CFD); - checkAccess(C, State, Call.getOriginExpr(), RetSym, &CFD); + //SmallString<100> buf; + //llvm::raw_svector_ostream os(buf); + //os << "Function \'???\' called here without checked return value"; + ExplodedNode *N = checkAccess(C, State, Call.getOriginExpr(), RetSym, &CFD); + if (!N) + return; + + C.addTransition(N->getState(), C.getNoteTag(NoteFn{getCheckerName(), RetSym, "Function \'???\' called here without checked return value"}, N)); } void ErrorReturnChecker::checkLocation(SVal L, bool IsLoad, const Stmt *S, diff --git a/clang/test/Analysis/error-return.c b/clang/test/Analysis/error-return.c --- a/clang/test/Analysis/error-return.c +++ b/clang/test/Analysis/error-return.c @@ -91,7 +91,8 @@ } void badcheck(int X) { - if (X == 0) { } // expected-warning{{Use of return value that was not checked}} + if (X == 0) { + } // expected-warning{{Use of return value that was not checked}} } void test_EOFOrNeg_Call() {