diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -153,6 +153,28 @@ return E; } +static const MemRegion * +getLocationRegionIfReference(const Expr *E, const ExplodedNode *N, + bool LookingForReference = true) { + if (const auto *DR = dyn_cast(E)) { + if (const auto *VD = dyn_cast(DR->getDecl())) { + if (LookingForReference && !VD->getType()->isReferenceType()) + return nullptr; + return N->getState() + ->getLValue(VD, N->getLocationContext()) + .getAsRegion(); + } + } + + // FIXME: This does not handle other kinds of null references, + // for example, references from FieldRegions: + // struct Wrapper { int &ref; }; + // Wrapper w = { *(int *)0 }; + // w.ref = 1; + + return nullptr; +} + /// 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 @@ -1241,16 +1263,17 @@ /// Show diagnostics for initializing or declaring a region \p R with a bad value. static void showBRDiagnostics(const char *action, llvm::raw_svector_ostream &os, - const MemRegion *R, SVal V, const DeclStmt *DS) { - if (R->canPrintPretty()) { - R->printPretty(os); + const MemRegion *NewR, SVal V, + const MemRegion *OldR, const DeclStmt *DS) { + if (NewR->canPrintPretty()) { + NewR->printPretty(os); os << " "; } if (V.getAs()) { bool b = false; - if (R->isBoundable()) { - if (const auto *TR = dyn_cast(R)) { + if (NewR->isBoundable()) { + if (const auto *TR = dyn_cast(NewR)) { if (TR->getValueType()->isObjCObjectPointerType()) { os << action << "nil"; b = true; @@ -1262,29 +1285,31 @@ } else if (auto CVal = V.getAs()) { os << action << CVal->getValue(); + } else if (OldR && OldR->canPrintPretty()) { + os << action << "the value of "; + OldR->printPretty(os); } else if (DS) { if (V.isUndef()) { - if (isa(R)) { + if (isa(NewR)) { const auto *VD = cast(DS->getSingleDecl()); if (VD->getInit()) { - os << (R->canPrintPretty() ? "initialized" : "Initializing") - << " to a garbage value"; + os << (NewR->canPrintPretty() ? "initialized" : "Initializing") + << " to a garbage value"; } else { - os << (R->canPrintPretty() ? "declared" : "Declaring") - << " without an initial value"; + os << (NewR->canPrintPretty() ? "declared" : "Declaring") + << " without an initial value"; } } } else { - os << (R->canPrintPretty() ? "initialized" : "Initialized") - << " here"; + os << (NewR->canPrintPretty() ? "initialized" : "Initialized") << " here"; } } } /// Display diagnostics for passing bad region as a parameter. -static void showBRParamDiagnostics(llvm::raw_svector_ostream& os, - const VarRegion *VR, - SVal V) { +static void showBRParamDiagnostics(llvm::raw_svector_ostream &os, + const VarRegion *VR, SVal V, + const MemRegion *ValueR) { const auto *Param = cast(VR->getDecl()); os << "Passing "; @@ -1298,6 +1323,8 @@ os << "uninitialized value"; } else if (auto CI = V.getAs()) { os << "the value " << CI->getValue(); + } else if (ValueR && ValueR->canPrintPretty()) { + ValueR->printPretty(os); } else { os << "value"; } @@ -1313,11 +1340,12 @@ /// Show default diagnostics for storing bad region. static void showBRDefaultDiagnostics(llvm::raw_svector_ostream &os, - const MemRegion *R, SVal V) { + const MemRegion *NewR, SVal V, + const MemRegion *OldR) { if (V.getAs()) { bool b = false; - if (R->isBoundable()) { - if (const auto *TR = dyn_cast(R)) { + if (NewR->isBoundable()) { + if (const auto *TR = dyn_cast(NewR)) { if (TR->getValueType()->isObjCObjectPointerType()) { os << "nil object reference stored"; b = true; @@ -1325,34 +1353,44 @@ } } if (!b) { - if (R->canPrintPretty()) + if (NewR->canPrintPretty()) os << "Null pointer value stored"; else os << "Storing null pointer value"; } } else if (V.isUndef()) { - if (R->canPrintPretty()) + if (NewR->canPrintPretty()) os << "Uninitialized value stored"; else os << "Storing uninitialized value"; } else if (auto CV = V.getAs()) { - if (R->canPrintPretty()) + if (NewR->canPrintPretty()) os << "The value " << CV->getValue() << " is assigned"; else os << "Assigning " << CV->getValue(); + } else if (OldR && OldR->canPrintPretty()) { + if (NewR->canPrintPretty()) { + os << "The value of "; + OldR->printPretty(os); + os << " is assigned"; + } else { + os << "Assigning the value of "; + OldR->printPretty(os); + } + } else { - if (R->canPrintPretty()) + if (NewR->canPrintPretty()) os << "Value assigned"; else os << "Assigning value"; } - if (R->canPrintPretty()) { + if (NewR->canPrintPretty()) { os << " to "; - R->printPretty(os); + NewR->printPretty(os); } } @@ -1451,8 +1489,76 @@ if (!IsParam) InitE = InitE->IgnoreParenCasts(); - bugreporter::trackExpressionValue( - StoreSite, InitE, BR, TKind, EnableNullFPSuppression); + bugreporter::trackExpressionValue(StoreSite, InitE, BR, TKind, + EnableNullFPSuppression); + } + + // Let's try to find the region where the value came from. + const MemRegion *OldRegion = nullptr; + + // If we have init expression, it might be simply a reference + // to a variable, so we can use it. + if (InitE) { + // That region might still be not exactly what we are looking for. + // In situations like `int &ref = val;`, we can't say that + // `ref` is initialized with `val`, rather refers to `val`. + // + // In order, to mitigate situations like this, we check if the last + // stored value in that region is the value that we track. + // + // TODO: support other situations better. + if (const MemRegion *Candidate = + getLocationRegionIfReference(InitE, Succ, false)) { + const StoreManager &SM = BRC.getStateManager().getStoreManager(); + + // Here we traverse the graph up to find the last node where the + // candidate region is still in the store. + for (const ExplodedNode *N = StoreSite; N; N = N->getFirstPred()) { + if (SM.includedInBindings(N->getState()->getStore(), Candidate)) { + // And if it was bound to the target value, we can use it. + if (N->getState()->getSVal(Candidate) == V) { + OldRegion = Candidate; + } + break; + } + } + } + } + + // Otherwise, if the current region does indeed contain the value + // we are looking for, we can look for a region where this value + // was before. + // + // It can be useful for situations like: + // new = identity(old) + // where the analyzer knows that 'identity' returns the value of its + // first argument. + // + // NOTE: If the region R is not a simple var region, it can contain + // V in one of its subregions. + if (!OldRegion && StoreSite->getState()->getSVal(R) == V) { + // Let's go up the graph to find the node where the region is + // bound to V. + const ExplodedNode *NodeWithoutBinding = StoreSite->getFirstPred(); + for (; + NodeWithoutBinding && NodeWithoutBinding->getState()->getSVal(R) == V; + NodeWithoutBinding = NodeWithoutBinding->getFirstPred()) { + } + + if (NodeWithoutBinding) { + // Let's try to find a unique binding for the value in that node. + // We want to use this to find unique bindings because of the following + // situations: + // b = a; + // c = identity(b); + // + // Telling the user that the value of 'a' is assigned to 'c', while + // correct, can be confusing. + StoreManager::FindUniqueBinding FB(V.getAsLocSymbol()); + BRC.getStateManager().iterBindings(NodeWithoutBinding->getState(), FB); + if (FB) + OldRegion = FB.getRegion(); + } } if (TKind == TrackingKind::Condition && @@ -1490,15 +1596,15 @@ } } if (action) - showBRDiagnostics(action, os, R, V, DS); + showBRDiagnostics(action, os, R, V, OldRegion, DS); } else if (StoreSite->getLocation().getAs()) { if (const auto *VR = dyn_cast(R)) - showBRParamDiagnostics(os, VR, V); + showBRParamDiagnostics(os, VR, V, OldRegion); } if (os.str().empty()) - showBRDefaultDiagnostics(os, R, V); + showBRDefaultDiagnostics(os, R, V, OldRegion); if (TKind == bugreporter::TrackingKind::Condition) os << WillBeUsedForACondition; @@ -1825,27 +1931,6 @@ // Implementation of trackExpressionValue. //===----------------------------------------------------------------------===// -static const MemRegion *getLocationRegionIfReference(const Expr *E, - const ExplodedNode *N) { - if (const auto *DR = dyn_cast(E)) { - if (const auto *VD = dyn_cast(DR->getDecl())) { - if (!VD->getType()->isReferenceType()) - return nullptr; - ProgramStateManager &StateMgr = N->getState()->getStateManager(); - MemRegionManager &MRMgr = StateMgr.getRegionManager(); - return MRMgr.getVarRegion(VD, N->getLocationContext()); - } - } - - // FIXME: This does not handle other kinds of null references, - // for example, references from FieldRegions: - // struct Wrapper { int &ref; }; - // Wrapper w = { *(int *)0 }; - // w.ref = 1; - - return nullptr; -} - /// \return A subexpression of @c Ex which represents the /// expression-of-interest. static const Expr *peelOffOuterExpr(const Expr *Ex, diff --git a/clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist b/clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist --- a/clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist +++ b/clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist @@ -5398,9 +5398,9 @@ depth0 extended_message - 'New' initialized here + 'New' initialized to the value of 'Original' message - 'New' initialized here + 'New' initialized to the value of 'Original' kindcontrol @@ -5886,9 +5886,9 @@ depth0 extended_message - 'Intermediate' initialized here + 'Intermediate' initialized to the value of 'Original' message - 'Intermediate' initialized here + 'Intermediate' initialized to the value of 'Original' kindcontrol @@ -5949,9 +5949,9 @@ depth0 extended_message - 'New' initialized here + 'New' initialized to the value of 'Intermediate' message - 'New' initialized here + 'New' initialized to the value of 'Intermediate' kindcontrol @@ -6438,9 +6438,9 @@ depth0 extended_message - Value assigned to 'New' + The value of 'Original' is assigned to 'New' message - Value assigned to 'New' + The value of 'Original' is assigned to 'New' kindcontrol diff --git a/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp b/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp --- a/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp +++ b/clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp @@ -28,16 +28,16 @@ } int testRefToNullPtr() { - int *p = 0; // expected-note {{'p' initialized to a null pointer value}} + int *p = 0; // expected-note {{'p' initialized to a null pointer value}} int *const &p2 = p; // expected-note{{'p2' initialized here}} - int *p3 = p2; // expected-note {{'p3' initialized to a null pointer value}} - return *p3; // expected-warning {{Dereference of null pointer}} - // expected-note@-1{{Dereference of null pointer}} + int *p3 = p2; // expected-note {{'p3' initialized to a null pointer value}} + return *p3; // expected-warning {{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} } int testRefToNullPtr2() { - int *p = 0; // expected-note {{'p' initialized to a null pointer value}} - int *const &p2 = p;// expected-note{{'p2' initialized here}} - return *p2; //expected-warning {{Dereference of null pointer}} - // expected-note@-1{{Dereference of null pointer}} -} \ No newline at end of file + int *p = 0; // expected-note {{'p' initialized to a null pointer value}} + int *const &p2 = p; // expected-note{{'p2' initialized here}} + return *p2; //expected-warning {{Dereference of null pointer}} + // expected-note@-1{{Dereference of null pointer}} +} diff --git a/clang/test/Analysis/osobject-retain-release.cpp b/clang/test/Analysis/osobject-retain-release.cpp --- a/clang/test/Analysis/osobject-retain-release.cpp +++ b/clang/test/Analysis/osobject-retain-release.cpp @@ -536,7 +536,7 @@ void check_dynamic_cast_alias() { OSObject *originalPtr = OSObject::generateObject(1); // expected-note {{Call to method 'OSObject::generateObject' returns an OSObject}} - OSArray *newPtr = OSDynamicCast(OSArray, originalPtr); // expected-note {{'newPtr' initialized here}} + OSArray *newPtr = OSDynamicCast(OSArray, originalPtr); // expected-note {{'newPtr' initialized to the value of 'originalPtr'}} if (newPtr) { // expected-note {{'newPtr' is non-null}} // expected-note@-1 {{Taking true branch}} originalPtr = OSObject::generateObject(42); @@ -549,7 +549,7 @@ void check_dynamic_cast_alias_cond() { OSObject *originalPtr = OSObject::generateObject(1); // expected-note {{Call to method 'OSObject::generateObject' returns an OSObject}} OSArray *newPtr = 0; - if ((newPtr = OSDynamicCast(OSArray, originalPtr))) { // expected-note {{Value assigned to 'newPtr'}} + if ((newPtr = OSDynamicCast(OSArray, originalPtr))) { // expected-note {{The value of 'originalPtr' is assigned to 'newPtr'}} // expected-note@-1 {{'newPtr' is non-null}} // expected-note@-2 {{Taking true branch}} originalPtr = OSObject::generateObject(42); @@ -563,7 +563,7 @@ OSObject *originalPtr = OSObject::generateObject(1); // expected-note {{Call to method 'OSObject::generateObject' returns an OSObject of type 'OSObject' with a +1 retain count}} OSObject *intermediate = originalPtr; // TODO: add note here as well OSArray *newPtr = 0; - if ((newPtr = OSDynamicCast(OSArray, intermediate))) { // expected-note {{Value assigned to 'newPtr'}} + if ((newPtr = OSDynamicCast(OSArray, intermediate))) { // expected-note {{The value of 'intermediate' is assigned to 'newPtr'}} // expected-note@-1 {{'newPtr' is non-null}} // expected-note@-2 {{Taking true branch}} intermediate = OSObject::generateObject(42); @@ -573,6 +573,21 @@ // expected-note@-1 {{Object leaked: object allocated and stored into 'newPtr' is not referenced later in this execution path and has a retain count of +1}} } +void check_dynamic_cast_alias_intermediate_2() { + OSObject *originalPtr = OSObject::generateObject(1); // expected-note {{Call to method 'OSObject::generateObject' returns an OSObject of type 'OSObject' with a +1 retain count}} + OSObject *intermediate = originalPtr; // TODO: add note here as well + OSArray *newPtr = 0; + if ((newPtr = OSDynamicCast(OSArray, intermediate))) { // expected-note {{Value assigned to 'newPtr'}} + // expected-note@-1 {{'newPtr' is non-null}} + // expected-note@-2 {{Taking true branch}} + intermediate = OSObject::generateObject(42); + (void)originalPtr; + } + (void)newPtr; + intermediate->release(); // expected-warning {{Potential leak of an object stored into 'newPtr'}} + // expected-note@-1 {{Object leaked: object allocated and stored into 'newPtr' is not referenced later in this execution path and has a retain count of +1}} +} + void use_after_release() { OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject of type 'OSArray' with a +1 retain count}} arr->release(); // expected-note{{Object released}} diff --git a/clang/test/Analysis/retain-release-path-notes.m b/clang/test/Analysis/retain-release-path-notes.m --- a/clang/test/Analysis/retain-release-path-notes.m +++ b/clang/test/Analysis/retain-release-path-notes.m @@ -339,7 +339,7 @@ // expected-note@216 {{Returning pointer (loaded from 'self')}} // expected-note@-3 {{Returning from 'initY'}} // expected-note@-4 {{'Original' initialized here}} - id New = Original; // expected-note {{'New' initialized here}} + id New = Original; // expected-note {{'New' initialized to the value of 'Original'}} Original = [[MyObj alloc] initZ]; (void)New; [Original release]; // expected-warning {{Potential leak of an object stored into 'New'}} @@ -352,8 +352,8 @@ // expected-note@216 {{Returning pointer (loaded from 'self')}} // expected-note@-3 {{Returning from 'initY'}} // expected-note@-4 {{'Original' initialized here}} - id Intermediate = Original; // expected-note {{'Intermediate' initialized here}} - id New = Intermediate; // expected-note {{'New' initialized here}} + id Intermediate = Original; // expected-note {{'Intermediate' initialized to the value of 'Original'}} + id New = Intermediate; // expected-note {{'New' initialized to the value of 'Intermediate'}} Original = [[MyObj alloc] initZ]; (void)New; [Original release]; // expected-warning {{Potential leak of an object stored into 'New'}} @@ -380,7 +380,7 @@ // expected-note@-3 {{Returning from 'initY'}} // expected-note@-4 {{'Original' initialized here}} id New = 0; - New = Original; // expected-note {{Value assigned to 'New'}} + New = Original; // expected-note {{The value of 'Original' is assigned to 'New'}} Original = [[MyObj alloc] initZ]; [self log:New with:[self calculate]]; [Original release]; // expected-warning {{Potential leak of an object stored into 'New'}} diff --git a/clang/test/Analysis/uninit-const.c b/clang/test/Analysis/uninit-const.c --- a/clang/test/Analysis/uninit-const.c +++ b/clang/test/Analysis/uninit-const.c @@ -37,7 +37,7 @@ void f_1_1(void) { int t; // expected-note {{'t' declared without an initial value}} int *tp1 = &t; // expected-note {{'tp1' initialized here}} - int* tp2 = tp1; // expected-note {{'tp2' initialized here}} + int *tp2 = tp1; // expected-note {{'tp2' initialized to the value of 'tp1'}} doStuff_pointerToConstInt(tp2); // expected-warning {{1st function call argument is a pointer to uninitialized value}} // expected-note@-1 {{1st function call argument is a pointer to uninitialized value}} } @@ -53,7 +53,7 @@ // expected-note@-1{{Calling 'f_2_sub'}} // expected-note@-2{{Returning from 'f_2_sub'}} // expected-note@-3{{'p' initialized here}} - int* tp = p; // expected-note {{'tp' initialized here}} + int *tp = p; // expected-note {{'tp' initialized to the value of 'p'}} doStuff_pointerToConstInt(tp); // expected-warning {{1st function call argument is a pointer to uninitialized value}} // expected-note@-1 {{1st function call argument is a pointer to uninitialized value}} } @@ -70,7 +70,7 @@ void f_5(void) { int ta[5]; // expected-note {{'ta' initialized here}} - int* tp = ta; // expected-note {{'tp' initialized here}} + int *tp = ta; // expected-note {{'tp' initialized here}} doStuff_pointerToConstInt(tp); // expected-warning {{1st function call argument is a pointer to uninitialized value}} // expected-note@-1 {{1st function call argument is a pointer to uninitialized value}} } diff --git a/clang/test/Analysis/uninit-const.cpp b/clang/test/Analysis/uninit-const.cpp --- a/clang/test/Analysis/uninit-const.cpp +++ b/clang/test/Analysis/uninit-const.cpp @@ -63,7 +63,7 @@ } void f6_1(void) { - int t; // expected-note{{'t' declared without an initial value}} + int t; // expected-note{{'t' declared without an initial value}} int p = f6_1_sub(t); //expected-warning {{Assigned value is garbage or undefined}} //expected-note@-1 {{Passing value via 1st parameter 'p'}} //expected-note@-2 {{Calling 'f6_1_sub'}} @@ -76,8 +76,8 @@ void f6_2(void) { int t; //expected-note {{'t' declared without an initial value}} int &p = t; //expected-note {{'p' initialized here}} - int &s = p; //expected-note {{'s' initialized here}} - int &q = s; //expected-note {{'q' initialized here}} + int &s = p; //expected-note {{'s' initialized to the value of 'p'}} + int &q = s; //expected-note {{'q' initialized to the value of 's'}} doStuff6(q); //expected-warning {{1st function call argument is an uninitialized value}} //expected-note@-1 {{1st function call argument is an uninitialized value}} }