From: Kristof Umann Date: Wed, 14 Aug 2019 09:39:38 +0000 (+0000) Subject: [analyzer] Note last writes to a condition only in a nested stackframe X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=be65cfe09ce1c0c619bceddda6d8d02b6e772979;p=clang [analyzer] Note last writes to a condition only in a nested stackframe Exactly what it says on the tin! The comments in the code detail this a little more too. Differential Revision: https://reviews.llvm.org/D64272 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@368817 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index 4ca38079d0..9069270a38 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -132,6 +132,7 @@ class FindLastStoreBRVisitor final : public BugReporterVisitor { using TrackingKind = bugreporter::TrackingKind; TrackingKind TKind; + const StackFrameContext *OriginSFC; public: /// Creates a visitor for every VarDecl inside a Stmt and registers it with @@ -145,11 +146,18 @@ public: /// \param EnableNullFPSuppression Whether we should employ false positive /// suppression (inlined defensive checks, returned null). /// \param TKind May limit the amount of notes added to the bug report. + /// \param OriginSFC Only adds notes when the last store happened in a + /// different stackframe to this one. Disregarded if the tracking kind + /// is thorough. + /// This is useful, because for non-tracked regions, notes about + /// changes to its value in a nested stackframe could be pruned, and + /// this visitor can prevent that without polluting the bugpath too + /// much. FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R, - bool InEnableNullFPSuppression, - TrackingKind TKind) + bool InEnableNullFPSuppression, TrackingKind TKind, + const StackFrameContext *OriginSFC = nullptr) : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression), - TKind(TKind) { + TKind(TKind), OriginSFC(OriginSFC) { assert(R); } diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index f0c8de26bc..8c17a367b8 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1440,6 +1440,10 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ, StoreSite, InitE, BR, TKind, EnableNullFPSuppression); } + if (TKind == TrackingKind::Condition && + !OriginSFC->isParentOf(StoreSite->getStackFrame())) + return nullptr; + // Okay, we've found the binding. Emit an appropriate message. SmallString<256> sbuf; llvm::raw_svector_ostream os(sbuf); @@ -1465,7 +1469,7 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ, if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) { if (auto KV = State->getSVal(OriginalR).getAs()) BR.addVisitor(llvm::make_unique( - *KV, OriginalR, EnableNullFPSuppression, TKind)); + *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC)); } } } @@ -1890,6 +1894,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, return false; ProgramStateRef LVState = LVNode->getState(); + const StackFrameContext *SFC = LVNode->getStackFrame(); // We only track expressions if we believe that they are important. Chances // are good that control dependencies to the tracking point are also improtant @@ -1926,7 +1931,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, if (RR && !LVIsNull) if (auto KV = LVal.getAs()) report.addVisitor(llvm::make_unique( - *KV, RR, EnableNullFPSuppression, TKind)); + *KV, RR, EnableNullFPSuppression, TKind, SFC)); // In case of C++ references, we want to differentiate between a null // reference and reference to null pointer. @@ -1963,7 +1968,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, if (auto KV = V.getAs()) report.addVisitor(llvm::make_unique( - *KV, R, EnableNullFPSuppression, TKind)); + *KV, R, EnableNullFPSuppression, TKind, SFC)); return true; } } @@ -2002,7 +2007,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, if (CanDereference) if (auto KV = RVal.getAs()) report.addVisitor(llvm::make_unique( - *KV, L->getRegion(), EnableNullFPSuppression, TKind)); + *KV, L->getRegion(), EnableNullFPSuppression, TKind, SFC)); const MemRegion *RegionRVal = RVal.getAsRegion(); if (RegionRVal && isa(RegionRVal)) { diff --git a/test/Analysis/track-control-dependency-conditions.cpp b/test/Analysis/track-control-dependency-conditions.cpp index 644eb6296f..35769f2473 100644 --- a/test/Analysis/track-control-dependency-conditions.cpp +++ b/test/Analysis/track-control-dependency-conditions.cpp @@ -127,10 +127,9 @@ int bar; void test() { int *x = 0; // expected-note{{'x' initialized to a null pointer value}} - if (int flag = foo()) // tracking-note{{'flag' initialized here}} - // debug-note@-1{{Tracking condition 'flag'}} - // expected-note@-2{{Assuming 'flag' is not equal to 0}} - // expected-note@-3{{Taking true branch}} + if (int flag = foo()) // debug-note{{Tracking condition 'flag'}} + // expected-note@-1{{Assuming 'flag' is not equal to 0}} + // expected-note@-2{{Taking true branch}} *x = 5; // expected-warning{{Dereference of null pointer}} // expected-note@-1{{Dereference of null pointer}} @@ -157,6 +156,28 @@ void test() { } // end of namespace variable_declaration_in_condition +namespace note_from_different_but_not_nested_stackframe { + +void nullptrDeref(int *ptr, bool True) { + if (True) // expected-note{{'True' is true}} + // expected-note@-1{{Taking true branch}} + // debug-note@-2{{Tracking condition 'True}} + *ptr = 5; + // expected-note@-1{{Dereference of null pointer (loaded from variable 'ptr')}} + // expected-warning@-2{{Dereference of null pointer (loaded from variable 'ptr')}} +} + +void f() { + int *ptr = nullptr; + // expected-note@-1{{'ptr' initialized to a null pointer value}} + bool True = true; + nullptrDeref(ptr, True); + // expected-note@-1{{Passing null pointer value via 1st parameter 'ptr'}} + // expected-note@-2{{Calling 'nullptrDeref'}} +} + +} // end of namespace note_from_different_but_not_nested_stackframe + namespace important_returning_pointer_loaded_from { bool coin(); @@ -194,8 +215,8 @@ bool coin(); int *getIntPtr(); int *conjurePointer() { - int *i = getIntPtr(); // tracking-note{{'i' initialized here}} - return i; // tracking-note{{Returning pointer (loaded from 'i')}} + int *i = getIntPtr(); + return i; } void f(int *ptr) { @@ -203,11 +224,9 @@ void f(int *ptr) { // expected-note@-1{{Taking false branch}} ; if (!conjurePointer()) - // tracking-note@-1{{Calling 'conjurePointer'}} - // tracking-note@-2{{Returning from 'conjurePointer'}} - // debug-note@-3{{Tracking condition '!conjurePointer()'}} - // expected-note@-4{{Assuming the condition is true}} - // expected-note@-5{{Taking true branch}} + // debug-note@-1{{Tracking condition '!conjurePointer()'}} + // expected-note@-2{{Assuming the condition is true}} + // expected-note@-3{{Taking true branch}} *ptr = 5; // expected-warning{{Dereference of null pointer}} // expected-note@-1{{Dereference of null pointer}} } @@ -225,10 +244,9 @@ void f() { int *x = 0; // expected-note{{'x' initialized to a null pointer value}} if (cast(conjure())) - // tracking-note@-1{{Passing value via 1st parameter 'P'}} - // debug-note@-2{{Tracking condition 'cast(conjure())'}} - // expected-note@-3{{Assuming the condition is false}} - // expected-note@-4{{Taking false branch}} + // debug-note@-1{{Tracking condition 'cast(conjure())'}} + // expected-note@-2{{Assuming the condition is false}} + // expected-note@-3{{Taking false branch}} return; *x = 5; // expected-warning{{Dereference of null pointer}} // expected-note@-1{{Dereference of null pointer}} @@ -314,7 +332,7 @@ namespace tracked_condition_is_only_initialized { int getInt(); void f() { - int flag = getInt(); // tracking-note{{'flag' initialized here}} + int flag = getInt(); int *x = 0; // expected-note{{'x' initialized to a null pointer value}} if (flag) // expected-note{{Assuming 'flag' is not equal to 0}} // expected-note@-1{{Taking true branch}} @@ -329,8 +347,8 @@ int flag; int getInt(); void f(int y) { - y = 1; // tracking-note{{The value 1 is assigned to 'y'}} - flag = y; // tracking-note{{The value 1 is assigned to 'flag'}} + y = 1; + flag = y; int *x = 0; // expected-note{{'x' initialized to a null pointer value}} if (flag) // expected-note{{'flag' is 1}} @@ -347,9 +365,8 @@ int getInt(); void foo() { int y; - y = 1; // tracking-note{{The value 1 is assigned to 'y'}} + y = 1; flag = y; // tracking-note{{The value 1 is assigned to 'flag'}} - } void f(int y) { @@ -378,9 +395,9 @@ void f() { int *x = 0; // expected-note{{'x' initialized to a null pointer value}} int y = 0; - foo(); // tracking-note{{Calling 'foo'}} - // tracking-note@-1{{Returning from 'foo'}} - y = flag; // tracking-note{{Value assigned to 'y'}} + foo(); // tracking-note{{Calling 'foo'}} + // tracking-note@-1{{Returning from 'foo'}} + y = flag; if (y) // expected-note{{Assuming 'y' is not equal to 0}} // expected-note@-1{{Taking true branch}} @@ -429,7 +446,7 @@ int getInt(); void f(int flag) { int *x = 0; // expected-note{{'x' initialized to a null pointer value}} - flag = getInt(); // tracking-note{{Value assigned to 'flag'}} + flag = getInt(); assert(flag); // tracking-note{{Calling 'assert'}} // tracking-note@-1{{Returning from 'assert'}}