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 @@ -179,9 +179,15 @@ /// Returns the symbolic value that represents the "has_value" property of the /// optional value `OptionalVal`. Returns null if `OptionalVal` is null. -BoolValue *getHasValue(Value *OptionalVal) { - if (OptionalVal) { - return cast(OptionalVal->getProperty("has_value")); +BoolValue *getHasValue(Environment &Env, Value *OptionalVal) { + if (OptionalVal != nullptr) { + auto *HasValueVal = + cast_or_null(OptionalVal->getProperty("has_value")); + if (HasValueVal == nullptr) { + HasValueVal = &Env.makeAtomicBoolValue(); + OptionalVal->setProperty("has_value", *HasValueVal); + } + return HasValueVal; } return nullptr; } @@ -218,6 +224,50 @@ .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 *maybeInitializeOptionalValueMember(QualType Q, + Value &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. Once + // the property is set, it will be shared by all environments that access the + // `Value` representing the optional (here, `OptionalVal`). + 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. + // FIXME: This situation means that the optional contents are not shared + // between branches and the like. Practically, this lack of sharing + // reduces the precision of the model when the contents are relevant to + // the check, like another optional or a boolean that influences control + // flow. + auto *ValueVal = Env.createValue(ValueLoc.getType()); + if (ValueVal == nullptr) + return nullptr; + Env.setValue(ValueLoc, *ValueVal); + } + return &ValueLoc; + } + + auto Ty = stripReference(Q); + auto *ValueVal = Env.createValue(Ty); + if (ValueVal == nullptr) + return nullptr; + 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) { @@ -233,11 +283,16 @@ LatticeTransferState &State) { if (auto *OptionalVal = 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 = maybeInitializeOptionalValueMember( + UnwrapExpr->getType(), *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. @@ -258,12 +313,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); @@ -284,12 +336,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)); @@ -376,8 +427,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 @@ -2165,7 +2165,6 @@ } )", UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7"))); - ExpectLatticeChecksFor( R"( #include "unchecked_optional_access_test.h" @@ -2231,8 +2230,95 @@ UnorderedElementsAre(Pair("check", "unsafe: input.cc:8:7"))); } +TEST_P(UncheckedOptionalAccessTest, OptionalValueOptional) { + // Basic test that nested values are populated. We nest an optional because + // its easy to use in a test, but the type of the nested value shouldn't + // matter. + 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"))); + + // Mutation is supported for nested values. + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + using Foo = $ns::$optional; + + void target($ns::$optional foo) { + if (foo && *foo) { + foo->reset(); + foo->value(); + /*[[reset]]*/ + } + } + )", + UnorderedElementsAre(Pair("reset", "unsafe: input.cc:9:9"))); +} + +// Tests that structs can be nested. We use an optional field because its easy +// to use in a test, but the type of the field shouldn't matter. +TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) { + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo { + $ns::$optional opt; + }; + + void target($ns::$optional foo) { + if (foo && foo->opt) { + foo->opt.value(); + /*[[access]]*/ + } + } + )", + UnorderedElementsAre(Pair("access", "safe"))); +} + +TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) { + // 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