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 @@ -167,10 +167,17 @@ } /// Returns the symbolic value that represents the "has_value" property of the -/// optional value `Val`. Returns null if `Val` is null. -BoolValue *getHasValue(Value *Val) { +/// optional value `Val`, creating a fresh one if none is set. Returns null if +/// `Val` is null. +BoolValue *getHasValue(Environment &Env, Value *Val) { if (auto *OptionalVal = cast_or_null(Val)) { - return cast(OptionalVal->getProperty("has_value")); + auto *HasValueVal = + cast_or_null(OptionalVal->getProperty("has_value")); + if (HasValueVal == nullptr) { + HasValueVal = &Env.makeAtomicBoolValue(); + OptionalVal->setProperty("has_value", *HasValueVal); + } + return HasValueVal; } return nullptr; } @@ -207,6 +214,47 @@ .getDesugaredType(ASTCtx)); } +/// Tries to initialize the `optional`'s value (that is, contents), and return +/// its location. Returns nullptr if the value can't be represented. +StorageLocation *initializeUnwrappedValue(const Expr &Expr, + StructValue &OptionalVal, + Environment &Env) { + // The "value" property represents a synthetic field. As such, it needs + // `StorageLocation`, like normal fields (and other variables). So, we model + // it with a `ReferenceValue`, since that includes a storage location. + if (auto *ValueProp = OptionalVal.getProperty("value")) { + auto *ValueRef = clang::cast(ValueProp); + auto &ValueLoc = ValueRef->getPointeeLoc(); + if (Env.getValue(ValueLoc) == nullptr) { + // The property was previously set, but the value has been lost. This can + // happen, for example, because of an environment merge (where the two + // environments mapped the property to different values, which resulted in + // them both being discarded), or when two blocks in the CFG, with neither + // a dominator of the other, visit the same optional value, or even when a + // block is revisited during testing to collect per-statement state. + auto *ValueVal = Env.createValue(stripReference(Expr.getType())); + if (ValueVal == nullptr) + return nullptr; + Env.setValue(ValueLoc, *ValueVal); + } + return &ValueLoc; + } + + auto Ty = stripReference(Expr.getType()); + auto *ValueVal = Env.createValue(Ty); + if (ValueVal == nullptr) + return nullptr; + // FIXME: the `StorageLocation` of the "value" property should be set + // eagerly when the optional is constructed, so that it can be shared by all + // environments that use the optional's value. (The creation of the + // underlying value can still be populated lazily). + auto &ValueLoc = Env.createStorageLocation(Ty); + Env.setValue(ValueLoc, *ValueVal); + auto ValueRef = std::make_unique(ValueLoc); + OptionalVal.setProperty("value", Env.takeOwnership(std::move(ValueRef))); + return &ValueLoc; +} + void initializeOptionalReference(const Expr *OptionalExpr, const MatchFinder::MatchResult &, LatticeTransferState &State) { @@ -222,11 +270,16 @@ LatticeTransferState &State) { if (auto *OptionalVal = cast_or_null( State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer))) { - auto *HasValueVal = getHasValue(OptionalVal); - assert(HasValueVal != nullptr); - - if (State.Env.flowConditionImplies(*HasValueVal)) - return; + if (State.Env.getStorageLocation(*UnwrapExpr, SkipPast::None) == nullptr) + if (auto *Loc = + initializeUnwrappedValue(*UnwrapExpr, *OptionalVal, State.Env)) + State.Env.setStorageLocation(*UnwrapExpr, *Loc); + + auto *Prop = OptionalVal->getProperty("has_value"); + if (auto *HasValueVal = cast_or_null(Prop)) { + if (State.Env.flowConditionImplies(*HasValueVal)) + return; + } } // Record that this unwrap is *not* provably safe. @@ -245,12 +298,9 @@ void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr, const MatchFinder::MatchResult &, LatticeTransferState &State) { - if (auto *OptionalVal = cast_or_null( - State.Env.getValue(*CallExpr->getImplicitObjectArgument(), - SkipPast::ReferenceThenPointer))) { - auto *HasValueVal = getHasValue(OptionalVal); - assert(HasValueVal != nullptr); - + if (auto *HasValueVal = getHasValue( + State.Env, State.Env.getValue(*CallExpr->getImplicitObjectArgument(), + SkipPast::ReferenceThenPointer))) { auto &CallExprLoc = State.Env.createStorageLocation(*CallExpr); State.Env.setValue(CallExprLoc, *HasValueVal); State.Env.setStorageLocation(*CallExpr, CallExprLoc); @@ -271,12 +321,11 @@ Result.Nodes.getNodeAs(ValueOrCallID) ->getImplicitObjectArgument(); - auto *OptionalVal = cast_or_null( - Env.getValue(*ObjectArgumentExpr, SkipPast::ReferenceThenPointer)); - if (OptionalVal == nullptr) + auto *HasValueVal = getHasValue( + State.Env, + State.Env.getValue(*ObjectArgumentExpr, SkipPast::ReferenceThenPointer)); + if (HasValueVal == nullptr) return; - auto *HasValueVal = getHasValue(OptionalVal); - assert(HasValueVal != nullptr); auto *ExprValue = cast_or_null( State.Env.getValue(*ValueOrPredExpr, SkipPast::None)); @@ -351,8 +400,9 @@ // This is a constructor/assignment call for `optional` with argument of // type `optional` such that `T` is constructible from `U`. - if (BoolValue *Val = getHasValue(State.Env.getValue(E, SkipPast::Reference))) - return *Val; + if (auto *HasValueVal = + getHasValue(State.Env, State.Env.getValue(E, SkipPast::Reference))) + return *HasValueVal; return State.Env.makeAtomicBoolValue(); } 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 @@ -2150,8 +2150,195 @@ UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe"))); } +TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptional) { + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + using Foo = $ns::$optional; + + void target($ns::$optional foo) { + if (foo && *foo) { + foo->value(); + /*[[access]]*/ + } + } + )", + UnorderedElementsAre(Pair("access", "safe"))); +} + +TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptionalStruct) { + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional opt; + }; + + void target(const $ns::$optional& foo) { + if (foo && foo->opt) { + foo->opt.value(); + /*[[access]]*/ + } + } + )", + UnorderedElementsAre(Pair("access", "safe"))); + + // `reset` changes the state. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional opt; + }; + + void target($ns::$optional& foo) { + if (foo && foo->opt) { + foo->opt.reset(); + foo->opt.value(); + /*[[reset]]*/ + } + } + )", + UnorderedElementsAre(Pair("reset", "unsafe: input.cc:11:9"))); + + // `has_value` and `operator*`. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional opt; + }; + + void target($ns::$optional& foo) { + if (foo && foo->opt.has_value()) { + *foo->opt; + /*[[other-ops]]*/ + } + } + )", + UnorderedElementsAre(Pair("other-ops", "safe"))); + + // `operator->`. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional opt; + }; + + void target($ns::$optional& foo) { + if (foo && foo->opt.has_value()) { + foo->opt->empty(); + /*[[arrow]]*/ + } + } + )", + UnorderedElementsAre(Pair("arrow", "safe"))); + + // Accessing the nested optional in the wrong branch. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional opt; + }; + + void target($ns::$optional& foo) { + if (!foo.has_value()) return; + if (!foo->opt.has_value()) { + foo->opt.value(); + /*[[not-set]]*/ + } + } + )", + UnorderedElementsAre(Pair("not-set", "unsafe: input.cc:11:9"))); +} + +TEST_P(UncheckedOptionalAccessTest, NestedOptionalInOptionalStructField) { + // Non-standard assignment. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional opt; + }; + + struct Bar { + $ns::$optional member; + }; + + Bar createBar(); + + void target() { + Bar bar = createBar(); + bar.member->opt = "a"; + /*[[ns-assign]]*/ + } + )", + UnorderedElementsAre(Pair("ns-assign", "unsafe: input.cc:16:7"))); + + // Resetting the nested optional. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Member { + $ns::$optional opt; + }; + + struct Foo { + $ns::$optional member; + }; + + Foo createFoo(); + + void target() { + Foo foo = createFoo(); + foo.member->opt.reset(); + /*[[nested-reset]]*/ + } + )", + UnorderedElementsAre(Pair("nested-reset", "unsafe: input.cc:16:7"))); +} + +TEST_P(UncheckedOptionalAccessTest, NestedOptionalInitialization) { + // FIXME: Fix when to initialize `value`. All unwrapping should be safe in + // this example, but `value` initialization is done multiple times during the + // fixpoint iterations and joining the environment won't correctly merge them. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + using Foo = $ns::$optional; + + void target($ns::$optional foo, bool b) { + if (!foo.has_value()) return; + if (b) { + if (!foo->has_value()) return; + // We have created `foo.value()`. + foo->value(); + } else { + if (!foo->has_value()) return; + // We have created `foo.value()` again, in a different environment. + foo->value(); + } + // Now we merge the two values. UncheckedOptionalAccessModel::merge() will + // throw away the "value" property. + foo->value(); + /*[[merge]]*/ + } + )", + UnorderedElementsAre(Pair("merge", "unsafe: input.cc:19:7"))); +} + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) // - invalidation (passing optional by non-const reference/pointer) -// - nested `optional` values