diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -19,6 +19,7 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Lex/Preprocessor.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h" @@ -588,6 +589,9 @@ /// A vector of BugReports for tracking the allocated pointers and cleanup. std::vector EQClassesVector; + /// User-provided in-code suppressions. + BugSuppression UserSuppressions; + public: BugReporter(BugReporterData &d); virtual ~BugReporter(); diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h new file mode 100644 --- /dev/null +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h @@ -0,0 +1,44 @@ +//===- BugSuppression.h - Suppression interface -----------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file defines BugSuppression, a simple interface class incapsulating +// all user provided in-code suppressions. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPTRESSION_H +#define LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPTRESSION_H + +#include "clang/Basic/SourceLocation.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/SmallVector.h" + +namespace clang { +class Decl; + +namespace ento { +class BugReport; + +class BugSuppression { +public: + /// Return true if the given bug report was explicitly suppressed by the user. + bool isSuppressed(const BugReport &); + +private: + // Overly pessimistic number, to be honest. + static constexpr unsigned EXPECTED_NUMBER_OF_SUPPRESSIONS = 8; + using CachedRanges = + llvm::SmallVector; + + llvm::DenseMap CachedSuppressionLocations; +}; + +} // end namespace ento +} // end namespace clang + +#endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_SUPPTRESSION_H diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -2872,107 +2872,6 @@ return Out; } -//===----------------------------------------------------------------------===// -// Manual suppressions -//===----------------------------------------------------------------------===// - -namespace { -llvm::Optional -findSuppressAttribute(ArrayRef Attrs) { - const auto *It = - llvm::find_if(Attrs, [](const Attr *A) { return isa(A); }); - - if (It != Attrs.end()) { - return cast(*It); - } - - return llvm::None; -} - -// TODO: In the future, when we come up with good stable IDs for checkers -// we can return a list of kinds to ignore, or all if no arguments were -// provided. -llvm::Optional findSuppressAttribute(const BugReport &BR, - ASTContext &AC) { - PathDiagnosticLocation Location = BR.getLocation(); - - if (const Stmt *BugStmt = Location.asStmt()) { - // If location is statement, it can actually be an expression buried - // deeply in other expressions, so the attribute can be few levels above - // in the AST. For this reason, we traverse this statement's parents - // and check if they have suppressions. - // - // The statement itself can have suppression attributes, so let's - // start with it. - llvm::SmallVector Stack{DynTypedNode::create(*BugStmt)}; - - while (!Stack.empty()) { - const auto &Current = Stack.back(); - Stack.pop_back(); - - if (const auto *AS = Current.get()) { - // Only AttributedStmt out of all statements can have attributes. - if (auto Suppress = findSuppressAttribute(AS->getAttrs())) { - return Suppress; - } - // Suppressions can still be further up, so let's continue. - } else if (const auto *VD = Current.get()) { - // Bug location could've been somewhere in the init value of - // a freshly declared variable. Even though it looks like the - // user applied attribute to a statement, it will apply to a - // variable declaration, and this is where we check for it. - if (VD->hasAttr()) { - return VD->getAttr(); - } - } else if (const auto *D = Current.get()) { - // If we got to some other form of declaration, we don't want - // to go further. Declaration attributes (in cases other than for - // variables) should be used ONLY to suppress warnings reported directly - // on that declaration. - continue; - } - - // Let's continue with the current node's parent(s). - for (const auto &Parent : AC.getParents(Current)) { - Stack.push_back(Parent); - } - } - } else if (const Decl *BugDecl = Location.asDecl()) { - // If we want to report on a declaration, we don't want to check parents - // or stuff like that, we simply want to check if that declaration has - // a supression attribute. - if (BugDecl->hasAttr()) { - return BugDecl->getAttr(); - } - } else if (Location.hasValidLocation()) { - // This is the trickiest part. Bug location is simply a SourceLocation - // object. We don't know a node to attach it to. - // - // At the moment, we check the declaration containing the bug for - // suppression attributes. It is indeed to crude, probably a better - // solution would be to suppress issues reported outside of the body - // of this declaration in this manner (e.g. reported on curly braces - // or on one of the function arguments, or on class parent). - if (const Decl *DeclWithIssue = BR.getDeclWithIssue()) { - if (DeclWithIssue->hasAttr()) { - return DeclWithIssue->getAttr(); - } - } - } - - return llvm::None; -} - -bool isSuppressed(const BugReport &R, ASTContext &AC) { - // TODO: Introduce stable IDs for checkers and check for those here - // to be more specific. Attribute without arguments should still - // be considered as "suppress all". - // It is already much finer granularity than what we have now - // (i.e. removing the whole function from the analysis). - return findSuppressAttribute(R, AC).hasValue(); -} -} // end anonymous namespace - void BugReporter::emitReport(std::unique_ptr R) { bool ValidSourceLoc = R->getLocation().isValid(); assert(ValidSourceLoc); @@ -2982,7 +2881,7 @@ return; // If the user asked to suppress this report, we should skip it. - if (isSuppressed(*R, getContext())) + if (UserSuppressions.isSuppressed(*R)) return; // Compute the bug report's hash to determine its equivalence class. diff --git a/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp new file mode 100644 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp @@ -0,0 +1,184 @@ +//===- BugSuppression.cpp - Suppression interface -------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" + +using namespace clang; +using namespace ento; + +namespace { + +using Ranges = llvm::SmallVectorImpl; + +inline bool hasSuppression(const Decl *D) { return D->hasAttr(); } +inline bool hasSuppression(const AttributedStmt *S) { + return llvm::any_of(S->getAttrs(), + [](const Attr *A) { return isa(A); }); +} + +template inline SourceRange getRange(const NodeType *Node) { + return Node->getSourceRange(); +} +template <> inline SourceRange getRange(const AttributedStmt *S) { + // Begin location for attributed statement node seems to be ALWAYS invalid. + // + // It is unlikely that we ever report any warnings on suppression + // attribute itself, but even if we do, we wouldn't want that warning + // to be suppressed by that same attribute. + // + // Long story short, we can use inner statement and it's not going to break + // anything. + return getRange(S->getSubStmt()); +} + +inline bool isLessOrEqual(SourceLocation LHS, SourceLocation RHS, + const SourceManager &SM) { + // SourceManager::isBeforeInTranslationUnit tests for strict + // inequality, when we need a non-strict comparison (bug + // can be reported directly on the annotated note). + // For this reason, we use the following equivalence: + // + // A <= B <==> !(B < A) + // + return !SM.isBeforeInTranslationUnit(RHS, LHS); +} + +inline bool fullyContains(SourceRange Larger, SourceRange Smaller, + const SourceManager &SM) { + // Essentially this means: + // + // Larger.fullyContains(Smaller) + // + // However, that method has a very trivial implementation and couldn't + // compare regular locations and locations from macro expansions. + // We could've converted everything into regular locations as a solution, + // but the following solution seems to be the most bulletproof. + return isLessOrEqual(Larger.getBegin(), Smaller.getBegin(), SM) && + isLessOrEqual(Smaller.getEnd(), Larger.getEnd(), SM); +} + +class CacheInitializer : public RecursiveASTVisitor { +public: + static void initialize(const Decl *D, Ranges &ToInit) { + CacheInitializer(ToInit).TraverseDecl(const_cast(D)); + } + + bool VisitVarDecl(VarDecl *VD) { + // Bug location could be somewhere in the init value of + // a freshly declared variable. Even though it looks like the + // user applied attribute to a statement, it will apply to a + // variable declaration, and this is where we check for it. + return VisitAttributedNode(VD); + } + + bool VisitAttributedStmt(AttributedStmt *AS) { + // When we apply attributes to statements, it actually creates + // a wrapper statement that only contains attributes and the wrapped + // statement. + return VisitAttributedNode(AS); + } + +private: + template bool VisitAttributedNode(NodeType *Node) { + if (hasSuppression(Node)) { + // TODO: In the future, when we come up with good stable IDs for checkers + // we can return a list of kinds to ignore, or all if no arguments + // were provided. + addRange(getRange(Node)); + } + // We should keep traversing AST. + return true; + } + + void addRange(SourceRange R) { + if (R.isValid()) { + Result.push_back(R); + } + } + + CacheInitializer(Ranges &R) : Result(R) {} + Ranges &Result; +}; + +inline bool isInDeclarationBody(PathDiagnosticLocation Location, + const Decl *D) { + // Couldn't be inside of body if there is no body. + if (!D->hasBody()) + return false; + + SourceRange BugRange = Location.asRange(); + // If the warning is reported on the closing brace, + // the user should suppress it on the declaration level. + if (BugRange.getBegin() == D->getBodyRBrace()) + return false; + + return fullyContains(D->getBody()->getSourceRange(), BugRange, + Location.getManager()); +} + +} // end anonymous namespace + +// TODO: Introduce stable IDs for checkers and check for those here +// to be more specific. Attribute without arguments should still +// be considered as "suppress all". +// It is already much finer granularity than what we have now +// (i.e. removing the whole function from the analysis). +bool BugSuppression::isSuppressed(const BugReport &R) { + PathDiagnosticLocation Location = R.getLocation(); + const Decl *DeclWithIssue = R.getDeclWithIssue(); + + if (!Location.isValid() || DeclWithIssue == nullptr) + return false; + + // There are two scenarios for suppressions: + // + // 1. warning is in the declaration's body + // 2. warning is somewhere else + // + // In the first case, we assume that the declaration is some sort + // of function and suppressions should be attached to statements + // within that body, for finer granularity. + // + // In the second case, even though it is not necessarily reported + // on the declaration itself (e.g. class' base specification), we + // still expect suppressions to be attached to the declaration. + // + // Case #2. + if (!isInDeclarationBody(Location, DeclWithIssue)) + return hasSuppression(DeclWithIssue); + + // Case #1. + // + // While some warnings are attached to AST nodes (mostly path-sensitive + // checks), others are simply associated with a plain source location + // or range. Figuring out the node based on locations can be tricky, + // so instead, we traverse the whole body of the declaration and gather + // information on ALL suppressions. After that we can simply check if + // any of those suppressions affect the warning in question. + // + // Traversing AST of a function is not a heavy operation, but for + // large functions with a lot of bugs it can make a dent in performance. + // In order to avoid this scenario, we cache traversal results. + auto InsertionResult = CachedSuppressionLocations.insert( + std::make_pair(DeclWithIssue, CachedRanges{})); + Ranges &SuppressionRanges = InsertionResult.first->second; + if (InsertionResult.second) { + // We haven't checked this declaration for suppressions yet! + CacheInitializer::initialize(DeclWithIssue, SuppressionRanges); + } + + SourceRange BugRange = Location.asRange(); + const SourceManager &SM = Location.getManager(); + + return llvm::any_of(SuppressionRanges, + [BugRange, &SM](SourceRange Suppression) { + return fullyContains(Suppression, BugRange, SM); + }); +} diff --git a/clang/lib/StaticAnalyzer/Core/CMakeLists.txt b/clang/lib/StaticAnalyzer/Core/CMakeLists.txt --- a/clang/lib/StaticAnalyzer/Core/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Core/CMakeLists.txt @@ -11,6 +11,7 @@ BlockCounter.cpp BugReporter.cpp BugReporterVisitors.cpp + BugSuppression.cpp CallEvent.cpp Checker.cpp CheckerContext.cpp diff --git a/clang/test/Analysis/suppression-attr.m b/clang/test/Analysis/suppression-attr.m --- a/clang/test/Analysis/suppression-attr.m +++ b/clang/test/Analysis/suppression-attr.m @@ -2,6 +2,9 @@ // RUN: -analyzer-checker=core \ // RUN: -analyzer-checker=osx.cocoa.MissingSuperCall \ // RUN: -analyzer-checker=osx.cocoa.NSError \ +// RUN: -analyzer-checker=osx.ObjCProperty \ +// RUN: -analyzer-checker=osx.cocoa.RetainCount \ +// RUN: -analyzer-checker=alpha.core.CastToStruct \ // RUN: -Wno-unused-value -Wno-objc-root-class -verify %s #define SUPPRESS __attribute__((suppress)) @@ -31,6 +34,9 @@ + (id)errorWithDomain:(NSString *)domain code:(NSInteger)code userInfo:(NSDictionary *)dict; @end +@interface NSMutableString : NSObject +@end + void dereference_1() { int *x = 0; *x; // expected-warning{{Dereference of null pointer (loaded from variable 'x')}} @@ -77,6 +83,28 @@ SUPPRESS int y = *x; // no-warning } +void retain_release_leak_1() { + [[NSMutableString alloc] init]; // expected-warning{{Potential leak of an object of type 'NSMutableString *'}} +} + +void retain_release_leak_suppression_1() { + SUPPRESS { [[NSMutableString alloc] init]; } +} + +void retain_release_leak_2(int cond) { + id obj = [[NSMutableString alloc] init]; // expected-warning{{Potential leak of an object stored into 'obj'}} + if (cond) { + [obj release]; + } +} + +void retain_release_leak__suppression_2(int cond) { + SUPPRESS id obj = [[NSMutableString alloc] init]; + if (cond) { + [obj release]; + } +} + @interface UIResponder : NSObject { } - (char)resignFirstResponder; @@ -84,6 +112,7 @@ @interface Test : UIResponder { } +@property(copy) NSMutableString *mutableStr; // expected-warning{{Property of mutable type 'NSMutableString' has 'copy' attribute; an immutable object will be stored instead}} @end @implementation Test @@ -98,6 +127,7 @@ @interface TestSuppress : UIResponder { } +@property(copy) SUPPRESS NSMutableString *mutableStr; @end @implementation TestSuppress @@ -108,3 +138,23 @@ - (void)methodWhichMayFail:(NSError **)error SUPPRESS { // no-warning } @end + +struct AB { + int A, B; +}; + +struct ABC { + int A, B, C; +}; + +void ast_checker_1() { + struct AB Ab; + struct ABC *Abc; + Abc = (struct ABC *)&Ab; // expected-warning {{Casting data to a larger structure type and accessing a field can lead to memory access errors or data corruption}} +} + +void ast_checker_suppress_1() { + struct AB Ab; + struct ABC *Abc; + SUPPRESS { Abc = (struct ABC *)&Ab; } +}