Support struct annotations in FuchsiaHandleChecker.

[aabbaabb]

Summary

Support adding handle annotations to sturucture that contains handles.
All the handles referenced by the structure (direct value or ptr) would
be treated as containing the release/use/acquire annotations directly.

This could be used to capture more error cases for example use after
free:

void spawn(ZX_HANDLE_RELEASE const fdio_spawn_action_t* actions);

int main() {
  zx_handle_t handle;
  get_handle(&handle);
  fdio_spawn_action actions[1] = {};
  actions[0].h.handle = handle;
  spawn(actions);
  use_handle(handle);
  return 0;
}

In this example, spawn would release the handle so using the handle
again should be considered an error. Before this patch, handle would
be considered escaped and thus using it again is not reported.

[phosek]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

231

Can you move this to the private section below?

233

236

238

No need for { and } around a block with a single statement.

250

261

This variable shouldn't be needed at all.

266–267

268–269

275–281

315–316

This can be a SmallVector.

381–382

Ditto, this can be a SmallVector.

523–530

Ditto, this can be a SmallVector.

[xazax.hun]

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

264

I wonder whether this will actually pick up handles outside of the structure. I.e. when the structure has a pointer field to a handle instead of the handle directly. What is the desired result in that case?

[aabbaabb]

In D91223#2393921, @xazax.hun wrote:

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

[aabbaabb]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

264

In my local test, it is smart enough to find even double pointers (**zx_handle_t) inside the structure.

[xazax.hun]

In D91223#2395368, @aabbaabb wrote:

In D91223#2393921, @xazax.hun wrote:

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

We have the tests here: https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/fuchsia_handle.cpp
In LLVM, we have a very strong requirement to commit tests together with features (unless it is really hard to devise a test).
As a result, you can almost always find the location of the tests by looking at the commit that created the file or introduced the feature.

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

264

This is great, but I think that contradicts the comment above: handle inside the structure.
One could argue, the handle in this case is not inside.

I think you should change to comment to reflect this and make sure this is the desired behavior. I.e. I want to know whether this is what the users would intuitively expect and want.

[aabbaabb]

In D91223#2396841, @xazax.hun wrote:

In D91223#2395368, @aabbaabb wrote:

In D91223#2393921, @xazax.hun wrote:

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

We have the tests here: https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/fuchsia_handle.cpp
In LLVM, we have a very strong requirement to commit tests together with features (unless it is really hard to devise a test).
As a result, you can almost always find the location of the tests by looking at the commit that created the file or introduced the feature.

Thanks, I didn't find it. Added the tests.

[aabbaabb]

In D91223#2396841, @xazax.hun wrote:

In D91223#2395368, @aabbaabb wrote:

In D91223#2393921, @xazax.hun wrote:

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

We have the tests here: https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/fuchsia_handle.cpp
In LLVM, we have a very strong requirement to commit tests together with features (unless it is really hard to devise a test).
As a result, you can almost always find the location of the tests by looking at the commit that created the file or introduced the feature.

I also have some questions:

1. This patch does not work for case where the structure is passed in as pointer. The structure becomes lazycompoundval and I could not find the symbol there. Do you know what is going on?
2. If the structure contains a union, the escape logic could not correctly find the escaped handle:

struct HandleStruct {
  zx_handle_t handle;
  union {
    struct {
      zx_handle_t handle;
    } h;
  };
};

zx_handle_t handle;
get_handle(&handle);
HandleStruct hs = {
  .handle = handle
};
// Call this function could escape the handle. 
call_function(hs);

If you change the code to:

HandleStruct hs = {
  .h.handle = handle;
}
// Call this function would not escape the handle. The handle is not in the escaped array. 
call_function(hs);

If you change it to:

HandleStruct hs;
hs.h.handle = handle;
// Call this function would escape handle even though this should be the same as the previous case.
call_function(hs);

This is causing false positives where handle are actually escaped but considered as leaked. Do you know why?

[xazax.hun]

In D91223#2402038, @aabbaabb wrote:

I also have some questions:

1. This patch does not work for case where the structure is passed in as pointer. The structure becomes lazycompoundval and I could not find the symbol there. Do you know what is going on?

