pgousseau updated this revision to Diff 37466. pgousseau added a comment. Fix incorrect caching of model files declarations.
http://reviews.llvm.org/D13731 Files: include/clang/Analysis/AnalysisContext.h include/clang/Analysis/CodeInjector.h include/clang/StaticAnalyzer/Frontend/FrontendActions.h include/clang/StaticAnalyzer/Frontend/ModelConsumer.h lib/Analysis/AnalysisDeclContext.cpp lib/Analysis/BodyFarm.cpp lib/Analysis/BodyFarm.h lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp lib/StaticAnalyzer/Frontend/FrontendActions.cpp lib/StaticAnalyzer/Frontend/ModelConsumer.cpp lib/StaticAnalyzer/Frontend/ModelInjector.cpp lib/StaticAnalyzer/Frontend/ModelInjector.h test/Analysis/Inputs/Models/returnNeedsCheckHasModelFile.model test/Analysis/unchecked-return.cpp
Index: test/Analysis/unchecked-return.cpp =================================================================== --- /dev/null +++ test/Analysis/unchecked-return.cpp @@ -0,0 +1,8 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -analyze -analyzer-checker=core,security.insecureAPI.UncheckedReturn -analyzer-config faux-bodies=true,model-path=%S/Inputs/Models %s -verify + +int returnNeedsCheckHasModelFile(); + +int f0() { + returnNeedsCheckHasModelFile(); // expected-warning{{The return value from the call to 'returnNeedsCheckHasModelFile' is not checked.}} + return 0; +} Index: test/Analysis/Inputs/Models/returnNeedsCheckHasModelFile.model =================================================================== --- /dev/null +++ test/Analysis/Inputs/Models/returnNeedsCheckHasModelFile.model @@ -0,0 +1 @@ +int returnNeedsCheckHasModelFile() __attribute__((warn_unused_result)); Index: lib/StaticAnalyzer/Frontend/ModelInjector.h =================================================================== --- lib/StaticAnalyzer/Frontend/ModelInjector.h +++ lib/StaticAnalyzer/Frontend/ModelInjector.h @@ -45,6 +45,7 @@ ModelInjector(CompilerInstance &CI); Stmt *getBody(const FunctionDecl *D) override; Stmt *getBody(const ObjCMethodDecl *D) override; + FunctionDecl *getModelDecl(const FunctionDecl *D) override; private: /// \brief Synthesize a body for a declaration @@ -67,6 +68,9 @@ // FIXME: double memoization is redundant, with memoization both here and in // BodyFarm. llvm::StringMap<Stmt *> Bodies; + + // Store the model's function declaration if provided. + llvm::StringMap<FunctionDecl *> Decls; }; } } Index: lib/StaticAnalyzer/Frontend/ModelInjector.cpp =================================================================== --- lib/StaticAnalyzer/Frontend/ModelInjector.cpp +++ lib/StaticAnalyzer/Frontend/ModelInjector.cpp @@ -37,11 +37,16 @@ return Bodies[D->getName()]; } +FunctionDecl *ModelInjector::getModelDecl(const FunctionDecl *D) { + onBodySynthesis(D); + return Decls[D->getName()]; +} + void ModelInjector::onBodySynthesis(const NamedDecl *D) { // FIXME: what about overloads? Declarations can be used as keys but what // about file name index? Mangled names may not be suitable for that either. - if (Bodies.count(D->getName()) != 0) + if (Bodies.count(D->getName()) != 0 || Decls.count(D->getName()) != 0) return; SourceManager &SM = CI.getSourceManager(); @@ -60,6 +65,7 @@ if (!llvm::sys::fs::exists(fileName.str())) { Bodies[D->getName()] = nullptr; + Decls[D->getName()] = nullptr; return; } @@ -95,7 +101,7 @@ Instance.getPreprocessor().InitializeForModelFile(); - ParseModelFileAction parseModelFile(Bodies); + ParseModelFileAction parseModelFile(Bodies, Decls); const unsigned ThreadStackSize = 8 << 20; llvm::CrashRecoveryContext CRC; Index: lib/StaticAnalyzer/Frontend/ModelConsumer.cpp =================================================================== --- lib/StaticAnalyzer/Frontend/ModelConsumer.cpp +++ lib/StaticAnalyzer/Frontend/ModelConsumer.cpp @@ -26,16 +26,20 @@ using namespace clang; using namespace ento; -ModelConsumer::ModelConsumer(llvm::StringMap<Stmt *> &Bodies) - : Bodies(Bodies) {} +ModelConsumer::ModelConsumer(llvm::StringMap<Stmt *> &Bodies, + llvm::StringMap<FunctionDecl *> &Decls) + : Bodies(Bodies), Decls(Decls) {} bool ModelConsumer::HandleTopLevelDecl(DeclGroupRef D) { for (DeclGroupRef::iterator I = D.begin(), E = D.end(); I != E; ++I) { - // Only interested in definitions. - const FunctionDecl *func = llvm::dyn_cast<FunctionDecl>(*I); - if (func && func->hasBody()) { - Bodies.insert(std::make_pair(func->getName(), func->getBody())); + // We are interested in definitions and declarations. + FunctionDecl *func = llvm::dyn_cast<FunctionDecl>(*I); + if (func) { + Decls.insert(std::make_pair(func->getName(), func)); + if (func->hasBody()) { + Bodies.insert(std::make_pair(func->getName(), func->getBody())); + } } } return true; Index: lib/StaticAnalyzer/Frontend/FrontendActions.cpp =================================================================== --- lib/StaticAnalyzer/Frontend/FrontendActions.cpp +++ lib/StaticAnalyzer/Frontend/FrontendActions.cpp @@ -18,11 +18,12 @@ return CreateAnalysisConsumer(CI); } -ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *> &Bodies) - : Bodies(Bodies) {} +ParseModelFileAction::ParseModelFileAction(llvm::StringMap<Stmt *> &Bodies, + llvm::StringMap<FunctionDecl *> &Decls) + : Bodies(Bodies), Decls(Decls) {} std::unique_ptr<ASTConsumer> ParseModelFileAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) { - return llvm::make_unique<ModelConsumer>(Bodies); + return llvm::make_unique<ModelConsumer>(Bodies, Decls); } Index: lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -12,6 +12,7 @@ //===----------------------------------------------------------------------===// #include "ClangSACheckers.h" +#include "clang/AST/Attr.h" #include "clang/AST/StmtVisitor.h" #include "clang/Analysis/AnalysisContext.h" #include "clang/Basic/TargetInfo.h" @@ -101,6 +102,9 @@ void checkCall_random(const CallExpr *CE, const FunctionDecl *FD); void checkCall_vfork(const CallExpr *CE, const FunctionDecl *FD); void checkUncheckedReturnValue(CallExpr *CE); + + bool IsAccessRightsAPI(const FunctionDecl *FD); + bool IsShouldCheckReturnAPI(const FunctionDecl *FD); }; } // end anonymous namespace @@ -689,6 +693,30 @@ if (!FD) return; + bool isAccessRightsAPI = IsAccessRightsAPI(FD); + if (!isAccessRightsAPI && !IsShouldCheckReturnAPI(FD)) + return; + + // Issue a warning. + SmallString<256> buf1; + llvm::raw_svector_ostream os1(buf1); + os1 << "Return value is not checked in call to '" << *FD << '\''; + + SmallString<256> buf2; + llvm::raw_svector_ostream os2(buf2); + os2 << "The return value from the call to '" << *FD << "' is not checked. "; + if (isAccessRightsAPI) + os2 << " If an error occurs in '" << *FD + << "', the following code may execute with unexpected privileges"; + + PathDiagnosticLocation CELoc = + PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); + BR.EmitBasicReport(AC->getDecl(), filter.checkName_UncheckedReturn, os1.str(), + "Security", os2.str(), CELoc, + CE->getCallee()->getSourceRange()); +} + +bool WalkAST::IsAccessRightsAPI(const FunctionDecl *FD) { if (II_setid[0] == nullptr) { static const char * const identifiers[num_setids] = { "setuid", "setgid", "seteuid", "setegid", @@ -707,38 +735,36 @@ break; if (identifierid >= num_setids) - return; + return false; const FunctionProtoType *FTP = FD->getType()->getAs<FunctionProtoType>(); if (!FTP) - return; + return false; // Verify that the function takes one or two arguments (depending on // the function). if (FTP->getNumParams() != (identifierid < 4 ? 1 : 2)) - return; + return false; // The arguments must be integers. for (unsigned i = 0; i < FTP->getNumParams(); i++) if (!FTP->getParamType(i)->isIntegralOrUnscopedEnumerationType()) - return; + return false; - // Issue a warning. - SmallString<256> buf1; - llvm::raw_svector_ostream os1(buf1); - os1 << "Return value is not checked in call to '" << *FD << '\''; + return true; +} - SmallString<256> buf2; - llvm::raw_svector_ostream os2(buf2); - os2 << "The return value from the call to '" << *FD - << "' is not checked. If an error occurs in '" << *FD - << "', the following code may execute with unexpected privileges"; +bool WalkAST::IsShouldCheckReturnAPI(const FunctionDecl *FD) { - PathDiagnosticLocation CELoc = - PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); - BR.EmitBasicReport(AC->getDecl(), filter.checkName_UncheckedReturn, os1.str(), - "Security", os2.str(), CELoc, - CE->getCallee()->getSourceRange()); + // Check if the model has a 'warn_unused_result' attribute. + AnalysisDeclContextManager *Mgr = AC->getManager(); + AnalysisDeclContext *FnAC = Mgr->getContext(FD); + Optional<AttrVec> Attrs = FnAC->getModelAttrs(); + + if (Attrs.hasValue() && hasSpecificAttr<WarnUnusedResultAttr>(*Attrs)) { + return true; + } + return false; } //===----------------------------------------------------------------------===// Index: lib/Analysis/BodyFarm.h =================================================================== --- lib/Analysis/BodyFarm.h +++ lib/Analysis/BodyFarm.h @@ -39,11 +39,16 @@ /// Factory method for creating bodies for Objective-C properties. Stmt *getBody(const ObjCMethodDecl *D); + /// Returns the model's declaration. + FunctionDecl *getModelDecl(const FunctionDecl *D); + private: typedef llvm::DenseMap<const Decl *, Optional<Stmt *> > BodyMap; + typedef llvm::DenseMap<const Decl *, Optional<FunctionDecl *> > DeclsMap; ASTContext &C; BodyMap Bodies; + DeclsMap Decls; CodeInjector *Injector; }; } Index: lib/Analysis/BodyFarm.cpp =================================================================== --- lib/Analysis/BodyFarm.cpp +++ lib/Analysis/BodyFarm.cpp @@ -386,6 +386,26 @@ return Val.getValue(); } +FunctionDecl *BodyFarm::getModelDecl(const FunctionDecl *D) { + D = D->getCanonicalDecl(); + + Optional<FunctionDecl *> &Val = Decls[D]; + if (Val.hasValue()) + return Val.getValue(); + + Val = nullptr; + + if (D->getIdentifier() == nullptr) + return nullptr; + + StringRef Name = D->getName(); + if (Name.empty()) + return nullptr; + + if (Injector) { Val = Injector->getModelDecl(D); } + return Val.getValue(); +} + static Stmt *createObjCPropertyGetter(ASTContext &Ctx, const ObjCPropertyDecl *Prop) { // First, find the backing ivar. Index: lib/Analysis/AnalysisDeclContext.cpp =================================================================== --- lib/Analysis/AnalysisDeclContext.cpp +++ lib/Analysis/AnalysisDeclContext.cpp @@ -123,6 +123,20 @@ return getBody(Tmp); } +Optional<AttrVec> AnalysisDeclContext::getModelAttrs() const { + + Optional<AttrVec> Val; + if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) { + if (Manager && Manager->synthesizeBodies()) { + Decl *ModelDecl = getBodyFarm(getASTContext(), Manager->Injector.get()) + .getModelDecl(FD); + if (ModelDecl && ModelDecl->hasAttrs()) + Val = ModelDecl->getAttrs(); + } + } + return Val; +} + bool AnalysisDeclContext::isBodyAutosynthesized() const { bool Tmp; getBody(Tmp); Index: include/clang/StaticAnalyzer/Frontend/ModelConsumer.h =================================================================== --- include/clang/StaticAnalyzer/Frontend/ModelConsumer.h +++ include/clang/StaticAnalyzer/Frontend/ModelConsumer.h @@ -31,12 +31,15 @@ /// from a model file. class ModelConsumer : public ASTConsumer { public: - ModelConsumer(llvm::StringMap<Stmt *> &Bodies); + ModelConsumer(llvm::StringMap<Stmt *> &Bodies, + llvm::StringMap<FunctionDecl *> &Decls); bool HandleTopLevelDecl(DeclGroupRef D) override; private: llvm::StringMap<Stmt *> &Bodies; + /// Store the models' declarations. + llvm::StringMap<FunctionDecl *> &Decls; }; } } Index: include/clang/StaticAnalyzer/Frontend/FrontendActions.h =================================================================== --- include/clang/StaticAnalyzer/Frontend/FrontendActions.h +++ include/clang/StaticAnalyzer/Frontend/FrontendActions.h @@ -40,15 +40,18 @@ /// parsed, the function definitions will be collected into a StringMap. class ParseModelFileAction : public ASTFrontendAction { public: - ParseModelFileAction(llvm::StringMap<Stmt *> &Bodies); + ParseModelFileAction(llvm::StringMap<Stmt *> &Bodies, + llvm::StringMap<FunctionDecl *> &Decls); bool isModelParsingAction() const override { return true; } protected: std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI, StringRef InFile) override; private: llvm::StringMap<Stmt *> &Bodies; + /// Stores the models' declarations. + llvm::StringMap<FunctionDecl *> &Decls; }; void printCheckerHelp(raw_ostream &OS, ArrayRef<std::string> plugins); Index: include/clang/Analysis/CodeInjector.h =================================================================== --- include/clang/Analysis/CodeInjector.h +++ include/clang/Analysis/CodeInjector.h @@ -40,6 +40,8 @@ virtual Stmt *getBody(const FunctionDecl *D) = 0; virtual Stmt *getBody(const ObjCMethodDecl *D) = 0; + /// \brief Returns the model's declaration. + virtual FunctionDecl *getModelDecl(const FunctionDecl *D) = 0; }; } Index: include/clang/Analysis/AnalysisContext.h =================================================================== --- include/clang/Analysis/AnalysisContext.h +++ include/clang/Analysis/AnalysisContext.h @@ -137,6 +137,9 @@ /// by the BodyFarm. Stmt *getBody(bool &IsAutosynthesized) const; + /// \brief Get the attributes from the model. + Optional<AttrVec> getModelAttrs() const; + /// \brief Checks if the body of the Decl is generated by the BodyFarm. /// /// Note, the lookup is not free. We are going to call getBody behind
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits