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 @@ -121,6 +121,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 isValueOrNotEqX() { + 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) { @@ -220,6 +257,70 @@ } } +/// `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); + + assert(ValueOrPredExpr); + auto *ExprValue = 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 = @@ -439,6 +540,12 @@ // std::swap .CaseOf(isStdSwapCall(), transferStdSwapCall) + // opt.value_or("").empty() + .CaseOf(isValueOrStringEmptyCall(), transferValueOrStringEmptyCall) + + // opt.value_or(X) != X + .CaseOf(isValueOrNotEqX(), 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 @@ -496,6 +496,24 @@ #endif // ABSL_TYPE_TRAITS_H )"; +static constexpr char StdStringHeader[] = R"( +#ifndef STRING_H +#define STRING_H + +namespace std { + +struct string { + string(const char*); + ~string(); + bool empty(); +}; +bool operator!=(const string &LHS, const char *RHS); + +} // namespace std + +#endif // STRING_H +)"; + static constexpr char StdUtilityHeader[] = R"( #ifndef UTILITY_H #define UTILITY_H @@ -1198,6 +1216,7 @@ std::vector> Headers; Headers.emplace_back("cstddef.h", CSDtdDefHeader); Headers.emplace_back("std_initializer_list.h", StdInitializerListHeader); + Headers.emplace_back("std_string.h", StdStringHeader); Headers.emplace_back("std_type_traits.h", StdTypeTraitsHeader); Headers.emplace_back("std_utility.h", StdUtilityHeader); Headers.emplace_back("std_optional.h", StdOptionalHeader); @@ -1209,6 +1228,7 @@ #include "base_optional.h" #include "std_initializer_list.h" #include "std_optional.h" + #include "std_string.h" #include "std_utility.h" template @@ -1712,6 +1732,102 @@ 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:9: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:9:9"))); + + // Strings. + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + 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:9:9"))); + + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + 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:9:9"))); + + // Pointer-to-optional. + // + // FIXME: make `opt` a parameter directly, once we ensure that all `optional` + // values have a `has_value` property. + ExpectLatticeChecksFor( + R"code( + #include "unchecked_optional_access_test.h" + + void target($ns::$optional p) { + $ns::$optional *opt = &p; + if (opt->value_or(0) != 0) { + opt->value(); + /*[[check-pto-1]]*/ + } else { + opt->value(); + /*[[check-pto-2]]*/ + } + } + )code", + UnorderedElementsAre(Pair("check-pto-1", "safe"), + Pair("check-pto-2", "unsafe: input.cc:10:9"))); +} + TEST_P(UncheckedOptionalAccessTest, Emplace) { ExpectLatticeChecksFor(R"( #include "unchecked_optional_access_test.h" @@ -2038,5 +2154,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