Invoking scanReachableSymbols on the pointer itself would also not work?

2. If the structure contains a union, the escape logic could not correctly find the escaped handle:

Unfortunately, union support is rather incomplete in the analyzer. There could be many problems in code relying on unions. In case you want to solve this problem it is very likely that you will need to improve the analyzer itself. You might be able to add workarounds in the checker to avoid false positives, like stop following symbols after it was stored into a union eagerly.

clang/test/Analysis/fuchsia_handle.cpp

2 ↗

(On Diff #305635)

Is this change intended?

[aabbaabb]

In D91223#2402183, @xazax.hun wrote:

In D91223#2402038, @aabbaabb wrote:

I also have some questions:

1. This patch does not work for case where the structure is passed in as pointer. The structure becomes lazycompoundval and I could not find the symbol there. Do you know what is going on?

Invoking scanReachableSymbols on the pointer itself would also not work?

2. If the structure contains a union, the escape logic could not correctly find the escaped handle:

Unfortunately, union support is rather incomplete in the analyzer. There could be many problems in code relying on unions. In case you want to solve this problem it is very likely that you will need to improve the analyzer itself. You might be able to add workarounds in the checker to avoid false positives, like stop following symbols after it was stored into a union eagerly.

Thanks.

1. Yes, that would also not work. I have dumped the visited symbols and I see a (conj_$3, int, XXX) symbol which I do not understand where does it come from, the handle symbol should be (conj_$1, zx_handle_t, XXX):

typedef uint32_t zx_handle_t;

void get_handle(ZX_HANDLE_ACQUIRE zx_handle_t* handle);

void release_handle_struct(ZX_HANDLE_RELEASE HandleStruct* s);

int main() {
  zx_handle_t handle;
  get_handle(&handle);
  HandleStruct action {
    .handle = handle,
    .a = 10
  };
  release_handle_struct(&action);
  return 0;
}

I use this code to test and the getFuchsiaHandles post the release_handle_struct call is unable to find the handle symbol.

2. Okay, if that's the case, I would ignore the union case for now.

[xazax.hun]

I think I might have an idea of what is going on. The handles are released in a PostCall. This is intended, as the handles are in the released state AFTER the call. When you call an unknown function with a pointer, that function is free to modify the contents of the pointer. Since the analyzer has no knowledge about the body of the function, it will do a conservative evaluation, escape the symbols in the struct, and create new symbols. This is why new symbols might be introduced. I think you could subscribe to escapes and check if you access the old symbols there. If that is the case and the escape is the result of calling an annotated function, maybe you can correctly mark those symbols as deleted.

[aabbaabb]

In D91223#2405639, @xazax.hun wrote:

I think I might have an idea of what is going on. The handles are released in a PostCall. This is intended, as the handles are in the released state AFTER the call. When you call an unknown function with a pointer, that function is free to modify the contents of the pointer. Since the analyzer has no knowledge about the body of the function, it will do a conservative evaluation, escape the symbols in the struct, and create new symbols. This is why new symbols might be introduced. I think you could subscribe to escapes and check if you access the old symbols there. If that is the case and the escape is the result of calling an annotated function, maybe you can correctly mark those symbols as deleted.

Thanks, i would try to improve that in a separate patch. Do you have any more comments on this CL? Could you +2 this one?

[xazax.hun]

Overall looks good, but please resolve the two nits before commiting.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

264

The comment above is not updated yet.

clang/test/Analysis/fuchsia_handle.cpp

2 ↗

(On Diff #305635)

I think we don't need this change.

[xazax.hun]

LGTM!

[MaskRay]

@haowei https://llvm.org/docs/DeveloperPolicy.html#commit-messages Please use git commit --amend --author for future patches where the majority of the work is done by others.

[haowei]

In D91223#2415137, @MaskRay wrote:

@haowei https://llvm.org/docs/DeveloperPolicy.html#commit-messages Please use git commit --amend --author for future patches where the majority of the work is done by others.

Thanks for your remind.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp

 // release_func represents the functions or syscalls that may release a handle.
 // use_func represents the functions or syscall that requires an open handle.
 //
 // If a tracked handle dies in "Released" or "Not Tracked" state, we assume it
 // is properly used. Otherwise a bug and will be reported.
 //
 // Note that, the analyzer does not always know for sure if a function failed
 // or succeeded. In those cases we use the state MaybeAllocated.
-// Thus, the diagramm above captures the intent, not implementation details.
+// Thus, the diagram above captures the intent, not implementation details.
 //
 // Due to the fact that the number of handle related syscalls in Fuchsia
 // is large, we adopt the annotation attributes to descript syscalls'
 // operations(acquire/release/use) on handles instead of hardcoding
 // everything in the checker.
 //
 // We use following annotation attributes for handle related syscalls or
 // functions:
if (!State->get<HStateMap>(Sym)) {
         return N;
     }
     Pred = N;
     N = N->getFirstPred();
   }
   return nullptr;
 }
 
