[analyzer] Improve FuchsiaHandleChecker's diagnostic messages

[xazax.hun]

Summary

I found that this information is useful when trying to understand bug reports. Moreover other checks seem to also report parameter indices.

[NoQ]

Nice! These are indeed useful when multiple things can happen. You may also consider adding stack hints a-la MallocChecker (i.e., "Returning; handle was closed").

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

314

I think this commend would have actually made a better note text. Maybe mention the function name as well, "Function foo() returns an open handle".

355

I think we often use "through" in this context. I.e., "Handle released through 2nd parameter".

359

What was wrong with good old ""?

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

359

I had two returns with different types so I either need to specify the return type of the lambda explicitly or make the two returns return the same type.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

359

Oh, right.

I think i specified the return type explicitly in most places but that's not a must.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
   ProgramStateRef State = C.getState();
 
   std::vector<std::function<std::string(BugReport & BR)>> Notes;
   SymbolRef ResultSymbol = nullptr;
   if (const auto *TypeDefTy = FuncDecl->getReturnType()->getAs<TypedefType>())
     if (TypeDefTy->getDecl()->getName() == ErrorTypeName)
       ResultSymbol = Call.getReturnValue().getAsSymbol();
 
   // Function returns an open handle.
   if (hasFuchsiaAttr<AcquireHandleAttr>(FuncDecl)) {
     SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
+    Notes.push_back([RetSym, FuncDecl](BugReport &BR) -> std::string {
+      auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
+      if (auto IsInteresting = PathBR->getInterestingnessKind(RetSym)) {
+        std::string SBuf;
+        llvm::raw_string_ostream OS(SBuf);
+        OS << "Function '" << FuncDecl->getNameAsString()
+           << "' returns an open handle";
+        return OS.str();
+      } else
+        return "";
+    });
     State =
         State->set<HStateMap>(RetSym, HandleState::getMaybeAllocated(nullptr));
   }
 
   for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
     if (Arg >= FuncDecl->getNumParams())
       break;
     const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
+    unsigned ParamDiagIdx = PVD->getFunctionScopeIndex() + 1;
     SymbolRef Handle =
         getFuchsiaHandleSymbol(PVD->getType(), Call.getArgSVal(Arg), State);
     if (!Handle)
       continue;
 
     const HandleState *HState = State->get<HStateMap>(Handle);
     if (HState && HState->isEscaped())
       continue;
     if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
       if (HState && HState->isReleased()) {
         reportDoubleRelease(Handle, Call.getArgSourceRange(Arg), C);
         return;
       } else {
-        Notes.push_back([Handle](BugReport &BR) {
+        Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string {
           auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
           if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
-            return "Handle released here.";
+            std::string SBuf;
+            llvm::raw_string_ostream OS(SBuf);
+            OS << "Handle released through " << ParamDiagIdx
+               << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
+            return OS.str();
           } else
             return "";
         });
         State = State->set<HStateMap>(Handle, HandleState::getReleased());
       }
     } else if (hasFuchsiaAttr<AcquireHandleAttr>(PVD)) {
-      Notes.push_back([Handle](BugReport &BR) {
+      Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string {
         auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
         if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
-          return "Handle allocated here.";
+          std::string SBuf;
+          llvm::raw_string_ostream OS(SBuf);
+          OS << "Handle allocated through " << ParamDiagIdx
+             << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter";
+          return OS.str();
         } else
           return "";
       });
       State = State->set<HStateMap>(
           Handle, HandleState::getMaybeAllocated(ResultSymbol));
     }
   }
   const NoteTag *T = nullptr;

clang/test/Analysis/fuchsia_handle.cpp

 zx_status_t zx_channel_create(
     uint32_t options,
     zx_handle_t *out0 ZX_HANDLE_ACQUIRE,
     zx_handle_t *out1 ZX_HANDLE_ACQUIRE);
 
 zx_status_t zx_handle_close(
     zx_handle_t handle ZX_HANDLE_RELEASE);
 
+ZX_HANDLE_ACQUIRE
+zx_handle_t return_handle();
+
 void escape1(zx_handle_t *in);
 void escape2(zx_handle_t in);
 void (*escape3)(zx_handle_t) = escape2; 
 
 void use1(const zx_handle_t *in ZX_HANDLE_USE);
 void use2(zx_handle_t in ZX_HANDLE_USE);
 
 void moreArgs(zx_handle_t, int, ...);
