diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index f97514955a5913..56f7ca63d00621 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -718,6 +718,91 @@ class NoStateChangeFuncVisitor : public BugReporterVisitor { PathSensitiveBugReport &R) final; }; +/// Put a diagnostic on return statement of all inlined functions +/// for which the region of interest \p RegionOfInterest was passed into, +/// but not written inside, and it has caused an undefined read or a null +/// pointer dereference outside. +class NoStoreFuncVisitor final : public NoStateChangeFuncVisitor { + const SubRegion *RegionOfInterest; + MemRegionManager &MmrMgr; + const SourceManager &SM; + const PrintingPolicy &PP; + + /// Recursion limit for dereferencing fields when looking for the + /// region of interest. + /// The limit of two indicates that we will dereference fields only once. + static const unsigned DEREFERENCE_LIMIT = 2; + + using RegionVector = SmallVector; + +public: + NoStoreFuncVisitor( + const SubRegion *R, + bugreporter::TrackingKind TKind = bugreporter::TrackingKind::Thorough) + : NoStateChangeFuncVisitor(TKind), RegionOfInterest(R), + MmrMgr(R->getMemRegionManager()), + SM(MmrMgr.getContext().getSourceManager()), + PP(MmrMgr.getContext().getPrintingPolicy()) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int Tag = 0; + ID.AddPointer(&Tag); + ID.AddPointer(RegionOfInterest); + } + +private: + /// \return Whether \c RegionOfInterest was modified at \p CurrN compared to + /// the value it holds in \p CallExitBeginN. + bool wasModifiedBeforeCallExit(const ExplodedNode *CurrN, + const ExplodedNode *CallExitBeginN) override; + + /// Attempts to find the region of interest in a given record decl, + /// by either following the base classes or fields. + /// Dereferences fields up to a given recursion limit. + /// Note that \p Vec is passed by value, leading to quadratic copying cost, + /// but it's OK in practice since its length is limited to DEREFERENCE_LIMIT. + /// \return A chain fields leading to the region of interest or std::nullopt. + const std::optional + findRegionOfInterestInRecord(const RecordDecl *RD, ProgramStateRef State, + const MemRegion *R, const RegionVector &Vec = {}, + int depth = 0); + + // Region of interest corresponds to an IVar, exiting a method + // which could have written into that IVar, but did not. + PathDiagnosticPieceRef maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R, + const ObjCMethodCall &Call, + const ExplodedNode *N) final; + + PathDiagnosticPieceRef maybeEmitNoteForCXXThis(PathSensitiveBugReport &R, + const CXXConstructorCall &Call, + const ExplodedNode *N) final; + + PathDiagnosticPieceRef + maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call, + const ExplodedNode *N) final; + + /// Consume the information on the no-store stack frame in order to + /// either emit a note or suppress the report entirely. + /// \return Diagnostics piece for region not modified in the current function, + /// if it decides to emit one. + PathDiagnosticPieceRef + maybeEmitNote(PathSensitiveBugReport &R, const CallEvent &Call, + const ExplodedNode *N, const RegionVector &FieldChain, + const MemRegion *MatchedRegion, StringRef FirstElement, + bool FirstIsReferenceType, unsigned IndirectionLevel); + + bool prettyPrintRegionName(const RegionVector &FieldChain, + const MemRegion *MatchedRegion, + StringRef FirstElement, bool FirstIsReferenceType, + unsigned IndirectionLevel, + llvm::raw_svector_ostream &os); + + StringRef prettyPrintFirstElement(StringRef FirstElement, + bool MoreItemsExpected, + int IndirectionLevel, + llvm::raw_svector_ostream &os); +}; + } // namespace ento } // namespace clang diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h index 0365f9e41312df..168983fd5cb686 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -69,6 +69,7 @@ class CheckerContext { /// the state of the program before the checker ran. Note, checkers should /// not retain the node in their state since the nodes might get invalidated. ExplodedNode *getPredecessor() { return Pred; } + const ProgramPoint getLocation() const { return Location; } const ProgramStateRef &getState() const { return Pred->getState(); } /// Check if the checker changed the state of the execution; ex: added diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h index def2970d448d48..a054a819a15a85 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h @@ -89,6 +89,8 @@ class SVal { SValKind getKind() const { return Kind; } + StringRef getKindStr() const; + // This method is required for using SVal in a FoldingSetNode. It // extracts a unique signature for this SVal object. void Profile(llvm::FoldingSetNodeID &ID) const { diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 8dd08f14b2728b..21a2d8828249d1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -16,6 +16,7 @@ #include "clang/Basic/Builtins.h" #include "clang/Basic/CharInfo.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" @@ -337,7 +338,8 @@ class CStringChecker : public Checker< eval::Call, const Stmt *S, StringRef WarningMsg) const; void emitAdditionOverflowBug(CheckerContext &C, ProgramStateRef State) const; void emitUninitializedReadBug(CheckerContext &C, ProgramStateRef State, - const Expr *E, StringRef Msg) const; + const Expr *E, const MemRegion *R, + StringRef Msg) const; ProgramStateRef checkAdditionOverflow(CheckerContext &C, ProgramStateRef state, NonLoc left, @@ -474,7 +476,8 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, OS << "The first element of the "; printIdxWithOrdinalSuffix(OS, Buffer.ArgumentIndex + 1); OS << " argument is undefined"; - emitUninitializedReadBug(C, State, Buffer.Expression, OS.str()); + emitUninitializedReadBug(C, State, Buffer.Expression, + FirstElementVal->getAsRegion(), OS.str()); return nullptr; } @@ -538,7 +541,8 @@ ProgramStateRef CStringChecker::checkInit(CheckerContext &C, OS << ") in the "; printIdxWithOrdinalSuffix(OS, Buffer.ArgumentIndex + 1); OS << " argument is undefined"; - emitUninitializedReadBug(C, State, Buffer.Expression, OS.str()); + emitUninitializedReadBug(C, State, Buffer.Expression, + LastElementVal.getAsRegion(), OS.str()); return nullptr; } return State; @@ -818,7 +822,7 @@ void CStringChecker::emitNullArgBug(CheckerContext &C, ProgramStateRef State, void CStringChecker::emitUninitializedReadBug(CheckerContext &C, ProgramStateRef State, - const Expr *E, + const Expr *E, const MemRegion *R, StringRef Msg) const { if (ExplodedNode *N = C.generateErrorNode(State)) { if (!BT_UninitRead) @@ -831,6 +835,7 @@ void CStringChecker::emitUninitializedReadBug(CheckerContext &C, Report->getLocation()); Report->addRange(E->getSourceRange()); bugreporter::trackExpressionValue(N, E, *Report); + Report->addVisitor(R->castAs()); C.emitReport(std::move(Report)); } } diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 7102bf51a57e8b..68c8a8dc682507 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -522,95 +522,6 @@ PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode( return maybeEmitNoteForParameters(R, *Call, N); } -//===----------------------------------------------------------------------===// -// Implementation of NoStoreFuncVisitor. -//===----------------------------------------------------------------------===// - -namespace { -/// Put a diagnostic on return statement of all inlined functions -/// for which the region of interest \p RegionOfInterest was passed into, -/// but not written inside, and it has caused an undefined read or a null -/// pointer dereference outside. -class NoStoreFuncVisitor final : public NoStateChangeFuncVisitor { - const SubRegion *RegionOfInterest; - MemRegionManager &MmrMgr; - const SourceManager &SM; - const PrintingPolicy &PP; - - /// Recursion limit for dereferencing fields when looking for the - /// region of interest. - /// The limit of two indicates that we will dereference fields only once. - static const unsigned DEREFERENCE_LIMIT = 2; - - using RegionVector = SmallVector; - -public: - NoStoreFuncVisitor(const SubRegion *R, bugreporter::TrackingKind TKind) - : NoStateChangeFuncVisitor(TKind), RegionOfInterest(R), - MmrMgr(R->getMemRegionManager()), - SM(MmrMgr.getContext().getSourceManager()), - PP(MmrMgr.getContext().getPrintingPolicy()) {} - - void Profile(llvm::FoldingSetNodeID &ID) const override { - static int Tag = 0; - ID.AddPointer(&Tag); - ID.AddPointer(RegionOfInterest); - } - -private: - /// \return Whether \c RegionOfInterest was modified at \p CurrN compared to - /// the value it holds in \p CallExitBeginN. - bool wasModifiedBeforeCallExit(const ExplodedNode *CurrN, - const ExplodedNode *CallExitBeginN) override; - - /// Attempts to find the region of interest in a given record decl, - /// by either following the base classes or fields. - /// Dereferences fields up to a given recursion limit. - /// Note that \p Vec is passed by value, leading to quadratic copying cost, - /// but it's OK in practice since its length is limited to DEREFERENCE_LIMIT. - /// \return A chain fields leading to the region of interest or std::nullopt. - const std::optional - findRegionOfInterestInRecord(const RecordDecl *RD, ProgramStateRef State, - const MemRegion *R, const RegionVector &Vec = {}, - int depth = 0); - - // Region of interest corresponds to an IVar, exiting a method - // which could have written into that IVar, but did not. - PathDiagnosticPieceRef maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R, - const ObjCMethodCall &Call, - const ExplodedNode *N) final; - - PathDiagnosticPieceRef maybeEmitNoteForCXXThis(PathSensitiveBugReport &R, - const CXXConstructorCall &Call, - const ExplodedNode *N) final; - - PathDiagnosticPieceRef - maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call, - const ExplodedNode *N) final; - - /// Consume the information on the no-store stack frame in order to - /// either emit a note or suppress the report enirely. - /// \return Diagnostics piece for region not modified in the current function, - /// if it decides to emit one. - PathDiagnosticPieceRef - maybeEmitNote(PathSensitiveBugReport &R, const CallEvent &Call, - const ExplodedNode *N, const RegionVector &FieldChain, - const MemRegion *MatchedRegion, StringRef FirstElement, - bool FirstIsReferenceType, unsigned IndirectionLevel); - - bool prettyPrintRegionName(const RegionVector &FieldChain, - const MemRegion *MatchedRegion, - StringRef FirstElement, bool FirstIsReferenceType, - unsigned IndirectionLevel, - llvm::raw_svector_ostream &os); - - StringRef prettyPrintFirstElement(StringRef FirstElement, - bool MoreItemsExpected, - int IndirectionLevel, - llvm::raw_svector_ostream &os); -}; -} // namespace - /// \return Whether the method declaration \p Parent /// syntactically has a binary operation writing into the ivar \p Ivar. static bool potentiallyWritesIntoIvar(const Decl *Parent, diff --git a/clang/lib/StaticAnalyzer/Core/SVals.cpp b/clang/lib/StaticAnalyzer/Core/SVals.cpp index 291e4fa752a8f7..84e7e033404c03 100644 --- a/clang/lib/StaticAnalyzer/Core/SVals.cpp +++ b/clang/lib/StaticAnalyzer/Core/SVals.cpp @@ -263,6 +263,23 @@ bool SVal::isZeroConstant() const { // Pretty-Printing. //===----------------------------------------------------------------------===// +StringRef SVal::getKindStr() const { + switch (getKind()) { +#define BASIC_SVAL(Id, Parent) \ + case Id##Kind: \ + return #Id; +#define LOC_SVAL(Id, Parent) \ + case Loc##Id##Kind: \ + return #Id; +#define NONLOC_SVAL(Id, Parent) \ + case NonLoc##Id##Kind: \ + return #Id; +#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def" +#undef REGION + } + llvm_unreachable("Unkown kind!"); +} + LLVM_DUMP_METHOD void SVal::dump() const { dumpToStream(llvm::errs()); } void SVal::printJson(raw_ostream &Out, bool AddQuotes) const { diff --git a/clang/test/Analysis/cstring-uninitread-notes.c b/clang/test/Analysis/cstring-uninitread-notes.c new file mode 100644 index 00000000000000..b62519a85c8cc9 --- /dev/null +++ b/clang/test/Analysis/cstring-uninitread-notes.c @@ -0,0 +1,25 @@ +// RUN: %clang_analyze_cc1 -verify %s \ +// RUN: -analyzer-checker=core,alpha.unix.cstring \ +// RUN: -analyzer-output=text + +#include "Inputs/system-header-simulator.h" + +// Inspired by a report on ffmpeg, libavcodec/tiertexseqv.c, seq_decode_op1(). +int coin(); + +void maybeWrite(const char *src, unsigned size, int *dst) { + if (coin()) // expected-note{{Assuming the condition is false}} + // expected-note@-1{{Taking false branch}} + memcpy(dst, src, size); +} // expected-note{{Returning without writing to '*dst'}} + +void returning_without_writing_to_memcpy(const char *src, unsigned size) { + int block[8 * 8]; // expected-note{{'block' initialized here}} + // expected-note@+1{{Calling 'maybeWrite'}} + maybeWrite(src, size, block); // expected-note{{Returning from 'maybeWrite'}} + + int buf[8 * 8]; + memcpy(buf, &block[0], 8); // expected-warning{{The first element of the 2nd argument is undefined [alpha.unix.cstring.UninitializedRead]}} + // expected-note@-1{{The first element of the 2nd argument is undefined}} + // expected-note@-2{{Other elements might also be undefined}} +}