-/// Returns the symbols extracted from the argument or null if it cannot be
-/// found.
-static SymbolRef getFuchsiaHandleSymbol(QualType QT, SVal Arg,
-                                        ProgramStateRef State) {
+namespace {
+class FuchsiaHandleSymbolVisitor final : public SymbolVisitor {
+public:
+  FuchsiaHandleSymbolVisitor(ProgramStateRef State) : State(std::move(State)) {}
+  ProgramStateRef getState() const { return State; }
+
+  bool VisitSymbol(SymbolRef S) override {
+    if (const auto *HandleType = S->getType()->getAs<TypedefType>())
+      if (HandleType->getDecl()->getName() == HandleTypeName)
+        Symbols.push_back(S);
+    return true;
+  }
+
+  SmallVector<SymbolRef, 1024> GetSymbols() {
+    return Symbols;
+  }
+
+ private:
+  SmallVector<SymbolRef, 1024> Symbols;
+  ProgramStateRef State;
+};
+} // end anonymous namespace
+
+/// Returns the symbols extracted from the argument or empty vector if it cannot
+/// be found. It is unlikely to have over 1024 symbols in one argument.
+static SmallVector<SymbolRef, 1024> getFuchsiaHandleSymbols(
+    QualType QT, SVal Arg, ProgramStateRef State) {
   int PtrToHandleLevel = 0;
   while (QT->isAnyPointerType() || QT->isReferenceType()) {
     ++PtrToHandleLevel;
     QT = QT->getPointeeType();
   }
+  if (QT->isStructureType()) {
+    // If we see a structure, see if there is any handle inside the structure.
+    FuchsiaHandleSymbolVisitor Visitor(State);
+    State->scanReachableSymbols(Arg, Visitor);
+    return Visitor.GetSymbols();
+  }
   if (const auto *HandleType = QT->getAs<TypedefType>()) {
     if (HandleType->getDecl()->getName() != HandleTypeName)
-      return nullptr;
-    if (PtrToHandleLevel > 1) {
+      return {};
+    if (PtrToHandleLevel > 1)
       // Not supported yet.
-      return nullptr;
-    }
+      return {};
 
     if (PtrToHandleLevel == 0) {
-      return Arg.getAsSymbol();
+      SymbolRef Sym = Arg.getAsSymbol();
+      if (Sym) {
+        return {Sym};
+      } else {
+        return {};
+      }
     } else {
       assert(PtrToHandleLevel == 1);
-      if (Optional<Loc> ArgLoc = Arg.getAs<Loc>())
-        return State->getSVal(*ArgLoc).getAsSymbol();
+      if (Optional<Loc> ArgLoc = Arg.getAs<Loc>()) {
+        SymbolRef Sym = State->getSVal(*ArgLoc).getAsSymbol();
+        if (Sym) {
+          return {Sym};
+        } else {
+          return {};
         }
       }
-  return nullptr;
+    }
+  }
+  return {};
 }
 
 void FuchsiaHandleChecker::checkPreCall(const CallEvent &Call,
                                         CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FuncDecl) {
     // Unknown call, escape by value handles. They are not covered by
     // PointerEscape callback.
     for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
       if (SymbolRef Handle = Call.getArgSVal(Arg).getAsSymbol())
         State = State->set<HStateMap>(Handle, HandleState::getEscaped());
     }
     C.addTransition(State);
     return;
   }
 
   for (unsigned Arg = 0; Arg < Call.getNumArgs(); ++Arg) {
     if (Arg >= FuncDecl->getNumParams())
       break;
     const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
-    SymbolRef Handle =
-        getFuchsiaHandleSymbol(PVD->getType(), Call.getArgSVal(Arg), State);
-    if (!Handle)
-      continue;
+    SmallVector<SymbolRef, 1024> Handles =
+        getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
 
     // Handled in checkPostCall.
     if (hasFuchsiaAttr<ReleaseHandleAttr>(PVD) ||
         hasFuchsiaAttr<AcquireHandleAttr>(PVD))
       continue;
 
+    for (SymbolRef Handle: Handles) {
       const HandleState *HState = State->get<HStateMap>(Handle);
       if (!HState || HState->isEscaped())
         continue;
 
       if (hasFuchsiaAttr<UseHandleAttr>(PVD) || PVD->getType()->isIntegerType()) {
         if (HState->isReleased()) {
           reportUseAfterFree(Handle, Call.getArgSourceRange(Arg), C);
           return;
         }
       }
       if (!hasFuchsiaAttr<UseHandleAttr>(PVD) &&
           PVD->getType()->isIntegerType()) {
         // Working around integer by-value escapes.
         State = State->set<HStateMap>(Handle, HandleState::getEscaped());
       }
     }
+  }
   C.addTransition(State);
 }
 
 void FuchsiaHandleChecker::checkPostCall(const CallEvent &Call,
                                          CheckerContext &C) const {
   const FunctionDecl *FuncDecl = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
   if (!FuncDecl)
     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;
+    SmallVector<SymbolRef, 1024> Handles =
+        getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State);
 
