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 @@ -113,6 +113,43 @@ hasArgument(1, hasOptionalType())); } +constexpr llvm::StringLiteral ValueOrCallID = "ValueOrCall"; + +auto isValueOrStringEmptyCall() { + // `opt.value_or("").empty()` + return cxxMemberCallExpr( + callee(cxxMethodDecl(hasName("empty"))), + onImplicitObjectArgument(ignoringImplicit( + cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))), + callee(cxxMethodDecl(hasName("value_or"), + ofClass(optionalClass()))), + hasArgument(0, stringLiteral(hasSize(0)))) + .bind(ValueOrCallID)))); +} + +auto isValueOrCondition() { + auto ComparesToSame = [](ast_matchers::internal::Matcher Arg) { + return hasOperands( + ignoringImplicit( + cxxMemberCallExpr(on(expr(unless(cxxThisExpr()))), + callee(cxxMethodDecl(hasName("value_or"), + ofClass(optionalClass()))), + hasArgument(0, Arg)) + .bind(ValueOrCallID)), + ignoringImplicit(Arg)); + }; + + // `opt.value_or(X) != X`, for X is `nullptr`, `""`, or `0`. Ideally, we'd + // support this pattern for any expression, but the AST does not have a + // generic expression comparison facility, so we specialize to common cases + // seen in practice. FIXME: define a matcher that compares values across + // nodes, which would let us generalize this to any `X`. + return binaryOperation(hasOperatorName("!="), + anyOf(ComparesToSame(cxxNullPtrLiteralExpr()), + ComparesToSame(stringLiteral(hasSize(0))), + ComparesToSame(integerLiteral(equals(0))))); +} + /// Creates a symbolic value for an `optional` value using `HasValueVal` as the /// symbolic value of its "has_value" property. StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) { @@ -212,6 +249,69 @@ } } +/// `ModelPred` builds a logical formula relating the predicate in +/// `ValueOrPredExpr` to the optional's `has_value` property. +void transferValueOrImpl(const clang::Expr *ValueOrPredExpr, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State, + BoolValue &(*ModelPred)(Environment &Env, + BoolValue &ExprVal, + BoolValue &HasValueVal)) { + auto &Env = State.Env; + + const auto *ObjectArgumentExpr = + Result.Nodes.getNodeAs(ValueOrCallID) + ->getImplicitObjectArgument(); + + auto *OptionalVal = cast_or_null( + Env.getValue(*ObjectArgumentExpr, SkipPast::ReferenceThenPointer)); + if (OptionalVal == nullptr) + return; + auto *HasValueVal = getHasValue(OptionalVal); + assert(HasValueVal != nullptr); + + auto *ExprValue = clang::cast_or_null( + State.Env.getValue(*ValueOrPredExpr, SkipPast::None)); + if (ExprValue == nullptr) { + auto &ExprLoc = State.Env.createStorageLocation(*ValueOrPredExpr); + ExprValue = &State.Env.makeAtomicBoolValue(); + State.Env.setValue(ExprLoc, *ExprValue); + State.Env.setStorageLocation(*ValueOrPredExpr, ExprLoc); + } + + Env.addToFlowCondition(ModelPred(Env, *ExprValue, *HasValueVal)); +} + +void transferValueOrStringEmptyCall(const clang::Expr *ComparisonExpr, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + return transferValueOrImpl(ComparisonExpr, Result, State, + [](Environment &Env, BoolValue &ExprVal, + BoolValue &HasValueVal) -> BoolValue & { + // If the result is *not* empty, then we know the + // optional must have been holding a value. If + // `ExprVal` is true, though, we don't learn + // anything definite about `has_value`, so we + // don't add any corresponding implications to + // the flow condition. + return Env.makeImplication(Env.makeNot(ExprVal), + HasValueVal); + }); +} + +void transferValueOrNotEqX(const Expr *ComparisonExpr, + const MatchFinder::MatchResult &Result, + LatticeTransferState &State) { + transferValueOrImpl(ComparisonExpr, Result, State, + [](Environment &Env, BoolValue &ExprVal, + BoolValue &HasValueVal) -> BoolValue & { + // We know that if `(opt.value_or(X) != X)` then + // `opt.hasValue()`, even without knowing further + // details about the contents of `opt`. + return Env.makeImplication(ExprVal, HasValueVal); + }); +} + void assignOptionalValue(const Expr &E, LatticeTransferState &State, BoolValue &HasValueVal) { if (auto *OptionalLoc = @@ -418,6 +518,12 @@ // std::swap .CaseOf(isStdSwapCall(), transferStdSwapCall) + // opt.value_or("").empty() + .CaseOf(isValueOrStringEmptyCall(), transferValueOrStringEmptyCall) + + // opt.value_or(X) != X + .CaseOf(isValueOrCondition(), transferValueOrNotEqX) + .Build(); } 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 @@ -1710,6 +1710,119 @@ UnorderedElementsAre(Pair("check", "safe"))); } +TEST_P(UncheckedOptionalAccessTest, ValueOrComparison) { + // Pointers. + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt; + if (opt.value_or(nullptr) != nullptr) { + opt.value(); + /*[[check-ptrs-1]]*/ + } else { + opt.value(); + /*[[check-ptrs-2]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-ptrs-1", "safe"), + Pair("check-ptrs-2", "unsafe: input.cc:10:9"))); + + // Integers. + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt; + if (opt.value_or(0) != 0) { + opt.value(); + /*[[check-ints-1]]*/ + } else { + opt.value(); + /*[[check-ints-2]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-ints-1", "safe"), + Pair("check-ints-2", "unsafe: input.cc:10:9"))); + + // Strings. + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + namespace std { + struct string { + bool empty(); + }; + } + + void target() { + $ns::$optional opt; + if (!opt.value_or("").empty()) { + opt.value(); + /*[[check-strings-1]]*/ + } else { + opt.value(); + /*[[check-strings-2]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-strings-1", "safe"), + Pair("check-strings-2", "unsafe: input.cc:16:9"))); + + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + namespace std { + struct string { + string(const char*); + ~string(); + bool empty(); + }; + bool operator!=(const string &LHS, const char *RHS); + } + + void target() { + $ns::$optional opt; + if (opt.value_or("") != "") { + opt.value(); + /*[[check-strings-neq-1]]*/ + } else { + opt.value(); + /*[[check-strings-neq-2]]*/ + } + } + )code", + UnorderedElementsAre( + Pair("check-strings-neq-1", "safe"), + Pair("check-strings-neq-2", "unsafe: input.cc:19:9"))); + + // Pointer-to-optional. + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt; + auto *opt_p = &opt; + if (opt_p->value_or(0) != 0) { + opt_p->value(); + /*[[check-pto-1]]*/ + } else { + opt_p->value(); + /*[[check-pto-2]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-pto-1", "safe"), + Pair("check-pto-2", "unsafe: input.cc:11:9"))); +} + TEST_P(UncheckedOptionalAccessTest, Emplace) { ExpectLatticeChecksFor(R"( #include "unchecked_optional_access_test.h" @@ -2008,5 +2121,4 @@ // - constructors (copy, move) // - assignment operators (default, copy, move) // - invalidation (passing optional by non-const reference/pointer) -// - `value_or(nullptr) != nullptr`, `value_or(0) != 0`, `value_or("").empty()` // - nested `optional` values