Re: r353943 - [Analyzer] Crash fix for FindLastStoreBRVisitor
Merged to release_80 in r354130. Please let me know if there are any follow-ups.
On Wed, Feb 13, 2019 at 1:25 PM Adam Balogh via cfe-commits
wrote:
>
> Author: baloghadamsoftware
> Date: Wed Feb 13 04:25:47 2019
> New Revision: 353943
>
> URL: http://llvm.org/viewvc/llvm-project?rev=353943&view=rev
> Log:
> [Analyzer] Crash fix for FindLastStoreBRVisitor
>
> FindLastStoreBRVisitor tries to find the first node in the exploded graph
> where
> the current value was assigned to a region. This node is called the "store
> site". It is identified by a pair of Pred and Succ nodes where Succ already
> has
> the binding for the value while Pred does not have it. However the visitor
> mistakenly identifies a node pair as the store site where the value is a
> `LazyCompoundVal` and `Pred` does not have a store yet but `Succ` has it. In
> this case the `LazyCompoundVal` is different in the `Pred` node because it
> also
> contains the store which is different in the two nodes. This error may lead to
> crashes (a declaration is cast to a parameter declaration without check) or
> misleading bug path notes.
>
> In this patch we fix this problem by checking for unequal `LazyCompoundVals`:
> if
> their region is equal, and their store is the same as the store of their nodes
> we consider them as equal when looking for the "store site". This is an
> approximation because we do not check for differences of the subvalues
> (structure members or array elements) in the stores.
>
> Differential Revision: https://reviews.llvm.org/D58067
>
>
> Added:
> cfe/trunk/test/Analysis/PR40625.cpp
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
> cfe/trunk/test/Analysis/uninit-vals.m
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=353943&r1=353942&r2=353943&view=diff
> ==
> --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Feb 13
> 04:25:47 2019
> @@ -153,6 +153,32 @@ const Expr *bugreporter::getDerefExpr(co
>return E;
> }
>
> +/// Comparing internal representations of symbolic values (via
> +/// SVal::operator==()) is a valid way to check if the value was updated,
> +/// unless it's a LazyCompoundVal that may have a different internal
> +/// representation every time it is loaded from the state. In this function
> we
> +/// do an approximate comparison for lazy compound values, checking that they
> +/// are the immediate snapshots of the tracked region's bindings within the
> +/// node's respective states but not really checking that these snapshots
> +/// actually contain the same set of bindings.
> +bool hasVisibleUpdate(const ExplodedNode *LeftNode, SVal LeftVal,
> + const ExplodedNode *RightNode, SVal RightVal) {
> + if (LeftVal == RightVal)
> +return true;
> +
> + const auto LLCV = LeftVal.getAs();
> + if (!LLCV)
> +return false;
> +
> + const auto RLCV = RightVal.getAs();
> + if (!RLCV)
> +return false;
> +
> + return LLCV->getRegion() == RLCV->getRegion() &&
> +LLCV->getStore() == LeftNode->getState()->getStore() &&
> +RLCV->getStore() == RightNode->getState()->getStore();
> +}
> +
>
> //===--===//
> // Definitions for bug reporter visitors.
>
> //===--===//
> @@ -1177,7 +1203,7 @@ FindLastStoreBRVisitor::VisitNode(const
> if (Succ->getState()->getSVal(R) != V)
>return nullptr;
>
> -if (Pred->getState()->getSVal(R) == V) {
> +if (hasVisibleUpdate(Pred, Pred->getState()->getSVal(R), Succ, V)) {
>Optional PS = Succ->getLocationAs();
>if (!PS || PS->getLocationValue() != R)
> return nullptr;
> @@ -1198,6 +1224,7 @@ FindLastStoreBRVisitor::VisitNode(const
> // UndefinedVal.)
> if (Optional CE = Succ->getLocationAs()) {
>if (const auto *VR = dyn_cast(R)) {
> +
> const auto *Param = cast(VR->getDecl());
>
> ProgramStateManager &StateMgr = BRC.getStateManager();
>
> Added: cfe/trunk/test/Analysis/PR40625.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/PR40625.cpp?rev=353943&view=auto
> ==
> --- cfe/trunk/test/Analysis/PR40625.cpp (added)
> +++ cfe/trunk/test/Analysis/PR40625.cpp Wed Feb 13 04:25:47 2019
> @@ -0,0 +1,16 @@
> +// RUN: %clang_analyze_cc1 -std=c++11
> -analyzer-checker=core,alpha.core.CallAndMessageUnInitRefArg %s -verify
> +
> +void f(const int *end);
> +
> +void g(const int (&arrr)[10]) {
> + f(arrr+sizeof(arrr)); // expected-warning{{1st function call argument is a
> pointer to uninitialized value}}
> + // FIXME: Th
r353943 - [Analyzer] Crash fix for FindLastStoreBRVisitor
Author: baloghadamsoftware
Date: Wed Feb 13 04:25:47 2019
New Revision: 353943
URL: http://llvm.org/viewvc/llvm-project?rev=353943&view=rev
Log:
[Analyzer] Crash fix for FindLastStoreBRVisitor
FindLastStoreBRVisitor tries to find the first node in the exploded graph where
the current value was assigned to a region. This node is called the "store
site". It is identified by a pair of Pred and Succ nodes where Succ already has
the binding for the value while Pred does not have it. However the visitor
mistakenly identifies a node pair as the store site where the value is a
`LazyCompoundVal` and `Pred` does not have a store yet but `Succ` has it. In
this case the `LazyCompoundVal` is different in the `Pred` node because it also
contains the store which is different in the two nodes. This error may lead to
crashes (a declaration is cast to a parameter declaration without check) or
misleading bug path notes.
In this patch we fix this problem by checking for unequal `LazyCompoundVals`: if
their region is equal, and their store is the same as the store of their nodes
we consider them as equal when looking for the "store site". This is an
approximation because we do not check for differences of the subvalues
(structure members or array elements) in the stores.
Differential Revision: https://reviews.llvm.org/D58067
Added:
cfe/trunk/test/Analysis/PR40625.cpp
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/test/Analysis/uninit-vals.m
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=353943&r1=353942&r2=353943&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Feb 13
04:25:47 2019
@@ -153,6 +153,32 @@ const Expr *bugreporter::getDerefExpr(co
return E;
}
+/// Comparing internal representations of symbolic values (via
+/// SVal::operator==()) is a valid way to check if the value was updated,
+/// unless it's a LazyCompoundVal that may have a different internal
+/// representation every time it is loaded from the state. In this function we
+/// do an approximate comparison for lazy compound values, checking that they
+/// are the immediate snapshots of the tracked region's bindings within the
+/// node's respective states but not really checking that these snapshots
+/// actually contain the same set of bindings.
+bool hasVisibleUpdate(const ExplodedNode *LeftNode, SVal LeftVal,
+ const ExplodedNode *RightNode, SVal RightVal) {
+ if (LeftVal == RightVal)
+return true;
+
+ const auto LLCV = LeftVal.getAs();
+ if (!LLCV)
+return false;
+
+ const auto RLCV = RightVal.getAs();
+ if (!RLCV)
+return false;
+
+ return LLCV->getRegion() == RLCV->getRegion() &&
+LLCV->getStore() == LeftNode->getState()->getStore() &&
+RLCV->getStore() == RightNode->getState()->getStore();
+}
+
//===--===//
// Definitions for bug reporter visitors.
//===--===//
@@ -1177,7 +1203,7 @@ FindLastStoreBRVisitor::VisitNode(const
if (Succ->getState()->getSVal(R) != V)
return nullptr;
-if (Pred->getState()->getSVal(R) == V) {
+if (hasVisibleUpdate(Pred, Pred->getState()->getSVal(R), Succ, V)) {
Optional PS = Succ->getLocationAs();
if (!PS || PS->getLocationValue() != R)
return nullptr;
@@ -1198,6 +1224,7 @@ FindLastStoreBRVisitor::VisitNode(const
// UndefinedVal.)
if (Optional CE = Succ->getLocationAs()) {
if (const auto *VR = dyn_cast(R)) {
+
const auto *Param = cast(VR->getDecl());
ProgramStateManager &StateMgr = BRC.getStateManager();
Added: cfe/trunk/test/Analysis/PR40625.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/PR40625.cpp?rev=353943&view=auto
==
--- cfe/trunk/test/Analysis/PR40625.cpp (added)
+++ cfe/trunk/test/Analysis/PR40625.cpp Wed Feb 13 04:25:47 2019
@@ -0,0 +1,16 @@
+// RUN: %clang_analyze_cc1 -std=c++11
-analyzer-checker=core,alpha.core.CallAndMessageUnInitRefArg %s -verify
+
+void f(const int *end);
+
+void g(const int (&arrr)[10]) {
+ f(arrr+sizeof(arrr)); // expected-warning{{1st function call argument is a
pointer to uninitialized value}}
+ // FIXME: This is a false positive that should be fixed. Until then this
+ //tests the crash fix in FindLastStoreBRVisitor (beside
+ //uninit-vals.m).
+}
+
+void h() {
+ int arr[10];
+
+ g(arr);
+}
Modified: cfe/trunk/test/Analysis/uninit-vals.m
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/uninit-vals.m?rev=353943&r1=353942&r2=353943&view=di
