[clang] [analyzer] Implement binary operations on LazyCompoundVals (PR #106982)
Kristóf Umann via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 2 05:55:40 PDT 2024
https://github.com/Szelethus created https://github.com/llvm/llvm-project/pull/106982
SValBuilder used to early exit when we did binary operations when one of the operands was a LazyCompoundVal. Riding on the new docs patch (#97407), this patch extracts the actual value behind the LazyCompoundVal, and proceeds with the operation.
This patch is unfortunately much larger than that, and initially, I'm only looking for feedback if I should split it up a bit more.
The goal of this patch was the following:
1. Implement binary operations on LazyCompoundVals,
2. Use this to be able to check if two LazyCompoundVals are equal,
3. Which can be used in NoStoreFuncVisitor to attach "Returning without writing to x" notes even when the tracked variable is a LazyCompoundVal,
4. Which is needed for alpha.unix.UninitializedRead, as elements of an array are often LazyCompoundVals.
This final point was inspired by a pretty poor bug report on FFMpeg:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?is-unique=on&run=curl_curl-7_66_0_cstring_uninit_baseline&run=ffmpeg_n4.3.1_cstring_uninit_baseline&run=memcached_1.6.8_cstring_uninit_baseline&run=openssl_openssl-3.0.0-alpha7_cstring_uninit_baseline&run=sqlite_version-3.33.0_cstring_uninit_baseline&run=tmux_2.6_cstring_uninit_baseline&run=twin_v0.8.1_cstring_uninit_baseline&run=vim_v8.2.1920_cstring_uninit_baseline&newcheck=curl_curl-7_66_0_cstring_uninit_final&newcheck=ffmpeg_n4.3.1_cstring_uninit_final&newcheck=memcached_1.6.8_cstring_uninit_final&newcheck=openssl_openssl-3.0.0-alpha7_cstring_uninit_final&newcheck=sqlite_version-3.33.0_cstring_uninit_final&newcheck=tmux_2.6_cstring_uninit_final&newcheck=twin_v0.8.1_cstring_uninit_final&newcheck=vim_v8.2.1920_cstring_uninit_final&diff-type=Unresolved&items-per-page=100&checker-name=alpha.unix.cstring.UninitializedRead&report-id=5766433&report-hash=82c63868ba782bfa216ee3f2b5734d6b&report-filepath=%2atiertexseqv.c
In this bug report, block is uninitialized, hence the bug report that it should not have been passed to memcpy. The confusing part is in line 93, where block was passed as a non-const pointer to seq_unpack_rle_block, which was obviously meant to initialize block. As developers, we know that clang likely didn't skip this function and found a path of execution on which this initialization failed, but NoStoreFuncVisitor failed to attach the usual "returning without writing to block" message.
I fixed this by instead of tracking the entire array, I tracked the actual element which was found to be uninitialized (Remember, we heuristically only check if the first and last-to-access element is initialized, not the entire array). This is how the bug report looks now:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?is-unique=on&run=curl_curl-7_66_0_cstring_uninit_baseline&run=ffmpeg_n4.3.1_cstring_uninit_baseline&run=memcached_1.6.8_cstring_uninit_baseline&run=openssl_openssl-3.0.0-alpha7_cstring_uninit_baseline&run=sqlite_version-3.33.0_cstring_uninit_baseline&run=tmux_2.6_cstring_uninit_baseline&run=twin_v0.8.1_cstring_uninit_baseline&run=vim_v8.2.1920_cstring_uninit_baseline&newcheck=curl_curl-7_66_0_cstring_uninit_final&newcheck=ffmpeg_n4.3.1_cstring_uninit_final&newcheck=memcached_1.6.8_cstring_uninit_final&newcheck=openssl_openssl-3.0.0-alpha7_cstring_uninit_final&newcheck=sqlite_version-3.33.0_cstring_uninit_final&newcheck=tmux_2.6_cstring_uninit_final&newcheck=twin_v0.8.1_cstring_uninit_final&newcheck=vim_v8.2.1920_cstring_uninit_final&diff-type=Unresolved&items-per-page=100&checker-name=alpha.unix.cstring.UninitializedRead&report-id=5768860&report-hash=82c63868ba782bfa216ee3f2b5734d6b&report-filepath=%2atiertexseqv.c
Since NoStoreFuncVisitor was a TU-local class, I moved it back to BugReporterVisitors.h, and registered it manually in CStringChecker.cpp. This was done because we don't have a good trackRegionValue() function, only a trackExpressionValue() function. We have an expression for the array, but not for its first (or last-to-access) element, so I only had a MemRegion on hand.
NoStoreFuncVisitor works by checking if the value of a region-of-interest changed in a function; if it wasn't, it emits a helpful diagnostic message. This relied on SValBuilder comparing values. Since it didn't support the comparison of LazyCompoundVals, which are often formed around C++ objects, it wasn't really prepped to deal with them. After my fix, I needed to iron out some kinds around constructors, hence the non-trivial changes made in BugReporterVisitors.cpp.
>From c41e2ba31913e2c81f0d80daf52dbe6c730f8044 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelethus at gmail.com>
Date: Tue, 16 Jul 2024 15:06:06 +0200
Subject: [PATCH] [analyzer] Implement binary operations on LazyCompoundVals
SValBuilder used to early exit when we did binary operations when one of
the operands was a LazyCompoundVal. Riding on the new docs patch
(#97407), this patch extracts the actual value behind the
LazyCompoundVal, and proceeds with the operation.
This patch is unfortunately much larger than that, and initially, I'm
only looking for feedback if I should split it up a bit more.
The goal of this patch was the following:
1. Implement binary operations on LazyCompoundVals,
2. Use this to be able to check if two LazyCompoundVals are equal,
3. Which can be used in NoStoreFuncVisitor to attach "Returning without
writing to x" notes even when the tracked variable is a
LazyCompoundVal,
4. Which is needed for alpha.unix.UninitializedRead, as elements of an
array are often LazyCompoundVals.
This final point was inspired by a pretty poor bug report on FFMpeg:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?is-unique=on&run=curl_curl-7_66_0_cstring_uninit_baseline&run=ffmpeg_n4.3.1_cstring_uninit_baseline&run=memcached_1.6.8_cstring_uninit_baseline&run=openssl_openssl-3.0.0-alpha7_cstring_uninit_baseline&run=sqlite_version-3.33.0_cstring_uninit_baseline&run=tmux_2.6_cstring_uninit_baseline&run=twin_v0.8.1_cstring_uninit_baseline&run=vim_v8.2.1920_cstring_uninit_baseline&newcheck=curl_curl-7_66_0_cstring_uninit_final&newcheck=ffmpeg_n4.3.1_cstring_uninit_final&newcheck=memcached_1.6.8_cstring_uninit_final&newcheck=openssl_openssl-3.0.0-alpha7_cstring_uninit_final&newcheck=sqlite_version-3.33.0_cstring_uninit_final&newcheck=tmux_2.6_cstring_uninit_final&newcheck=twin_v0.8.1_cstring_uninit_final&newcheck=vim_v8.2.1920_cstring_uninit_final&diff-type=Unresolved&items-per-page=100&checker-name=alpha.unix.cstring.UninitializedRead&report-id=5766433&report-hash=82c63868ba782bfa216ee3f2b5734d6b&report-filepath=%2atiertexseqv.c
In this bug report, block is uninitialized, hence the bug report that it
should not have been passed to memcpy. The confusing part is in line 93,
where block was passed as a non-const pointer to seq_unpack_rle_block,
which was obviously meant to initialize block. As developers, we know
that clang likely didn't skip this function and found a path of
execution on which this initialization failed, but NoStoreFuncVisitor
failed to attach the usual "returning without writing to block" message.
I fixed this by instead of tracking the entire array, I tracked the
actual element which was found to be uninitialized (Remember, we
heuristically only check if the first and last-to-access element is
initialized, not the entire array). This is how the bug report looks
now:
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?is-unique=on&run=curl_curl-7_66_0_cstring_uninit_baseline&run=ffmpeg_n4.3.1_cstring_uninit_baseline&run=memcached_1.6.8_cstring_uninit_baseline&run=openssl_openssl-3.0.0-alpha7_cstring_uninit_baseline&run=sqlite_version-3.33.0_cstring_uninit_baseline&run=tmux_2.6_cstring_uninit_baseline&run=twin_v0.8.1_cstring_uninit_baseline&run=vim_v8.2.1920_cstring_uninit_baseline&newcheck=curl_curl-7_66_0_cstring_uninit_final&newcheck=ffmpeg_n4.3.1_cstring_uninit_final&newcheck=memcached_1.6.8_cstring_uninit_final&newcheck=openssl_openssl-3.0.0-alpha7_cstring_uninit_final&newcheck=sqlite_version-3.33.0_cstring_uninit_final&newcheck=tmux_2.6_cstring_uninit_final&newcheck=twin_v0.8.1_cstring_uninit_final&newcheck=vim_v8.2.1920_cstring_uninit_final&diff-type=Unresolved&items-per-page=100&checker-name=alpha.unix.cstring.UninitializedRead&report-id=5768860&report-hash=82c63868ba782bfa216ee3f2b5734d6b&report-filepath=%2atiertexseqv.c
Since NoStoreFuncVisitor was a TU-local class, I moved it back to
BugReporterVisitors.h, and registered it manually in CStringChecker.cpp.
This was done because we don't have a good trackRegionValue() function,
only a trackExpressionValue() function. We have an expression for the
array, but not for its first (or last-to-access) element, so I only had
a MemRegion on hand.
NoStoreFuncVisitor works by checking if the value of a
region-of-interest changed in a function; if it wasn't, it emits a
helpful diagnostic message. This relied on SValBuilder comparing values.
Since it didn't support the comparison of LazyCompoundVals, which are
often formed around C++ objects, it wasn't really prepped to deal with
them. After my fix, I needed to iron out some kinds around constructors,
hence the non-trivial changes made in BugReporterVisitors.cpp.
---
.../Core/BugReporter/BugReporterVisitors.h | 85 ++++++++++
.../Core/PathSensitive/CheckerContext.h | 1 +
.../StaticAnalyzer/Core/PathSensitive/SVals.h | 2 +
.../Checkers/CStringChecker.cpp | 13 +-
.../Core/BugReporterVisitors.cpp | 159 ++++++++----------
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp | 58 ++++++-
clang/lib/StaticAnalyzer/Core/SVals.cpp | 17 ++
.../test/Analysis/cstring-uninitread-notes.c | 25 +++
8 files changed, 268 insertions(+), 92 deletions(-)
create mode 100644 clang/test/Analysis/cstring-uninitread-notes.c
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index f97514955a5913..305622d81dc3e7 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<const MemRegion *, 5>;
+
+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<RegionVector>
+ 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 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..d8b818eead26be 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,
+ FirstElementVal->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<NoStoreFuncVisitor>(R->castAs<SubRegion>());
C.emitReport(std::move(Report));
}
}
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 7102bf51a57e8b..9ec287d9044eac 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -315,6 +315,62 @@ static bool isFunctionMacroExpansion(SourceLocation Loc,
return EInfo.isFunctionMacroExpansion();
}
+static const LocationContext *getFirstNonCtorCall(const LocationContext *LCtx) {
+ while (llvm::isa_and_nonnull<CXXConstructorDecl>(LCtx->getDecl()))
+ LCtx = LCtx->getParent();
+ return LCtx;
+}
+
+static const MemRegion *getInitializerRegion(const PostInitializer &PI) {
+ return reinterpret_cast<const MemRegion *>(PI.getLocationValue());
+}
+
+/// Based largely on isInitializationOfVar(). Checks if N initializes FR.
+static bool isInitializationOfField(const ExplodedNode *N,
+ const FieldRegion *FR) {
+ std::optional<PostInitializer> P = N->getLocationAs<PostInitializer>();
+ if (!P)
+ return false;
+
+ const MemRegion *InitR = getInitializerRegion(*P);
+
+ if (FR != InitR)
+ return false;
+
+ const MemSpaceRegion *FRSpace = FR->getMemorySpace();
+ const auto *FRCtx = dyn_cast<StackSpaceRegion>(FRSpace);
+
+ if (!FRCtx)
+ return true;
+
+ // The stack frame of N is the constructor, but the FieldRegion is not local
+ // to the constructor, but rather to the caller function.
+ const LocationContext *CallerCtx =
+ getFirstNonCtorCall(N->getLocationContext());
+
+ return FRCtx->getStackFrame() == CallerCtx->getStackFrame();
+}
+
+static bool isConstructorCallFor(const SubRegion *RegionOfInterest,
+ const ExplodedNode *N) {
+
+ if (N->getStackFrame()->inTopFrame())
+ return false;
+
+ ProgramStateRef State = N->getState();
+
+ CallEventRef<> Call =
+ State->getStateManager().getCallEventManager().getCaller(
+ N->getStackFrame(), State);
+
+ if (const CXXConstructorCall *CC = dyn_cast<CXXConstructorCall>(Call)) {
+ if (CC->getCXXThisVal().getAsRegion() == RegionOfInterest) {
+ return true;
+ }
+ }
+ return false;
+}
+
/// \return Whether \c RegionOfInterest was modified at \p N,
/// where \p ValueAfter is \c RegionOfInterest's value at the end of the
/// stack frame.
@@ -324,10 +380,22 @@ static bool wasRegionOfInterestModifiedAt(const SubRegion *RegionOfInterest,
ProgramStateRef State = N->getState();
ProgramStateManager &Mgr = N->getState()->getStateManager();
+ // If the region of interest is constructed right here, it is obviously
+ // modifying. This gets rid of notes like "Retuning without writing to *this",
+ // which makes no sense right in the constructor call.
+ if (isConstructorCallFor(RegionOfInterest, N))
+ return true;
+
if (!N->getLocationAs<PostStore>() && !N->getLocationAs<PostInitializer>() &&
!N->getLocationAs<PostStmt>())
return false;
+ // If we are tracking a field, check if we reached its initialization point
+ // in an initializer list.
+ if (const FieldRegion *FR = RegionOfInterest->getAs<FieldRegion>())
+ if (isInitializationOfField(N, FR))
+ return true;
+
// Writing into region of interest.
if (auto PS = N->getLocationAs<PostStmt>())
if (auto *BO = PS->getStmtAs<BinaryOperator>())
@@ -335,8 +403,12 @@ static bool wasRegionOfInterestModifiedAt(const SubRegion *RegionOfInterest,
N->getSVal(BO->getLHS()).getAsRegion()))
return true;
- // SVal after the state is possibly different.
SVal ValueAtN = N->getState()->getSVal(RegionOfInterest);
+
+ // If ValueAtN and ValueAfter are different, then RegionOfInterest was
+ // modified. We also need to handle the special case of undefined values,
+ // we need to check that the other value is not undefined, only then was
+ // the region modified.
if (!Mgr.getSValBuilder()
.areEqual(State, ValueAtN, ValueAfter)
.isConstrainedTrue() &&
@@ -526,91 +598,6 @@ PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode(
// 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<const MemRegion *, 5>;
-
-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<RegionVector>
- 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/SValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index eb9cde5c8918fc..c40f686818db8e 100644
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -36,6 +36,7 @@
#include "llvm/ADT/APSInt.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/Compiler.h"
+#include "llvm/Support/ErrorHandling.h"
#include <cassert>
#include <optional>
#include <tuple>
@@ -490,6 +491,47 @@ SVal SValBuilder::evalUnaryOp(ProgramStateRef state, UnaryOperator::Opcode opc,
llvm_unreachable("Unexpected unary operator");
}
+namespace {
+/// Iterate through to store to find the actual value this LazyCompoundVal
+/// corresponds to. Further reading is in LazyCompoundVal's docs.
+struct LazyHandler : public StoreManager::BindingsHandler {
+ nonloc::LazyCompoundVal l;
+ std::optional<SVal> &Ret;
+
+ LazyHandler(nonloc::LazyCompoundVal l, std::optional<SVal> &Ret)
+ : l(l), Ret(Ret) {}
+
+ virtual bool HandleBinding(StoreManager &SMgr, Store Store,
+ const MemRegion *Region, SVal Val) override {
+ if (const MemRegion *MR = Val.getAsRegion()) {
+ if (MR->isSubRegionOf(l.getRegion()) && MR != l.getAsRegion()) {
+ Ret = Val;
+ return false;
+ }
+ }
+ return true;
+ }
+};
+} // namespace
+
+/// Find the actual value behind a LazyCompoundVal. May return none if the
+/// query fails, or only finds another LazyCompoundVal (e.g. circular
+/// reference).
+static std::optional<SVal> extractActualValueFrom(ProgramStateRef State,
+ nonloc::LazyCompoundVal L) {
+ std::optional<SVal> Ret;
+ LazyHandler handler{L, Ret};
+ State->getStateManager().getStoreManager().iterBindings(State->getStore(),
+ handler);
+ if (Ret) {
+ std::optional<SVal> RVal = Ret = State->getSVal(Ret->getAsRegion());
+ // If our best efforts lead us back to another LazyCompoundValue, give up.
+ if (Ret == RVal)
+ return {};
+ }
+ return Ret;
+}
+
SVal SValBuilder::evalBinOp(ProgramStateRef state, BinaryOperator::Opcode op,
SVal lhs, SVal rhs, QualType type) {
if (lhs.isUndef() || rhs.isUndef())
@@ -498,8 +540,20 @@ SVal SValBuilder::evalBinOp(ProgramStateRef state, BinaryOperator::Opcode op,
if (lhs.isUnknown() || rhs.isUnknown())
return UnknownVal();
- if (isa<nonloc::LazyCompoundVal>(lhs) || isa<nonloc::LazyCompoundVal>(rhs)) {
- return UnknownVal();
+ if (isa<nonloc::LazyCompoundVal>(lhs)) {
+ std::optional<SVal> LhsVal =
+ extractActualValueFrom(state, lhs.castAs<nonloc::LazyCompoundVal>());
+ if (!LhsVal)
+ return UnknownVal();
+ return evalBinOp(state, op, *LhsVal, rhs, type);
+ }
+
+ if (isa<nonloc::LazyCompoundVal>(rhs)) {
+ std::optional<SVal> RhsVal =
+ extractActualValueFrom(state, rhs.castAs<nonloc::LazyCompoundVal>());
+ if (!RhsVal)
+ return UnknownVal();
+ return evalBinOp(state, op, lhs, *RhsVal, type);
}
if (op == BinaryOperatorKind::BO_Cmp) {
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 at -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 at +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 at -1{{The first element of the 2nd argument is undefined}}
+ // expected-note at -2{{Other elements might also be undefined}}
+}
More information about the cfe-commits
mailing list