From: Jordan Rose Date: Thu, 7 Mar 2013 01:23:25 +0000 (+0000) Subject: [analyzer] Check for returning null references in ReturnUndefChecker. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=c236b7327f989c1e7fe6b08a188bfef86727513d;p=clang [analyzer] Check for returning null references in ReturnUndefChecker. Officially in the C++ standard, a null reference cannot exist. However, it's still very easy to create one: int &getNullRef() { int *p = 0; return *p; } We already check that binds to reference regions don't create null references. This patch checks that we don't create null references by returning, either. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@176601 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp b/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp index 1fa4ab9ff7..7a5d993601 100644 --- a/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp @@ -24,9 +24,13 @@ using namespace clang; using namespace ento; namespace { -class ReturnUndefChecker : - public Checker< check::PreStmt > { - mutable OwningPtr BT; +class ReturnUndefChecker : public Checker< check::PreStmt > { + mutable OwningPtr BT_Undef; + mutable OwningPtr BT_NullReference; + + void emitUndef(CheckerContext &C, const Expr *RetE) const; + void checkReference(CheckerContext &C, const Expr *RetE, + DefinedOrUnknownSVal RetVal) const; public: void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const; }; @@ -34,43 +38,75 @@ public: void ReturnUndefChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const { - const Expr *RetE = RS->getRetValue(); if (!RetE) return; - - if (!C.getState()->getSVal(RetE, C.getLocationContext()).isUndef()) - return; - - // "return;" is modeled to evaluate to an UndefinedValue. Allow UndefinedValue - // to be returned in functions returning void to support the following pattern: - // void foo() { - // return; - // } - // void test() { - // return foo(); - // } + SVal RetVal = C.getSVal(RetE); + const StackFrameContext *SFC = C.getStackFrame(); QualType RT = CallEvent::getDeclaredResultType(SFC->getDecl()); - if (!RT.isNull() && RT->isSpecificBuiltinType(BuiltinType::Void)) + + if (RetVal.isUndef()) { + // "return;" is modeled to evaluate to an UndefinedVal. Allow UndefinedVal + // to be returned in functions returning void to support this pattern: + // void foo() { + // return; + // } + // void test() { + // return foo(); + // } + if (RT.isNull() || !RT->isVoidType()) + emitUndef(C, RetE); return; + } - ExplodedNode *N = C.generateSink(); + if (RT.isNull()) + return; + if (RT->isReferenceType()) { + checkReference(C, RetE, RetVal.castAs()); + return; + } +} + +static void emitBug(CheckerContext &C, BuiltinBug &BT, const Expr *RetE, + const Expr *TrackingE = 0) { + ExplodedNode *N = C.generateSink(); if (!N) return; - - if (!BT) - BT.reset(new BuiltinBug("Garbage return value", - "Undefined or garbage value returned to caller")); - - BugReport *report = - new BugReport(*BT, BT->getDescription(), N); - - report->addRange(RetE->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, RetE, *report); - - C.emitReport(report); + + BugReport *Report = new BugReport(BT, BT.getDescription(), N); + + Report->addRange(RetE->getSourceRange()); + bugreporter::trackNullOrUndefValue(N, TrackingE ? TrackingE : RetE, *Report); + + C.emitReport(Report); +} + +void ReturnUndefChecker::emitUndef(CheckerContext &C, const Expr *RetE) const { + if (!BT_Undef) + BT_Undef.reset(new BuiltinBug("Garbage return value", + "Undefined or garbage value " + "returned to caller")); + emitBug(C, *BT_Undef, RetE); +} + +void ReturnUndefChecker::checkReference(CheckerContext &C, const Expr *RetE, + DefinedOrUnknownSVal RetVal) const { + ProgramStateRef StNonNull, StNull; + llvm::tie(StNonNull, StNull) = C.getState()->assume(RetVal); + + if (StNonNull) { + // Going forward, assume the location is non-null. + C.addTransition(StNonNull); + return; + } + + // The return value is known to be null. Emit a bug report. + if (!BT_NullReference) + BT_NullReference.reset(new BuiltinBug("Returning null reference")); + + emitBug(C, *BT_NullReference, RetE, bugreporter::getDerefExpr(RetE)); } void ento::registerReturnUndefChecker(CheckerManager &mgr) { diff --git a/test/Analysis/inlining/false-positive-suppression.cpp b/test/Analysis/inlining/false-positive-suppression.cpp index 7a26eead31..f27c7cf364 100644 --- a/test/Analysis/inlining/false-positive-suppression.cpp +++ b/test/Analysis/inlining/false-positive-suppression.cpp @@ -110,6 +110,18 @@ namespace References { object->doSomething(); #ifndef SUPPRESSED // expected-warning@-2 {{Called C++ object pointer is null}} +#endif + } + + SomeClass *getNull() { + return 0; + } + + SomeClass &returnNullReference() { + SomeClass *x = getNull(); + return *x; +#ifndef SUPPRESSED + // expected-warning@-2 {{Returning null reference}} #endif } } diff --git a/test/Analysis/inlining/path-notes.cpp b/test/Analysis/inlining/path-notes.cpp index 86b2864099..6c63e1400c 100644 --- a/test/Analysis/inlining/path-notes.cpp +++ b/test/Analysis/inlining/path-notes.cpp @@ -184,6 +184,15 @@ namespace ReturnZeroNote { } } + +int &returnNullReference() { + int *x = 0; + // expected-note@-1 {{'x' initialized to a null pointer value}} + return *x; // expected-warning{{Returning null reference}} + // expected-note@-1 {{Returning null reference}} +} + + // CHECK: diagnostics // CHECK-NEXT: // CHECK-NEXT: @@ -3214,4 +3223,113 @@ namespace ReturnZeroNote { // 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: line189 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line189 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line189 +// CHECK-NEXT: col8 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: 'x' initialized to a null pointer value +// CHECK-NEXT: message +// CHECK-NEXT: 'x' initialized to a null pointer value +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: kindcontrol +// CHECK-NEXT: edges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: start +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line189 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line189 +// CHECK-NEXT: col5 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: end +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line191 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line191 +// 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: line191 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: ranges +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line191 +// CHECK-NEXT: col10 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: line191 +// CHECK-NEXT: col11 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: depth0 +// CHECK-NEXT: extended_message +// CHECK-NEXT: Returning null reference +// CHECK-NEXT: message +// CHECK-NEXT: Returning null reference +// CHECK-NEXT: +// CHECK-NEXT: +// CHECK-NEXT: descriptionReturning null reference +// CHECK-NEXT: categoryLogic error +// CHECK-NEXT: typeReturning null reference +// CHECK-NEXT: issue_context_kindfunction +// CHECK-NEXT: issue_contextreturnNullReference +// CHECK-NEXT: issue_hash3 +// CHECK-NEXT: location +// CHECK-NEXT: +// CHECK-NEXT: line191 +// CHECK-NEXT: col3 +// CHECK-NEXT: file0 +// CHECK-NEXT: +// CHECK-NEXT: // CHECK-NEXT: diff --git a/test/Analysis/reference.cpp b/test/Analysis/reference.cpp index ce0ee8ed57..ed05720fe6 100644 --- a/test/Analysis/reference.cpp +++ b/test/Analysis/reference.cpp @@ -135,6 +135,20 @@ void testFunctionPointerReturn(void *opaque) { clang_analyzer_eval(x == 42); // expected-warning{{TRUE}} } +int &testReturnNullReference() { + int *x = 0; + return *x; // expected-warning{{Returning null reference}} +} + +char &refFromPointer() { + return *ptr(); +} + +void testReturnReference() { + clang_analyzer_eval(ptr() == 0); // expected-warning{{UNKNOWN}} + clang_analyzer_eval(&refFromPointer() == 0); // expected-warning{{FALSE}} +} + // ------------------------------------ // False negatives @@ -147,9 +161,4 @@ namespace rdar11212286 { B *x = 0; return *x; // should warn here! } - - B &testRef() { - B *x = 0; - return *x; // should warn here! - } }