Skip to content

Commit

Permalink
[analyzer] ConditionBRVisitor: MemberExpr support
Browse files Browse the repository at this point in the history 
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

llvm-svn: 362026
  • Loading branch information
@Charusso
Charusso committed May 29, 2019
1 parent 9942a99 commit d1f0ec3
Show file tree
Hide file tree
Showing 11 changed files with 164 additions and 33 deletions.
8 changes: 7 additions & 1 deletion clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
View file
Expand Up @@ -202,6 +202,11 @@ class ConditionBRVisitor final : public BugReporterVisitor {
BugReporterContext &BRC, BugReport &R, const ExplodedNode *N,
bool TookTrue, bool IsAssuming);

std::shared_ptr<PathDiagnosticPiece>
VisitTrueTest(const Expr *Cond, const MemberExpr *ME, BugReporterContext &BRC,
BugReport &R, const ExplodedNode *N, bool TookTrue,
bool IsAssuming);

std::shared_ptr<PathDiagnosticPiece>
VisitConditionVariable(StringRef LhsString, const Expr *CondVarExpr,
BugReporterContext &BRC, BugReport &R,
Expand All @@ -225,7 +230,8 @@ class ConditionBRVisitor final : public BugReporterVisitor {
BugReporterContext &BRC,
BugReport &R,
const ExplodedNode *N,
Optional<bool> &prunable);
Optional<bool> &prunable,
bool IsSameFieldName);

static bool isPieceMessageGeneric(const PathDiagnosticPiece *Piece);
};
Expand Down
69 changes: 62 additions & 7 deletions clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
View file
Expand Up @@ -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<MemberExpr>(CondTmp),
BRC, R, N, TookTrueTmp, IsAssuming))
return P;
break;
case Stmt::UnaryOperatorClass: {
const auto *UO = cast<UnaryOperator>(CondTmp);
if (UO->getOpcode() == UO_LNot) {
Expand Down Expand Up @@ -2025,7 +2030,8 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex,
BugReporterContext &BRC,
BugReport &report,
const ExplodedNode *N,
Optional<bool> &prunable) {
Optional<bool> &prunable,
bool IsSameFieldName) {
const Expr *OriginalExpr = Ex;
Ex = Ex->IgnoreParenCasts();

Expand Down Expand Up @@ -2091,6 +2097,17 @@ bool ConditionBRVisitor::patternMatch(const Expr *Ex,
return false;
}

if (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
if (!IsSameFieldName)
Out << "field '" << ME->getMemberDecl()->getName() << '\'';
else
Out << '\''
<< Lexer::getSourceText(
CharSourceRange::getTokenRange(Ex->getSourceRange()),
BRC.getSourceManager(), BRC.getASTContext().getLangOpts(), 0)
<< '\'';
}

return false;
}

Expand All @@ -2100,13 +2117,23 @@ std::shared_ptr<PathDiagnosticPiece> ConditionBRVisitor::VisitTrueTest(
bool shouldInvert = false;
Optional<bool> 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<MemberExpr>(BExpr->getLHS()->IgnoreParenCasts()))
if (const auto *RhsME =
dyn_cast<MemberExpr>(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;
}
Expand Down Expand Up @@ -2170,11 +2197,15 @@ std::shared_ptr<PathDiagnosticPiece> 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<PathDiagnosticPopUpPiece>(Loc, Out.str());
return std::make_shared<PathDiagnosticPopUpPiece>(Loc, Message);

auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
auto event = std::make_shared<PathDiagnosticEventPiece>(Loc, Message);
if (shouldPrune.hasValue())
event->setPrunable(shouldPrune.getValue());
return event;
Expand Down Expand Up @@ -2246,6 +2277,30 @@ std::shared_ptr<PathDiagnosticPiece> ConditionBRVisitor::VisitTrueTest(
return std::move(event);
}

std::shared_ptr<PathDiagnosticPiece> 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<PathDiagnosticPopUpPiece>(Loc, Out.str());

return std::make_shared<PathDiagnosticEventPiece>(Loc, Out.str());
}

bool ConditionBRVisitor::printValue(const Expr *CondVarExpr, raw_ostream &Out,
const ExplodedNode *N, bool TookTrue,
bool IsAssuming) {
Expand Down
62 changes: 62 additions & 0 deletions clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
View file
Expand Up @@ -22227,6 +22227,68 @@
<key>file</key><integer>0</integer>
</dict>
</array>
<key>end</key>
<array>
<dict>
<key>line</key><integer>587</integer>
<key>col</key><integer>11</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
<key>line</key><integer>587</integer>
<key>col</key><integer>11</integer>
<key>file</key><integer>0</integer>
</dict>
</array>
</dict>
</array>
</dict>
<dict>
<key>kind</key><string>pop-up</string>
<key>location</key>
<dict>
<key>line</key><integer>587</integer>
<key>col</key><integer>11</integer>
<key>file</key><integer>0</integer>
</dict>
<key>ranges</key>
<array>
<array>
<dict>
<key>line</key><integer>587</integer>
<key>col</key><integer>11</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
<key>line</key><integer>587</integer>
<key>col</key><integer>16</integer>
<key>file</key><integer>0</integer>
</dict>
</array>
</array>
<key>extended_message</key>
<string>Field &apos;b&apos; is equal to 2</string>
<key>message</key>
<string>Field &apos;b&apos; is equal to 2</string>
</dict>
<dict>
<key>kind</key><string>control</string>
<key>edges</key>
<array>
<dict>
<key>start</key>
<array>
<dict>
<key>line</key><integer>587</integer>
<key>col</key><integer>11</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
<key>line</key><integer>587</integer>
<key>col</key><integer>11</integer>
<key>file</key><integer>0</integer>
</dict>
</array>
<key>end</key>
<array>
<dict>
Expand Down
8 changes: 4 additions & 4 deletions clang/test/Analysis/diagnostics/Inputs/expected-plists/deref-track-symbolic-region.c.plist
View file
Expand Up @@ -165,9 +165,9 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>Assuming pointer value is null</string>
<string>Assuming field &apos;x&apos; is null</string>
<key>message</key>
<string>Assuming pointer value is null</string>
<string>Assuming field &apos;x&apos; is null</string>
</dict>
<dict>
<key>kind</key><string>control</string>
Expand Down Expand Up @@ -454,9 +454,9 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>Assuming pointer value is null</string>
<string>Assuming field &apos;x&apos; is null</string>
<key>message</key>
<string>Assuming pointer value is null</string>
<string>Assuming field &apos;x&apos; is null</string>
</dict>
<dict>
<key>kind</key><string>control</string>
Expand Down
8 changes: 4 additions & 4 deletions clang/test/Analysis/diagnostics/deref-track-symbolic-region.c
View file
Expand Up @@ -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++;

Expand All @@ -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}}
Expand Down
9 changes: 5 additions & 4 deletions clang/test/Analysis/diagnostics/dtors.cpp
View file
Expand Up @@ -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}}
}
};

