[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.

<rdar://problem/13364378>

llvm-svn: 176601

4 files changed, 210 insertions, 35 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
index 1fa4ab9ff72..7a5d9936010 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
@@ -24,9 +24,13 @@ using namespace clang;
 using namespace ento;
 
 namespace {
-class ReturnUndefChecker : 
-    public Checker< check::PreStmt<ReturnStmt> > {
-  mutable OwningPtr<BuiltinBug> BT;
+class ReturnUndefChecker : public Checker< check::PreStmt<ReturnStmt> > {
+  mutable OwningPtr<BuiltinBug> BT_Undef;
+  mutable OwningPtr<BuiltinBug> 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<DefinedOrUnknownSVal>());
+    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/clang/test/Analysis/inlining/false-positive-suppression.cpp b/clang/test/Analysis/inlining/false-positive-suppression.cpp
index 7a26eead31b..f27c7cf364a 100644
--- a/clang/test/Analysis/inlining/false-positive-suppression.cpp
+++ b/clang/test/Analysis/inlining/false-positive-suppression.cpp
@@ -112,4 +112,16 @@ namespace References {
     // 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/clang/test/Analysis/inlining/path-notes.cpp b/clang/test/Analysis/inlining/path-notes.cpp
index 86b28640993..6c63e1400c7 100644
--- a/clang/test/Analysis/inlining/path-notes.cpp
+++ b/clang/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:  <key>diagnostics</key>
 // CHECK-NEXT:  <array>
 // CHECK-NEXT:   <dict>
@@ -3214,4 +3223,113 @@ namespace ReturnZeroNote {
 // CHECK-NEXT:    <key>file</key><integer>0</integer>
 // CHECK-NEXT:   </dict>
 // CHECK-NEXT:   </dict>
+// CHECK-NEXT:   <dict>
+// CHECK-NEXT:    <key>path</key>
+// CHECK-NEXT:    <array>
+// CHECK-NEXT:     <dict>
+// CHECK-NEXT:      <key>kind</key><string>event</string>
+// CHECK-NEXT:      <key>location</key>
+// CHECK-NEXT:      <dict>
+// CHECK-NEXT:       <key>line</key><integer>189</integer>
+// CHECK-NEXT:       <key>col</key><integer>3</integer>
+// CHECK-NEXT:       <key>file</key><integer>0</integer>
+// CHECK-NEXT:      </dict>
+// CHECK-NEXT:      <key>ranges</key>
+// CHECK-NEXT:      <array>
+// CHECK-NEXT:        <array>
+// CHECK-NEXT:         <dict>
+// CHECK-NEXT:          <key>line</key><integer>189</integer>
+// CHECK-NEXT:          <key>col</key><integer>3</integer>
+// CHECK-NEXT:          <key>file</key><integer>0</integer>
+// CHECK-NEXT:         </dict>
+// CHECK-NEXT:         <dict>
+// CHECK-NEXT:          <key>line</key><integer>189</integer>
+// CHECK-NEXT:          <key>col</key><integer>8</integer>
+// CHECK-NEXT:          <key>file</key><integer>0</integer>
+// CHECK-NEXT:         </dict>
+// CHECK-NEXT:        </array>
+// CHECK-NEXT:      </array>
+// CHECK-NEXT:      <key>depth</key><integer>0</integer>
+// CHECK-NEXT:      <key>extended_message</key>
+// CHECK-NEXT:      <string>&apos;x&apos; initialized to a null pointer value</string>
+// CHECK-NEXT:      <key>message</key>
+// CHECK-NEXT:      <string>&apos;x&apos; initialized to a null pointer value</string>
+// CHECK-NEXT:     </dict>
+// CHECK-NEXT:     <dict>
+// CHECK-NEXT:      <key>kind</key><string>control</string>
+// CHECK-NEXT:      <key>edges</key>
+// CHECK-NEXT:       <array>
+// CHECK-NEXT:        <dict>
+// CHECK-NEXT:         <key>start</key>
+// CHECK-NEXT:          <array>
+// CHECK-NEXT:           <dict>
+// CHECK-NEXT:            <key>line</key><integer>189</integer>
+// CHECK-NEXT:            <key>col</key><integer>3</integer>
+// CHECK-NEXT:            <key>file</key><integer>0</integer>
+// CHECK-NEXT:           </dict>
+// CHECK-NEXT:           <dict>
+// CHECK-NEXT:            <key>line</key><integer>189</integer>
+// CHECK-NEXT:            <key>col</key><integer>5</integer>
+// CHECK-NEXT:            <key>file</key><integer>0</integer>
+// CHECK-NEXT:           </dict>
+// CHECK-NEXT:          </array>
+// CHECK-NEXT:         <key>end</key>
+// CHECK-NEXT:          <array>
+// CHECK-NEXT:           <dict>
+// CHECK-NEXT:            <key>line</key><integer>191</integer>
+// CHECK-NEXT:            <key>col</key><integer>3</integer>
+// CHECK-NEXT:            <key>file</key><integer>0</integer>
+// CHECK-NEXT:           </dict>
+// CHECK-NEXT:           <dict>
+// CHECK-NEXT:            <key>line</key><integer>191</integer>
+// CHECK-NEXT:            <key>col</key><integer>8</integer>
+// CHECK-NEXT:            <key>file</key><integer>0</integer>
+// CHECK-NEXT:           </dict>
+// CHECK-NEXT:          </array>
+// CHECK-NEXT:        </dict>
+// CHECK-NEXT:       </array>
+// CHECK-NEXT:     </dict>
+// CHECK-NEXT:     <dict>
+// CHECK-NEXT:      <key>kind</key><string>event</string>
+// CHECK-NEXT:      <key>location</key>
+// CHECK-NEXT:      <dict>
+// CHECK-NEXT:       <key>line</key><integer>191</integer>
+// CHECK-NEXT:       <key>col</key><integer>3</integer>
+// CHECK-NEXT:       <key>file</key><integer>0</integer>
+// CHECK-NEXT:      </dict>
+// CHECK-NEXT:      <key>ranges</key>
+// CHECK-NEXT:      <array>
+// CHECK-NEXT:        <array>
+// CHECK-NEXT:         <dict>
+// CHECK-NEXT:          <key>line</key><integer>191</integer>
+// CHECK-NEXT:          <key>col</key><integer>10</integer>
+// CHECK-NEXT:          <key>file</key><integer>0</integer>
+// CHECK-NEXT:         </dict>
+// CHECK-NEXT:         <dict>
+// CHECK-NEXT:          <key>line</key><integer>191</integer>
+// CHECK-NEXT:          <key>col</key><integer>11</integer>
+// CHECK-NEXT:          <key>file</key><integer>0</integer>
+// CHECK-NEXT:         </dict>
+// CHECK-NEXT:        </array>
+// CHECK-NEXT:      </array>
+// CHECK-NEXT:      <key>depth</key><integer>0</integer>
+// CHECK-NEXT:      <key>extended_message</key>
+// CHECK-NEXT:      <string>Returning null reference</string>
+// CHECK-NEXT:      <key>message</key>
+// CHECK-NEXT:      <string>Returning null reference</string>
+// CHECK-NEXT:     </dict>
+// CHECK-NEXT:    </array>
+// CHECK-NEXT:    <key>description</key><string>Returning null reference</string>
+// CHECK-NEXT:    <key>category</key><string>Logic error</string>
+// CHECK-NEXT:    <key>type</key><string>Returning null reference</string>
+// CHECK-NEXT:   <key>issue_context_kind</key><string>function</string>
+// CHECK-NEXT:   <key>issue_context</key><string>returnNullReference</string>
+// CHECK-NEXT:   <key>issue_hash</key><string>3</string>
+// CHECK-NEXT:   <key>location</key>
+// CHECK-NEXT:   <dict>
+// CHECK-NEXT:    <key>line</key><integer>191</integer>
+// CHECK-NEXT:    <key>col</key><integer>3</integer>
+// CHECK-NEXT:    <key>file</key><integer>0</integer>
+// CHECK-NEXT:   </dict>
+// CHECK-NEXT:   </dict>
 // CHECK-NEXT:  </array>
diff --git a/clang/test/Analysis/reference.cpp b/clang/test/Analysis/reference.cpp
index ce0ee8ed57d..ed05720fe66 100644
--- a/clang/test/Analysis/reference.cpp
+++ b/clang/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!
-  }
 }