diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -24,6 +24,18 @@ namespace clang { namespace dataflow { +// FIXME: Explore using an allowlist-approach, where constructs supported by the +// analysis are always enabled and additional constructs are enabled through the +// `Options`. +struct UncheckedOptionalAccessModelOptions { + /// Ignore optionals reachable through overloaded `operator*` or `operator->` + /// (other than those of the optional type itself). The analysis does not + /// equate the results of such calls, so it can't identify when their results + /// are used safely (across calls), resulting in false positives in all such + /// cases. Note: this option does not cover access through `operator[]`. + bool IgnoreSmartPointerDereference = false; +}; + /// Dataflow analysis that discovers unsafe accesses of optional values and /// adds the respective source locations to the lattice. /// @@ -34,7 +46,8 @@ : public DataflowAnalysis { public: - explicit UncheckedOptionalAccessModel(ASTContext &AstContext); + UncheckedOptionalAccessModel( + ASTContext &AstContext, UncheckedOptionalAccessModelOptions Options = {}); static SourceLocationsLattice initialElement() { return SourceLocationsLattice(); diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -45,15 +45,23 @@ auto hasOptionalType() { return hasType(optionalClass()); } -auto isOptionalMemberCallWithName(llvm::StringRef MemberName) { +auto isOptionalMemberCallWithName( + llvm::StringRef MemberName, + llvm::Optional Ignorable = llvm::None) { + auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr())) + : cxxThisExpr()); return cxxMemberCallExpr( - on(expr(unless(cxxThisExpr()))), + on(expr(Exception)), callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass())))); } -auto isOptionalOperatorCallWithName(llvm::StringRef OperatorName) { - return cxxOperatorCallExpr(hasOverloadedOperatorName(OperatorName), - callee(cxxMethodDecl(ofClass(optionalClass())))); +auto isOptionalOperatorCallWithName( + llvm::StringRef operator_name, + llvm::Optional Ignorable = llvm::None) { + return cxxOperatorCallExpr( + hasOverloadedOperatorName(operator_name), + callee(cxxMethodDecl(ofClass(optionalClass()))), + Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr()); } auto isMakeOptionalCall() { @@ -333,10 +341,22 @@ transferSwap(*OptionalLoc1, *OptionalLoc2, State); } -auto buildTransferMatchSwitch() { +llvm::Optional +ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) { + if (Options.IgnoreSmartPointerDereference) + return memberExpr(hasObjectExpression(ignoringParenImpCasts( + cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("->"), + hasOverloadedOperatorName("*")), + unless(hasArgument(0, expr(hasOptionalType()))))))); + return llvm::None; +} + +auto buildTransferMatchSwitch( + const UncheckedOptionalAccessModelOptions &Options) { // FIXME: Evaluate the efficiency of matchers. If using matchers results in a // lot of duplicated work (e.g. string comparisons), consider providing APIs // that avoid it through memoization. + auto IgnorableOptional = ignorableOptional(Options); return MatchSwitchBuilder() // Attach a symbolic "has_value" state to optional values that we see for // the first time. @@ -371,19 +391,20 @@ // optional::value .CaseOf( - isOptionalMemberCallWithName("value"), + isOptionalMemberCallWithName("value", IgnorableOptional), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { transferUnwrapCall(E, E->getImplicitObjectArgument(), State); }) // optional::operator*, optional::operator-> - .CaseOf(expr(anyOf(isOptionalOperatorCallWithName("*"), - isOptionalOperatorCallWithName("->"))), - [](const CallExpr *E, const MatchFinder::MatchResult &, - LatticeTransferState &State) { - transferUnwrapCall(E, E->getArg(0), State); - }) + .CaseOf( + expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional), + isOptionalOperatorCallWithName("->", IgnorableOptional))), + [](const CallExpr *E, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferUnwrapCall(E, E->getArg(0), State); + }) // optional::has_value .CaseOf(isOptionalMemberCallWithName("has_value"), @@ -423,10 +444,11 @@ } // namespace -UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx) +UncheckedOptionalAccessModel::UncheckedOptionalAccessModel( + ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options) : DataflowAnalysis( Ctx), - TransferMatchSwitch(buildTransferMatchSwitch()) {} + TransferMatchSwitch(buildTransferMatchSwitch(Options)) {} void UncheckedOptionalAccessModel::transfer(const Stmt *S, SourceLocationsLattice &L, diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -1219,7 +1219,9 @@ llvm::Error Error = checkDataflow( SourceCode, FuncMatcher, [](ASTContext &Ctx, Environment &) { - return UncheckedOptionalAccessModel(Ctx); + return UncheckedOptionalAccessModel( + Ctx, UncheckedOptionalAccessModelOptions{ + /*IgnoreSmartPointerDereference=*/true}); }, [&MatchesLatticeChecks]( llvm::ArrayRef + struct smart_ptr { + T& operator*() &; + T* operator->(); + }; + + struct Foo { + $ns::$optional opt; + }; + + void target() { + smart_ptr foo; + *foo->opt; + /*[[check-1]]*/ + *(*foo).opt; + /*[[check-2]]*/ + } + )", + UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe"))); +} + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move)