Expand Down
4 changes: 2 additions & 2 deletions clang/test/Analysis/inlining/Inputs/expected-plists/path-notes.cpp.plist
View file
Expand Up @@ -4271,9 +4271,9 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
<string>Assuming pointer value is null</string>
<string>Assuming field &apos;arr&apos; is null</string>
<key>message</key>
<string>Assuming pointer value is null</string>
<string>Assuming field &apos;arr&apos; is null</string>
</dict>
<dict>
<key>kind</key><string>control</string>
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/inlining/path-notes.cpp
View file
Expand Up @@ -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}}
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/null-deref-path-notes.cpp
View file
Expand Up @@ -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'}}
}
}
8 changes: 6 additions & 2 deletions clang/test/Analysis/osobject-retain-release.cpp
View file
Expand Up @@ -601,16 +601,18 @@ void test_smart_ptr_uaf() {
{
OSObjectPtr p(obj); // expected-note{{Calling constructor for 'smart_ptr<OSObject>'}}
// expected-note@-1{{Returning from constructor for 'smart_ptr<OSObject>'}}
// 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}}
Expand All @@ -621,16 +623,18 @@ void test_smart_ptr_leak() {
{
OSObjectPtr p(obj); // expected-note{{Calling constructor for 'smart_ptr<OSObject>'}}
// expected-note@-1{{Returning from constructor for 'smart_ptr<OSObject>'}}
// 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}}

Expand Down
17 changes: 10 additions & 7 deletions clang/test/Analysis/uninit-vals.m
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}}
Expand Down

0 comments on commit d1f0ec3

Please sign in to comment.