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 <cfe-commits@lists.llvm.org> 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<nonloc::LazyCompoundVal>(); > + if (!LLCV) > + return false; > + > + const auto RLCV = RightVal.getAs<nonloc::LazyCompoundVal>(); > + 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<PostStore> PS = Succ->getLocationAs<PostStore>(); > if (!PS || PS->getLocationValue() != R) > return nullptr; > @@ -1198,6 +1224,7 @@ FindLastStoreBRVisitor::VisitNode(const > // UndefinedVal.) > if (Optional<CallEnter> CE = Succ->getLocationAs<CallEnter>()) { > if (const auto *VR = dyn_cast<VarRegion>(R)) { > + > const auto *Param = cast<ParmVarDecl>(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=diff > ============================================================================== > --- cfe/trunk/test/Analysis/uninit-vals.m (original) > +++ cfe/trunk/test/Analysis/uninit-vals.m Wed Feb 13 04:25:47 2019 > @@ -394,11 +394,11 @@ void testSmallStructBitfieldsFirstUnname > struct { > int : 4; > int y : 4; > - } a, b, c; > + } a, b, c; // expected-note{{'c' initialized here}} > > a.y = 2; > > - b = a; // expected-note{{Value assigned to 'c'}} > + b = a; > clang_analyzer_eval(b.y == 2); // expected-warning{{TRUE}} > // expected-note@-1{{TRUE}} > > @@ -411,11 +411,11 @@ void testSmallStructBitfieldsSecondUnnam > struct { > int x : 4; > int : 4; > - } a, b, c; > + } a, b, c; // expected-note{{'c' initialized here}} > > a.x = 1; > > - b = a; // expected-note{{Value assigned to 'c'}} > + b = a; > clang_analyzer_eval(b.x == 1); // expected-warning{{TRUE}} > // expected-note@-1{{TRUE}} > > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
