From 0da7dda4eabb11c3ed7f7dc793d03d735e81d3cd Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Wed, 27 Sep 2017 09:33:37 +0000 Subject: [PATCH] [analyzer] Fix and refactor bugreporter::getDerefExpr() API. This API is used by checkers (and other entities) in order to track where does a value originate from, by jumping from an expression value of which is equal to that value to the expression from which this value has "appeared". For example, it may be an lvalue from which the rvalue was loaded, or a function call from which the dereferenced pointer was returned. The function now avoids incorrectly unwrapping implicit lvalue-to-rvalue casts, which caused crashes and incorrect intermediate diagnostic pieces. It also no longer relies on how the expression is written when guessing what it means. Fixes pr34373 and pr34731. rdar://problem/33594502 Differential Revision: https://reviews.llvm.org/D37023 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@314287 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Core/BugReporterVisitors.cpp | 80 +++--- test/Analysis/null-deref-path-notes.c | 10 + test/Analysis/null-deref-path-notes.cpp | 25 ++ test/Analysis/null-deref-path-notes.m | 240 ++++++++++++++++++ 4 files changed, 325 insertions(+), 30 deletions(-) create mode 100644 test/Analysis/null-deref-path-notes.c create mode 100644 test/Analysis/null-deref-path-notes.cpp diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index d00182a871..716f23b2bc 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -42,48 +42,68 @@ bool bugreporter::isDeclRefExprToReference(const Expr *E) { return false; } +/// Given that expression S represents a pointer that would be dereferenced, +/// try to find a sub-expression from which the pointer came from. +/// This is used for tracking down origins of a null or undefined value: +/// "this is null because that is null because that is null" etc. +/// We wipe away field and element offsets because they merely add offsets. +/// We also wipe away all casts except lvalue-to-rvalue casts, because the +/// latter represent an actual pointer dereference; however, we remove +/// the final lvalue-to-rvalue cast before returning from this function +/// because it demonstrates more clearly from where the pointer rvalue was +/// loaded. Examples: +/// x->y.z ==> x (lvalue) +/// foo()->y.z ==> foo() (rvalue) const Expr *bugreporter::getDerefExpr(const Stmt *S) { - // Pattern match for a few useful cases: - // a[0], p->f, *p const Expr *E = dyn_cast(S); if (!E) return nullptr; - E = E->IgnoreParenCasts(); while (true) { - if (const BinaryOperator *B = dyn_cast(E)) { - assert(B->isAssignmentOp()); - E = B->getLHS()->IgnoreParenCasts(); - continue; - } - else if (const UnaryOperator *U = dyn_cast(E)) { - if (U->getOpcode() == UO_Deref) - return U->getSubExpr()->IgnoreParenCasts(); - } - else if (const MemberExpr *ME = dyn_cast(E)) { - if (ME->isImplicitAccess()) { - return ME; - } else if (ME->isArrow() || isDeclRefExprToReference(ME->getBase())) { - return ME->getBase()->IgnoreParenCasts(); + if (const CastExpr *CE = dyn_cast(E)) { + if (CE->getCastKind() == CK_LValueToRValue) { + // This cast represents the load we're looking for. + break; + } + E = CE->getSubExpr(); + } else if (isa(E)) { + // Probably more arithmetic can be pattern-matched here, + // but for now give up. + break; + } else if (const UnaryOperator *U = dyn_cast(E)) { + if (U->getOpcode() == UO_Deref) { + // Operators '*' and '&' don't actually mean anything. + // We look at casts instead. + E = U->getSubExpr(); } else { - // If we have a member expr with a dot, the base must have been - // dereferenced. - return getDerefExpr(ME->getBase()); + // Probably more arithmetic can be pattern-matched here, + // but for now give up. + break; } } - else if (const ObjCIvarRefExpr *IvarRef = dyn_cast(E)) { - return IvarRef->getBase()->IgnoreParenCasts(); - } - else if (const ArraySubscriptExpr *AE = dyn_cast(E)) { - return getDerefExpr(AE->getBase()); - } - else if (isa(E)) { - return E; + // Pattern match for a few useful cases: a[0], p->f, *p etc. + else if (const MemberExpr *ME = dyn_cast(E)) { + E = ME->getBase(); + } else if (const ObjCIvarRefExpr *IvarRef = dyn_cast(E)) { + E = IvarRef->getBase(); + } else if (const ArraySubscriptExpr *AE = dyn_cast(E)) { + E = AE->getBase(); + } else if (const ParenExpr *PE = dyn_cast(E)) { + E = PE->getSubExpr(); + } else { + // Other arbitrary stuff. + break; } - break; } - return nullptr; + // Special case: remove the final lvalue-to-rvalue cast, but do not recurse + // deeper into the sub-expression. This way we return the lvalue from which + // our pointer rvalue was loaded. + if (const ImplicitCastExpr *CE = dyn_cast(E)) + if (CE->getCastKind() == CK_LValueToRValue) + E = CE->getSubExpr(); + + return E; } const Stmt *bugreporter::GetDenomExpr(const ExplodedNode *N) { diff --git a/test/Analysis/null-deref-path-notes.c b/test/Analysis/null-deref-path-notes.c new file mode 100644 index 0000000000..7070bc44c4 --- /dev/null +++ b/test/Analysis/null-deref-path-notes.c @@ -0,0 +1,10 @@ +// RUN: %clang_analyze_cc1 -w -x c -analyzer-checker=core -analyzer-output=text -verify %s + +// Avoid the crash when finding the expression for tracking the origins +// of the null pointer for path notes. Apparently, not much actual tracking +// needs to be done in this example. +void pr34373() { + int *a = 0; + (a + 0)[0]; // expected-warning{{Array access results in a null pointer dereference}} + // expected-note@-1{{Array access results in a null pointer dereference}} +} diff --git a/test/Analysis/null-deref-path-notes.cpp b/test/Analysis/null-deref-path-notes.cpp new file mode 100644 index 0000000000..617f5def15 --- /dev/null +++ b/test/Analysis/null-deref-path-notes.cpp @@ -0,0 +1,25 @@ +// RUN: %clang_analyze_cc1 -w -x c++ -analyzer-checker=core -analyzer-output=text -analyzer-eagerly-assume -verify %s + +namespace pr34731 { +int b; +class c { + class B { + public: + double ***d; + B(); + }; + void e(double **, int); + void f(B &, int &); +}; + +// Properly track the null pointer in the array field back to the default +// constructor of 'h'. +void c::f(B &g, int &i) { + e(g.d[9], i); // expected-warning{{Array access (via field 'd') results in a null pointer dereference}} + // 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}} + f(h, b); // expected-note{{Calling 'c::f'}} +} +} diff --git a/test/Analysis/null-deref-path-notes.m b/test/Analysis/null-deref-path-notes.m index 242b5daa9b..39cf9c79f3 100644 --- a/test/Analysis/null-deref-path-notes.m +++ b/test/Analysis/null-deref-path-notes.m @@ -50,6 +50,23 @@ void repeatedStores(int coin) { *p = 1; // expected-warning{{Dereference of null pointer}} expected-note{{Dereference of null pointer}} } +@interface WithArrayPtr +- (void) useArray; +@end + +@implementation WithArrayPtr { +@public int *p; +} +- (void)useArray { + p[1] = 2; // expected-warning{{Array access (via ivar 'p') results in a null pointer dereference}} + // expected-note@-1{{Array access (via ivar 'p') results in a null pointer dereference}} +} +@end + +void testWithArrayPtr(WithArrayPtr *w) { + w->p = 0; // expected-note{{Null pointer value stored to 'p'}} + [w useArray]; // expected-note{{Calling 'useArray'}} +} // CHECK: diagnostics // CHECK-NEXT: @@ -801,4 +818,227 @@ void repeatedStores(int coin) { // CHECK-NEXT: file0 // CHECK-NEXT: // CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: path +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line67 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line67 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line67 +// CHECK-NEXT: col10 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Null pointer value stored to 'p' +// CHECK-NEXT: message +// CHECK-NEXT: Null pointer value stored to 'p' +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line67 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line67 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line68 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line68 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line68 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line68 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line68 +// CHECK-NEXT: col14 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Calling 'useArray' +// CHECK-NEXT: message +// CHECK-NEXT: Calling 'useArray' +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line60 +// CHECK-NEXT: col1 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: depth1 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Entered call from 'testWithArrayPtr' +// CHECK-NEXT: message +// CHECK-NEXT: Entered call from 'testWithArrayPtr' +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line60 +// CHECK-NEXT: col1 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line60 +// CHECK-NEXT: col1 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col8 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col8 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindevent +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col8 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth1 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Array access (via ivar 'p') results in a null pointer dereference +// CHECK-NEXT: message +// CHECK-NEXT: Array access (via ivar 'p') results in a null pointer dereference +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: descriptionArray access (via ivar 'p') results in a null pointer dereference +// CHECK-NEXT: categoryLogic error +// CHECK-NEXT: typeDereference of null pointer +// CHECK-NEXT: check_namecore.NullDereference +// CHECK-NEXT: +// CHECK-NEXT: issue_hash_content_of_line_in_contextfb0ad1e4e3090d9834d542eb54bc9d2e +// CHECK-NEXT: issue_context_kindObjective-C method +// CHECK-NEXT: issue_contextuseArray +// CHECK-NEXT: issue_hash_function_offset1 +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line61 +// CHECK-NEXT: col8 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: // CHECK-NEXT: -- 2.50.1