Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp +++ 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,72 @@ 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 +313,30 @@ 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,46 +379,46 @@ 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; @@ -481,13 +521,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); + } + } } } Index: clang/test/Analysis/fuchsia_handle.cpp =================================================================== --- clang/test/Analysis/fuchsia_handle.cpp +++ clang/test/Analysis/fuchsia_handle.cpp @@ -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); @@ -138,7 +138,7 @@ return; zx_handle_close(sa); zx_handle_close(sb); -} +} void checkLeak01(int tag) { zx_handle_t sa, sb; @@ -166,7 +166,7 @@ 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}} + switch(tag) { // expected-note {{Control jumps to the 'default' case at line}} case 0: use2(sa); return; @@ -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 @@ -270,7 +356,7 @@ return; zx_handle_close(sb); } - + // Various escaping scenarios zx_handle_t *get_handle_address();