+    for (SymbolRef Handle: Handles) {
       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, ParamDiagIdx](BugReport &BR) -> std::string {
             auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
             if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
               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, ParamDiagIdx](BugReport &BR) -> std::string {
           auto *PathBR = static_cast<PathSensitiveBugReport *>(&BR);
           if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) {
             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;
   if (!Notes.empty()) {
     T = C.getNoteTag([this, Notes{std::move(Notes)}](
                          PathSensitiveBugReport &BR) -> std::string {
           if (&BR.getBugType() != &UseAfterReleaseBugType &&
               &BR.getBugType() != &LeakBugType &&
               &BR.getBugType() != &DoubleReleaseBugType)
             return "";
ProgramStateRef FuchsiaHandleChecker::checkPointerEscape(
   // Not all calls should escape our symbols.
   if (FuncDecl &&
       (Kind == PSK_DirectEscapeOnCall || Kind == PSK_IndirectEscapeOnCall ||
        Kind == PSK_EscapeOutParameters)) {
     for (unsigned Arg = 0; Arg < Call->getNumArgs(); ++Arg) {
       if (Arg >= FuncDecl->getNumParams())
         break;
       const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg);
-      SymbolRef Handle =
-          getFuchsiaHandleSymbol(PVD->getType(), Call->getArgSVal(Arg), State);
-      if (!Handle)
-        continue;
+      SmallVector<SymbolRef, 1024> Handles =
+          getFuchsiaHandleSymbols(PVD->getType(), Call->getArgSVal(Arg), State);
+      for (SymbolRef Handle: Handles) {
         if (hasFuchsiaAttr<UseHandleAttr>(PVD) ||
-          hasFuchsiaAttr<ReleaseHandleAttr>(PVD))
+            hasFuchsiaAttr<ReleaseHandleAttr>(PVD)) {
           UnEscaped.insert(Handle);
         }
       }
+    }
+  }
 
   // For out params, we have to deal with derived symbols. See
   // MacOSKeychainAPIChecker for details.
   for (auto I : State->get<HStateMap>()) {
     if (Escaped.count(I.first) && !UnEscaped.count(I.first))
       State = State->set<HStateMap>(I.first, HandleState::getEscaped());
     if (const auto *SD = dyn_cast<SymbolDerived>(I.first)) {
       auto ParentSym = SD->getParentSymbol();