void checkNoLeak06() {
   if (zx_channel_create(0, &sa, &sb))
     return;
   zx_handle_close(sa);
   zx_handle_close(sb);
 } 
 
 void checkLeak01(int tag) {
   zx_handle_t sa, sb;
-  if (zx_channel_create(0, &sa, &sb)) // expected-note    {{Handle allocated here}}
+  if (zx_channel_create(0, &sa, &sb)) // expected-note    {{Handle allocated through 2nd parameter}}
     return;                           // expected-note@-1 {{Assuming the condition is false}}
                                       // expected-note@-2 {{Taking false branch}}
   use1(&sa);
   if (tag) // expected-note {{Assuming 'tag' is 0}}
     zx_handle_close(sa);
   // expected-note@-2 {{Taking false branch}}
   use2(sb); // expected-warning {{Potential leak of handle}}
   // expected-note@-1 {{Potential leak of handle}}
   zx_handle_close(sb);
 }
 
+void checkLeakFromReturn01(int tag) {
+  zx_handle_t sa = return_handle(); // expected-note {{Function 'return_handle' returns an open handle}}
+  (void)sa;
+} // expected-note {{Potential leak of handle}}
+  // expected-warning@-1 {{Potential leak of handle}}
+
 void checkReportLeakOnOnePath(int tag) {
   zx_handle_t sa, sb;
-  if (zx_channel_create(0, &sa, &sb)) // expected-note {{Handle allocated here}}
+  if (zx_channel_create(0, &sa, &sb)) // expected-note {{Handle allocated through 2nd parameter}}
     return;                           // expected-note@-1 {{Assuming the condition is false}}
                                       // expected-note@-2 {{Taking false branch}}
   zx_handle_close(sb);
   switch(tag) { // expected-note {{Control jumps to the 'default' case at line}} 
     case 0:
       use2(sa);
       return;
     case 1:
default:
       return; // expected-warning {{Potential leak of handle}}
               // expected-note@-1 {{Potential leak of handle}}
   }
 }
 
 void checkDoubleRelease01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
-  // expected-note@-1 {{Handle allocated here}}
+  // expected-note@-1 {{Handle allocated through 2nd parameter}}
   if (tag) // expected-note {{Assuming 'tag' is not equal to 0}}
-    zx_handle_close(sa); // expected-note {{Handle released here}}
+    zx_handle_close(sa); // expected-note {{Handle released through 1st parameter}}
   // expected-note@-2 {{Taking true branch}}
   zx_handle_close(sa); // expected-warning {{Releasing a previously released handle}}
   // expected-note@-1 {{Releasing a previously released handle}}
   zx_handle_close(sb);
 }
 
 void checkUseAfterFree01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
-  // expected-note@-1 {{Handle allocated here}}
-  // expected-note@-2 {{Handle allocated here}}
+  // expected-note@-1 {{Handle allocated through 2nd parameter}}
+  // expected-note@-2 {{Handle allocated through 3rd parameter}}
   // expected-note@+2 {{Taking true branch}}
   // expected-note@+1 {{Taking false branch}}
   if (tag) {
     // expected-note@-1 {{Assuming 'tag' is not equal to 0}}
-    zx_handle_close(sa); // expected-note {{Handle released here}}
+    zx_handle_close(sa); // expected-note {{Handle released through 1st parameter}}
     use1(&sa); // expected-warning {{Using a previously released handle}}
     // expected-note@-1 {{Using a previously released handle}}
   }
   // expected-note@-6 {{Assuming 'tag' is 0}}
-  zx_handle_close(sb); // expected-note {{Handle released here}}
+  zx_handle_close(sb); // expected-note {{Handle released through 1st parameter}}
   use2(sb); // expected-warning {{Using a previously released handle}}
   // expected-note@-1 {{Using a previously released handle}}
 }
 
 void checkMemberOperatorIndices() {
   zx_handle_t sa, sb, sc;
   zx_channel_create(0, &sa, &sb);
   zx_handle_close(sb);