From: Csaba Dabis Date: Wed, 29 May 2019 20:29:02 +0000 (+0000) Subject: [analyzer] ConditionBRVisitor: MemberExpr support X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6a908cf3156939257474e8a18b4ae8981be3ea57;p=clang [analyzer] ConditionBRVisitor: MemberExpr support Summary: - Reviewers: NoQ, george.karpenkov Reviewed By: NoQ Subscribers: cfe-commits, xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp Tags: #clang Differential Revision: https://reviews.llvm.org/D58206 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@362026 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index 36372dacd3..ef5d327d39 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -202,6 +202,11 @@ public: BugReporterContext &BRC, BugReport &R, const ExplodedNode *N, bool TookTrue, bool IsAssuming); + std::shared_ptr + VisitTrueTest(const Expr *Cond, const MemberExpr *ME, BugReporterContext &BRC, + BugReport &R, const ExplodedNode *N, bool TookTrue, + bool IsAssuming); + std::shared_ptr VisitConditionVariable(StringRef LhsString, const Expr *CondVarExpr, BugReporterContext &BRC, BugReport &R, @@ -225,7 +230,8 @@ public: BugReporterContext &BRC, BugReport &R, const ExplodedNode *N, - Optional &prunable); + Optional &prunable, + bool IsSameFieldName); static bool isPieceMessageGeneric(const PathDiagnosticPiece *Piece); }; diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index fb5b375082..c45b519582 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1991,6 +1991,11 @@ ConditionBRVisitor::VisitTrueTest(const Expr *Cond, BugReporterContext &BRC, BRC, R, N, TookTrueTmp, IsAssuming)) return P; break; + case Stmt::MemberExprClass: + if (auto P = VisitTrueTest(Cond, cast(CondTmp), + BRC, R, N, TookTrueTmp, IsAssuming)) + return P; + break; case Stmt::UnaryOperatorClass: { const auto *UO = cast(CondTmp); if (UO->getOpcode() == UO_LNot) { @@ -2025,7 +2030,8 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex, BugReporterContext &BRC, BugReport &report, const ExplodedNode *N, - Optional &prunable) { + Optional &prunable, + bool IsSameFieldName) { const Expr *OriginalExpr = Ex; Ex = Ex->IgnoreParenCasts(); @@ -2091,6 +2097,17 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex, return false; } + if (const auto *ME = dyn_cast(Ex)) { + if (!IsSameFieldName) + Out << "field '" << ME->getMemberDecl()->getName() << '\''; + else + Out << '\'' + << Lexer::getSourceText( + CharSourceRange::getTokenRange(Ex->getSourceRange()), + BRC.getSourceManager(), BRC.getASTContext().getLangOpts(), 0) + << '\''; + } + return false; } @@ -2100,13 +2117,23 @@ std::shared_ptr ConditionBRVisitor::VisitTrueTest( bool shouldInvert = false; Optional shouldPrune; + // Check if the field name of the MemberExprs is ambiguous. Example: + // " 'a.d' is equal to 'h.d' " in 'test/Analysis/null-deref-path-notes.cpp'. + bool IsSameFieldName = false; + if (const auto *LhsME = + dyn_cast(BExpr->getLHS()->IgnoreParenCasts())) + if (const auto *RhsME = + dyn_cast(BExpr->getRHS()->IgnoreParenCasts())) + IsSameFieldName = LhsME->getMemberDecl()->getName() == + RhsME->getMemberDecl()->getName(); + SmallString<128> LhsString, RhsString; { llvm::raw_svector_ostream OutLHS(LhsString), OutRHS(RhsString); - const bool isVarLHS = patternMatch(BExpr->getLHS(), BExpr, OutLHS, - BRC, R, N, shouldPrune); - const bool isVarRHS = patternMatch(BExpr->getRHS(), BExpr, OutRHS, - BRC, R, N, shouldPrune); + const bool isVarLHS = patternMatch(BExpr->getLHS(), BExpr, OutLHS, BRC, R, + N, shouldPrune, IsSameFieldName); + const bool isVarRHS = patternMatch(BExpr->getRHS(), BExpr, OutRHS, BRC, R, + N, shouldPrune, IsSameFieldName); shouldInvert = !isVarLHS && isVarRHS; } @@ -2170,11 +2197,15 @@ std::shared_ptr ConditionBRVisitor::VisitTrueTest( const LocationContext *LCtx = N->getLocationContext(); PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx); + // Convert 'field ...' to 'Field ...' if it is a MemberExpr. + std::string Message = Out.str(); + Message[0] = toupper(Message[0]); + // If we know the value create a pop-up note. if (!IsAssuming) - return std::make_shared(Loc, Out.str()); + return std::make_shared(Loc, Message); - auto event = std::make_shared(Loc, Out.str()); + auto event = std::make_shared(Loc, Message); if (shouldPrune.hasValue()) event->setPrunable(shouldPrune.getValue()); return event; @@ -2246,6 +2277,30 @@ std::shared_ptr ConditionBRVisitor::VisitTrueTest( return std::move(event); } +std::shared_ptr ConditionBRVisitor::VisitTrueTest( + const Expr *Cond, const MemberExpr *ME, BugReporterContext &BRC, + BugReport &report, const ExplodedNode *N, bool TookTrue, bool IsAssuming) { + SmallString<256> Buf; + llvm::raw_svector_ostream Out(Buf); + + Out << (IsAssuming ? "Assuming field '" : "Field '") + << ME->getMemberDecl()->getName() << "' is "; + + if (!printValue(ME, Out, N, TookTrue, IsAssuming)) + return nullptr; + + const LocationContext *LCtx = N->getLocationContext(); + PathDiagnosticLocation Loc(Cond, BRC.getSourceManager(), LCtx); + if (!Loc.isValid() || !Loc.asLocation().isValid()) + return nullptr; + + // If we know the value create a pop-up note. + if (!IsAssuming) + return std::make_shared(Loc, Out.str()); + + return std::make_shared(Loc, Out.str()); +} + bool ConditionBRVisitor::printValue(const Expr *CondVarExpr, raw_ostream &Out, const ExplodedNode *N, bool TookTrue, bool IsAssuming) { diff --git a/test/Analysis/Inputs/expected-plists/edges-new.mm.plist b/test/Analysis/Inputs/expected-plists/edges-new.mm.plist index 4eca510c3f..b4c79018c6 100644 --- a/test/Analysis/Inputs/expected-plists/edges-new.mm.plist +++ b/test/Analysis/Inputs/expected-plists/edges-new.mm.plist @@ -22227,6 +22227,68 @@ file0 + end + + + line587 + col11 + file0 + + + line587 + col11 + file0 + + + + + + + kindpop-up + location + + line587 + col11 + file0 + + ranges + + + + line587 + col11 + file0 + + + line587 + col16 + file0 + + + + extended_message + Field 'b' is equal to 2 + message + Field 'b' is equal to 2 + + + kindcontrol + edges + + + start + + + line587 + col11 + file0 + + + line587 + col11 + file0 + + end diff --git a/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist b/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist index 35b14fba4c..47c3b8df09 100644 --- a/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist +++ b/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist @@ -165,9 +165,9 @@ depth0 extended_message - Assuming pointer value is null + Assuming field 'x' is null message - Assuming pointer value is null + Assuming field 'x' is null kindcontrol @@ -454,9 +454,9 @@ depth0 extended_message - Assuming pointer value is null + Assuming field 'x' is null message - Assuming pointer value is null + Assuming field 'x' is null kindcontrol diff --git a/test/Analysis/diagnostics/deref-track-symbolic-region.c b/test/Analysis/diagnostics/deref-track-symbolic-region.c index 63d0971b85..1a1190f934 100644 --- a/test/Analysis/diagnostics/deref-track-symbolic-region.c +++ b/test/Analysis/diagnostics/deref-track-symbolic-region.c @@ -15,8 +15,8 @@ void test(struct S syz, int *pp) { struct S *ps = &syz; if (ps->x) - //expected-note@-1{{Taking false branch}} - //expected-note@-2{{Assuming pointer value is null}} + //expected-note@-1{{Assuming field 'x' is null}} + //expected-note@-2{{Taking false branch}} m++; @@ -30,8 +30,8 @@ void testTrackConstraintBRVisitorIsTrackingTurnedOn(struct S syz, int *pp) { struct S *ps = &syz; if (ps->x) - //expected-note@-1{{Taking false branch}} - //expected-note@-2{{Assuming pointer value is null}} + //expected-note@-1{{Assuming field 'x' is null}} + //expected-note@-2{{Taking false branch}} m++; int *p = syz.x; //expected-note {{'p' initialized to a null pointer value}} diff --git a/test/Analysis/diagnostics/dtors.cpp b/test/Analysis/diagnostics/dtors.cpp index b3fe7ec803..18bedc61f9 100644 --- a/test/Analysis/diagnostics/dtors.cpp +++ b/test/Analysis/diagnostics/dtors.cpp @@ -16,10 +16,11 @@ struct smart_ptr { S *s; smart_ptr(S *); S *get() { - return (x || 0) ? nullptr : s; // expected-note{{Left side of '||' is false}} - // expected-note@-1{{'?' condition is false}} - // expected-warning@-2{{Use of memory after it is freed}} - // expected-note@-3{{Use of memory after it is freed}} + return (x || 0) ? nullptr : s; // expected-note{{Field 'x' is 0}} + // expected-note@-1{{Left side of '||' is false}} + // expected-note@-2{{'?' condition is false}} + // expected-warning@-3{{Use of memory after it is freed}} + // expected-note@-4{{Use of memory after it is freed}} } }; diff --git a/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist b/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist index f517d4dddb..c9fd8c848b 100644 --- a/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist +++ b/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist @@ -4271,9 +4271,9 @@ depth0 extended_message - Assuming pointer value is null + Assuming field 'arr' is null message - Assuming pointer value is null + Assuming field 'arr' is null kindcontrol diff --git a/test/Analysis/inlining/path-notes.cpp b/test/Analysis/inlining/path-notes.cpp index 4385923743..ef56cc76f8 100644 --- a/test/Analysis/inlining/path-notes.cpp +++ b/test/Analysis/inlining/path-notes.cpp @@ -231,7 +231,7 @@ struct Owner { }; void Owner::testGetDerefExprOnMemberExprWithADot() { - if (arr) // expected-note {{Assuming pointer value is null}} + if (arr) // expected-note {{Assuming field 'arr' is null}} // expected-note@-1 {{Taking false branch}} ; arr[1].x = 1; //expected-warning {{Dereference of null pointer}} diff --git a/test/Analysis/null-deref-path-notes.cpp b/test/Analysis/null-deref-path-notes.cpp index dd54b24e78..c7b0619e29 100644 --- a/test/Analysis/null-deref-path-notes.cpp +++ b/test/Analysis/null-deref-path-notes.cpp @@ -19,7 +19,7 @@ void c::f(B &g, int &i) { // expected-note@-1{{Array access (via field 'd') results in a null pointer dereference}} B h, a; // expected-note{{Value assigned to 'h.d'}} a.d == __null; // expected-note{{Assuming the condition is true}} - a.d != h.d; // expected-note{{Assuming pointer value is null}} + a.d != h.d; // expected-note{{Assuming 'a.d' is equal to 'h.d'}} f(h, b); // expected-note{{Calling 'c::f'}} } } diff --git a/test/Analysis/osobject-retain-release.cpp b/test/Analysis/osobject-retain-release.cpp index 9d7cd85442..10ef144bf3 100644 --- a/test/Analysis/osobject-retain-release.cpp +++ b/test/Analysis/osobject-retain-release.cpp @@ -601,16 +601,18 @@ void test_smart_ptr_uaf() { { OSObjectPtr p(obj); // expected-note{{Calling constructor for 'smart_ptr'}} // expected-note@-1{{Returning from constructor for 'smart_ptr'}} + // expected-note@os_smart_ptr.h:13{{Field 'pointer' is non-null}} // expected-note@os_smart_ptr.h:13{{Taking true branch}} // expected-note@os_smart_ptr.h:14{{Calling 'smart_ptr::_retain'}} // expected-note@os_smart_ptr.h:71{{Reference count incremented. The object now has a +2 retain count}} // expected-note@os_smart_ptr.h:14{{Returning from 'smart_ptr::_retain'}} } // expected-note{{Calling '~smart_ptr'}} + // expected-note@os_smart_ptr.h:35{{Field 'pointer' is non-null}} // expected-note@os_smart_ptr.h:35{{Taking true branch}} // expected-note@os_smart_ptr.h:36{{Calling 'smart_ptr::_release'}} // expected-note@os_smart_ptr.h:76{{Reference count decremented. The object now has a +1 retain count}} // expected-note@os_smart_ptr.h:36{{Returning from 'smart_ptr::_release'}} - // expected-note@-5{{Returning from '~smart_ptr'}} + // expected-note@-6{{Returning from '~smart_ptr'}} obj->release(); // expected-note{{Object released}} obj->release(); // expected-warning{{Reference-counted object is used after it is released}} // expected-note@-1{{Reference-counted object is used after it is released}} @@ -621,16 +623,18 @@ void test_smart_ptr_leak() { { OSObjectPtr p(obj); // expected-note{{Calling constructor for 'smart_ptr'}} // expected-note@-1{{Returning from constructor for 'smart_ptr'}} + // expected-note@os_smart_ptr.h:13{{Field 'pointer' is non-null}} // expected-note@os_smart_ptr.h:13{{Taking true branch}} // expected-note@os_smart_ptr.h:14{{Calling 'smart_ptr::_retain'}} // expected-note@os_smart_ptr.h:71{{Reference count incremented. The object now has a +2 retain count}} // expected-note@os_smart_ptr.h:14{{Returning from 'smart_ptr::_retain'}} } // expected-note{{Calling '~smart_ptr'}} + // expected-note@os_smart_ptr.h:35{{Field 'pointer' is non-null}} // expected-note@os_smart_ptr.h:35{{Taking true branch}} // expected-note@os_smart_ptr.h:36{{Calling 'smart_ptr::_release'}} // expected-note@os_smart_ptr.h:76{{Reference count decremented. The object now has a +1 retain count}} // expected-note@os_smart_ptr.h:36{{Returning from 'smart_ptr::_release'}} - // expected-note@-5{{Returning from '~smart_ptr'}} + // expected-note@-6{{Returning from '~smart_ptr'}} } // expected-warning{{Potential leak of an object stored into 'obj'}} // expected-note@-1{{Object leaked: object allocated and stored into 'obj' is not referenced later in this execution path and has a retain count of +1}} diff --git a/test/Analysis/uninit-vals.m b/test/Analysis/uninit-vals.m index 5b959c7bfe..b59f72b107 100644 --- a/test/Analysis/uninit-vals.m +++ b/test/Analysis/uninit-vals.m @@ -164,7 +164,7 @@ void PR14765_test() { // expected-note@-1{{TRUE}} testObj->origin = makePoint(0.0, 0.0); - if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is false}} + if (testObj->size > 0) { ; } // expected-note{{Assuming field 'size' is <= 0}} // expected-note@-1{{Taking false branch}} // FIXME: Assigning to 'testObj->origin' kills the default binding for the @@ -219,13 +219,13 @@ void PR14765_test_int() { // expected-note@-1{{TRUE}} testObj->origin = makeIntPoint(1, 2); - if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is false}} + if (testObj->size > 0) { ; } // expected-note{{Assuming field 'size' is <= 0}} // expected-note@-1{{Taking false branch}} - // expected-note@-2{{Assuming the condition is false}} + // expected-note@-2{{Assuming field 'size' is <= 0}} // expected-note@-3{{Taking false branch}} - // expected-note@-4{{Assuming the condition is false}} + // expected-note@-4{{Assuming field 'size' is <= 0}} // expected-note@-5{{Taking false branch}} - // expected-note@-6{{Assuming the condition is false}} + // expected-note@-6{{Assuming field 'size' is <= 0}} // expected-note@-7{{Taking false branch}} // FIXME: Assigning to 'testObj->origin' kills the default binding for the @@ -321,9 +321,12 @@ void testSmallStructInLargerStruct() { // expected-note@-1{{TRUE}} testObj->origin = makeIntPoint2D(1, 2); - if (testObj->size > 0) { ; } // expected-note{{Taking false branch}} + if (testObj->size > 0) { ; } // expected-note{{Field 'size' is <= 0}} // expected-note@-1{{Taking false branch}} - // expected-note@-2{{Taking false branch}} + // expected-note@-2{{Field 'size' is <= 0}} + // expected-note@-3{{Taking false branch}} + // expected-note@-4{{Field 'size' is <= 0}} + // expected-note@-5{{Taking false branch}} clang_analyzer_eval(testObj->size == 0); // expected-warning{{TRUE}} // expected-note@-1{{TRUE}}