diff --git a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp @@ -53,7 +53,7 @@ // // 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' @@ -226,32 +226,70 @@ 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()) + if (HandleType->getDecl()->getName() == HandleTypeName) + Symbols.push_back(S); + return true; + } + + SmallVector GetSymbols() { return Symbols; } + +private: + SmallVector 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 +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 referenced by the + // structure. + FuchsiaHandleSymbolVisitor Visitor(State); + State->scanReachableSymbols(Arg, Visitor); + return Visitor.GetSymbols(); + } if (const auto *HandleType = QT->getAs()) { 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 ArgLoc = Arg.getAs()) - return State->getSVal(*ArgLoc).getAsSymbol(); + if (Optional ArgLoc = Arg.getAs()) { + SymbolRef Sym = State->getSVal(*ArgLoc).getAsSymbol(); + if (Sym) { + return {Sym}; + } else { + return {}; + } + } } } - return nullptr; + return {}; } void FuchsiaHandleChecker::checkPreCall(const CallEvent &Call, @@ -273,30 +311,31 @@ if (Arg >= FuncDecl->getNumParams()) break; const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg); - SymbolRef Handle = - getFuchsiaHandleSymbol(PVD->getType(), Call.getArgSVal(Arg), State); - if (!Handle) - continue; + SmallVector Handles = + getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State); // Handled in checkPostCall. if (hasFuchsiaAttr(PVD) || hasFuchsiaAttr(PVD)) continue; - const HandleState *HState = State->get(Handle); - if (!HState || HState->isEscaped()) - continue; + for (SymbolRef Handle : Handles) { + const HandleState *HState = State->get(Handle); + if (!HState || HState->isEscaped()) + continue; - if (hasFuchsiaAttr(PVD) || PVD->getType()->isIntegerType()) { - if (HState->isReleased()) { - reportUseAfterFree(Handle, Call.getArgSourceRange(Arg), C); - return; + if (hasFuchsiaAttr(PVD) || + PVD->getType()->isIntegerType()) { + if (HState->isReleased()) { + reportUseAfterFree(Handle, Call.getArgSourceRange(Arg), C); + return; + } + } + if (!hasFuchsiaAttr(PVD) && + PVD->getType()->isIntegerType()) { + // Working around integer by-value escapes. + State = State->set(Handle, HandleState::getEscaped()); } - } - if (!hasFuchsiaAttr(PVD) && - PVD->getType()->isIntegerType()) { - // Working around integer by-value escapes. - State = State->set(Handle, HandleState::getEscaped()); } } C.addTransition(State); @@ -339,63 +378,63 @@ 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 Handles = + getFuchsiaHandleSymbols(PVD->getType(), Call.getArgSVal(Arg), State); - const HandleState *HState = State->get(Handle); - if (HState && HState->isEscaped()) - continue; - if (hasFuchsiaAttr(PVD)) { - if (HState && HState->isReleased()) { - reportDoubleRelease(Handle, Call.getArgSourceRange(Arg), C); - return; - } else { + for (SymbolRef Handle : Handles) { + const HandleState *HState = State->get(Handle); + if (HState && HState->isEscaped()) + continue; + if (hasFuchsiaAttr(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(&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(Handle, HandleState::getReleased()); + } + } else if (hasFuchsiaAttr(PVD)) { Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string { auto *PathBR = static_cast(&BR); if (auto IsInteresting = PathBR->getInterestingnessKind(Handle)) { std::string SBuf; llvm::raw_string_ostream OS(SBuf); - OS << "Handle released through " << ParamDiagIdx + OS << "Handle allocated through " << ParamDiagIdx << llvm::getOrdinalSuffix(ParamDiagIdx) << " parameter"; return OS.str(); } else return ""; }); - State = State->set(Handle, HandleState::getReleased()); + State = State->set( + Handle, HandleState::getMaybeAllocated(ResultSymbol)); } - } else if (hasFuchsiaAttr(PVD)) { - Notes.push_back([Handle, ParamDiagIdx](BugReport &BR) -> std::string { - auto *PathBR = static_cast(&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( - 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 ""; - for (auto &Note : Notes) { - std::string Text = Note(BR); - if (!Text.empty()) - return Text; - } - return ""; - }); + if (&BR.getBugType() != &UseAfterReleaseBugType && + &BR.getBugType() != &LeakBugType && + &BR.getBugType() != &DoubleReleaseBugType) + return ""; + for (auto &Note : Notes) { + std::string Text = Note(BR); + if (!Text.empty()) + return Text; + } + return ""; + }); } C.addTransition(State, T); } @@ -481,13 +520,14 @@ if (Arg >= FuncDecl->getNumParams()) break; const ParmVarDecl *PVD = FuncDecl->getParamDecl(Arg); - SymbolRef Handle = - getFuchsiaHandleSymbol(PVD->getType(), Call->getArgSVal(Arg), State); - if (!Handle) - continue; - if (hasFuchsiaAttr(PVD) || - hasFuchsiaAttr(PVD)) - UnEscaped.insert(Handle); + SmallVector Handles = + getFuchsiaHandleSymbols(PVD->getType(), Call->getArgSVal(Arg), State); + for (SymbolRef Handle : Handles) { + if (hasFuchsiaAttr(PVD) || + hasFuchsiaAttr(PVD)) { + UnEscaped.insert(Handle); + } + } } } diff --git a/clang/test/Analysis/fuchsia_handle.cpp b/clang/test/Analysis/fuchsia_handle.cpp --- a/clang/test/Analysis/fuchsia_handle.cpp +++ b/clang/test/Analysis/fuchsia_handle.cpp @@ -9,9 +9,9 @@ #define ZX_HANDLE_INVALID 0 #if defined(__clang__) -#define ZX_HANDLE_ACQUIRE __attribute__((acquire_handle("Fuchsia"))) -#define ZX_HANDLE_RELEASE __attribute__((release_handle("Fuchsia"))) -#define ZX_HANDLE_USE __attribute__((use_handle("Fuchsia"))) +#define ZX_HANDLE_ACQUIRE __attribute__((acquire_handle("Fuchsia"))) +#define ZX_HANDLE_RELEASE __attribute__((release_handle("Fuchsia"))) +#define ZX_HANDLE_USE __attribute__((use_handle("Fuchsia"))) #else #define ZX_HANDLE_ACQUIRE #define ZX_HANDLE_RELEASE @@ -31,7 +31,7 @@ void escape1(zx_handle_t *in); void escape2(zx_handle_t in); -void (*escape3)(zx_handle_t) = escape2; +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); @@ -82,8 +82,8 @@ // expected-note-re@-3 {{Handle allocated through {{(2nd|3rd)}} parameter}} if (status == 0) { // expected-note {{Assuming 'status' is equal to 0}} // expected-note@-1 {{Taking true branch}} - return; // expected-warning {{Potential leak of handle}} - // expected-note@-1 {{Potential leak of handle}} + return; // expected-warning {{Potential leak of handle}} + // expected-note@-1 {{Potential leak of handle}} } __builtin_trap(); } @@ -138,7 +138,7 @@ return; zx_handle_close(sa); zx_handle_close(sb); -} +} void checkLeak01(int tag) { zx_handle_t sa, sb; @@ -158,7 +158,7 @@ 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}} +// expected-warning@-1 {{Potential leak of handle}} void checkReportLeakOnOnePath(int tag) { zx_handle_t sa, sb; @@ -166,26 +166,26 @@ 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: - use2(sa); - return; - case 2: - use2(sa); - return; - case 3: - use2(sa); - return; - case 4: - use2(sa); - return; - default: - use2(sa); - return; // expected-warning {{Potential leak of handle}} - // expected-note@-1 {{Potential leak of handle}} + switch (tag) { // expected-note {{Control jumps to the 'default' case at line}} + case 0: + use2(sa); + return; + case 1: + use2(sa); + return; + case 2: + use2(sa); + return; + case 3: + use2(sa); + return; + case 4: + use2(sa); + return; + default: + use2(sa); + return; // expected-warning {{Potential leak of handle}} + // expected-note@-1 {{Potential leak of handle}} } } @@ -193,7 +193,7 @@ zx_handle_t sa, sb; zx_channel_create(0, &sa, &sb); // expected-note@-1 {{Handle allocated through 2nd parameter}} - if (tag) // expected-note {{Assuming 'tag' is not equal to 0}} + if (tag) // expected-note {{Assuming 'tag' is not equal to 0}} 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}} @@ -211,12 +211,12 @@ if (tag) { // expected-note@-1 {{Assuming 'tag' is not equal to 0}} zx_handle_close(sa); // expected-note {{Handle released through 1st parameter}} - use1(&sa); // expected-warning {{Using a previously released handle}} + 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 through 1st parameter}} - use2(sb); // expected-warning {{Using a previously released handle}} + use2(sb); // expected-warning {{Using a previously released handle}} // expected-note@-1 {{Using a previously released handle}} } @@ -229,6 +229,92 @@ zx_handle_close(sc); } +struct HandleStruct { + zx_handle_t h; +}; + +void close_handle_struct(HandleStruct hs ZX_HANDLE_RELEASE); + +void use_handle_struct(HandleStruct hs ZX_HANDLE_USE); + +void checkHandleInStructureUseAfterFree() { + zx_handle_t sa, sb; + zx_channel_create(0, &sa, &sb); // expected-note {{Handle allocated through 3rd parameter}} + HandleStruct hs; + hs.h = sb; + use_handle_struct(hs); + close_handle_struct(hs); // expected-note {{Handle released through 1st parameter}} + zx_handle_close(sa); + + use2(sb); // expected-warning {{Using a previously released handle}} + // expected-note@-1 {{Using a previously released handle}} +} + +void checkHandleInStructureUseAfterFree2() { + zx_handle_t sa, sb; + zx_channel_create(0, &sa, &sb); // expected-note {{Handle allocated through 3rd parameter}} + HandleStruct hs; + hs.h = sb; + use_handle_struct(hs); + zx_handle_close(sb); // expected-note {{Handle released through 1st parameter}} + zx_handle_close(sa); + + use_handle_struct(hs); // expected-warning {{Using a previously released handle}} + // expected-note@-1 {{Using a previously released handle}} +} + +void checkHandleInStructureLeak() { + zx_handle_t sa, sb; + zx_channel_create(0, &sa, &sb); // expected-note {{Handle allocated through 3rd parameter}} + HandleStruct hs; + hs.h = sb; + zx_handle_close(sa); // expected-warning {{Potential leak of handle}} + // expected-note@-1 {{Potential leak of handle}} +} + +struct HandlePtrStruct { + zx_handle_t *h; +}; + +void close_handle_struct(HandlePtrStruct hs ZX_HANDLE_RELEASE); + +void use_handle_struct(HandlePtrStruct hs ZX_HANDLE_USE); + +void checkHandlePtrInStructureUseAfterFree() { + zx_handle_t sa, sb; + zx_channel_create(0, &sa, &sb); + HandlePtrStruct hs; + hs.h = &sb; + use_handle_struct(hs); + close_handle_struct(hs); // expected-note {{Handle released through 1st parameter}} + zx_handle_close(sa); + + use2(sb); // expected-warning {{Using a previously released handle}} + // expected-note@-1 {{Using a previously released handle}} +} + +void checkHandlePtrInStructureUseAfterFree2() { + zx_handle_t sa, sb; + zx_channel_create(0, &sa, &sb); + HandlePtrStruct hs; + hs.h = &sb; + use_handle_struct(hs); + zx_handle_close(sb); // expected-note {{Handle released through 1st parameter}} + zx_handle_close(sa); + + use_handle_struct(hs); // expected-warning {{Using a previously released handle}} + // expected-note@-1 {{Using a previously released handle}} +} + +void checkHandlePtrInStructureLeak() { + zx_handle_t sa, sb; + zx_channel_create(0, &sa, &sb); // expected-note {{Handle allocated through 3rd parameter}} + HandlePtrStruct hs; + hs.h = &sb; + zx_handle_close(sa); // expected-warning {{Potential leak of handle}} + // expected-note@-1 {{Potential leak of handle}} +} + // RAII template @@ -239,6 +325,7 @@ zx_handle_close(handle); } T *get_handle_address() { return &handle; } + private: T handle; }; @@ -259,6 +346,7 @@ zx_handle_close(handle); } T *get_handle_address() { return &handle; } + private: T handle; }; @@ -270,7 +358,7 @@ return; zx_handle_close(sb); } - + // Various escaping scenarios zx_handle_t *get_handle_address();