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 @@ -1,3 +1,16 @@ +//===-- UncheckedOptionalAccessModel.h --------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file defines a dataflow analysis that detects unsafe uses of optional +// values. +// +//===----------------------------------------------------------------------===// + #ifndef CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H #define CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -345,7 +345,8 @@ StorageLocation *Environment::getStorageLocation(const Expr &E, SkipPast SP) const { - auto It = ExprToLoc.find(&E); + // FIXME: Add a test with parens. + auto It = ExprToLoc.find(E.IgnoreParens()); return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP); } 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 @@ -1,3 +1,16 @@ +//===-- UncheckedOptionalAccessModel.cpp ------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file defines a dataflow analysis that detects unsafe uses of optional +// values. +// +//===----------------------------------------------------------------------===// + #include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h" #include "clang/AST/ASTContext.h" #include "clang/AST/Expr.h" @@ -78,6 +91,22 @@ argumentCountIs(1), hasArgument(0, unless(hasNulloptType()))); } +auto isOptionalValueOrConversionAssignment() { + return cxxOperatorCallExpr( + hasOverloadedOperatorName("="), + callee(cxxMethodDecl(ofClass(optionalClass()))), + unless(hasDeclaration(cxxMethodDecl( + anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator())))), + argumentCountIs(2), hasArgument(1, unless(hasNulloptType()))); +} + +auto isOptionalNulloptAssignment() { + return cxxOperatorCallExpr(hasOverloadedOperatorName("="), + callee(cxxMethodDecl(ofClass(optionalClass()))), + argumentCountIs(2), + hasArgument(1, hasNulloptType())); +} + /// 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) { @@ -186,34 +215,78 @@ } } +/// Returns a symbolic value for the "has_value" property of an `optional` +/// value that is constructed/assigned from a value of type `U` or `optional` +/// where `T` is constructible from `U`. +BoolValue & +getValueOrConversionHasValue(const FunctionDecl &F, const Expr &E, + const MatchFinder::MatchResult &MatchRes, + LatticeTransferState &State) { + assert(F.getTemplateSpecializationArgs()->size() > 0); + + const int TemplateParamOptionalWrappersCount = countOptionalWrappers( + *MatchRes.Context, + stripReference(F.getTemplateSpecializationArgs()->get(0).getAsType())); + const int ArgTypeOptionalWrappersCount = + countOptionalWrappers(*MatchRes.Context, stripReference(E.getType())); + + // Check if this is a constructor/assignment call for `optional` with + // argument of type `U` such that `T` is constructible from `U`. + if (TemplateParamOptionalWrappersCount == ArgTypeOptionalWrappersCount) + return State.Env.getBoolLiteralValue(true); + + // 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; + return State.Env.makeAtomicBoolValue(); +} + void transferValueOrConversionConstructor( const CXXConstructExpr *E, const MatchFinder::MatchResult &MatchRes, LatticeTransferState &State) { - assert(E->getConstructor()->getTemplateSpecializationArgs()->size() > 0); assert(E->getNumArgs() > 0); - const int TemplateParamOptionalWrappersCount = countOptionalWrappers( - *MatchRes.Context, stripReference(E->getConstructor() - ->getTemplateSpecializationArgs() - ->get(0) - .getAsType())); - const int ArgTypeOptionalWrappersCount = countOptionalWrappers( - *MatchRes.Context, stripReference(E->getArg(0)->getType())); - auto *HasValueVal = - (TemplateParamOptionalWrappersCount == ArgTypeOptionalWrappersCount) - // This is a constructor call for optional with argument of type U - // such that T is constructible from U. - ? &State.Env.getBoolLiteralValue(true) - // This is a constructor call for optional with argument of type - // optional such that T is constructible from U. - : getHasValue(State.Env.getValue(*E->getArg(0), SkipPast::Reference)); - if (HasValueVal == nullptr) - HasValueVal = &State.Env.makeAtomicBoolValue(); - - assignOptionalValue(*E, State, *HasValueVal); + assignOptionalValue(*E, State, + getValueOrConversionHasValue(*E->getConstructor(), + *E->getArg(0), MatchRes, + State)); +} + +void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal, + LatticeTransferState &State) { + assert(E->getNumArgs() > 0); + + auto *OptionalLoc = + State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference); + assert(OptionalLoc != nullptr); + + State.Env.setValue(*OptionalLoc, createOptionalValue(State.Env, HasValueVal)); + + // Assign a storage location for the whole expression. + State.Env.setStorageLocation(*E, *OptionalLoc); +} + +void transferValueOrConversionAssignment( + const CXXOperatorCallExpr *E, const MatchFinder::MatchResult &MatchRes, + LatticeTransferState &State) { + assert(E->getNumArgs() > 1); + transferAssignment(E, + getValueOrConversionHasValue( + *E->getDirectCallee(), *E->getArg(1), MatchRes, State), + State); +} + +void transferNulloptAssignment(const CXXOperatorCallExpr *E, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + transferAssignment(E, State.Env.getBoolLiteralValue(false), State); } auto buildTransferMatchSwitch() { + // 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. return MatchSwitchBuilder() // Attach a symbolic "has_value" state to optional values that we see for // the first time. @@ -223,7 +296,7 @@ // make_optional .CaseOf(isMakeOptionalCall(), transferMakeOptionalCall) - // constructors: + // optional::optional .CaseOf( isOptionalInPlaceConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, @@ -240,6 +313,12 @@ .CaseOf(isOptionalValueOrConversionConstructor(), transferValueOrConversionConstructor) + // optional::operator= + .CaseOf(isOptionalValueOrConversionAssignment(), + transferValueOrConversionAssignment) + .CaseOf(isOptionalNulloptAssignment(), + transferNulloptAssignment) + // optional::value .CaseOf( isOptionalMemberCallWithName("value"), diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -382,7 +382,12 @@ if (Val == nullptr) return; + // Assign a value to the storage location of the object. Env.setValue(*ObjectLoc, *Val); + + // FIXME: Add a test for the value of the whole expression. + // Assign a storage location for the whole expression. + Env.setStorageLocation(*S, *ObjectLoc); } } 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 @@ -30,13 +30,28 @@ using ::testing::UnorderedElementsAre; // FIXME: Move header definitions in separate file(s). +static constexpr char CSDtdDefHeader[] = R"( +#ifndef CSTDDEF_H +#define CSTDDEF_H + +namespace std { + +typedef decltype(sizeof(char)) size_t; + +using nullptr_t = decltype(nullptr); + +} // namespace std + +#endif // CSTDDEF_H +)"; + static constexpr char StdTypeTraitsHeader[] = R"( #ifndef STD_TYPE_TRAITS_H #define STD_TYPE_TRAITS_H -namespace std { +#include "cstddef.h" -typedef decltype(sizeof(char)) size_t; +namespace std { template struct integral_constant { @@ -287,6 +302,9 @@ template using _And = decltype(__and_helper<_Pred...>(0)); +template +struct _Not : _BoolConstant {}; + struct __check_tuple_constructor_fail { static constexpr bool __enable_explicit_default() { return false; } static constexpr bool __enable_implicit_default() { return false; } @@ -300,6 +318,150 @@ } }; +template +struct __select_2nd { + typedef _Tp type; +}; +template +typename __select_2nd() = declval<_Arg>())), + true_type>::type +__is_assignable_test(int); +template +false_type __is_assignable_test(...); +template ::value || is_void<_Arg>::value> +struct __is_assignable_imp + : public decltype((__is_assignable_test<_Tp, _Arg>(0))) {}; +template +struct __is_assignable_imp<_Tp, _Arg, true> : public false_type {}; +template +struct is_assignable : public __is_assignable_imp<_Tp, _Arg> {}; + +template +struct __libcpp_is_integral : public false_type {}; +template <> +struct __libcpp_is_integral : public true_type {}; +template <> +struct __libcpp_is_integral : public true_type {}; +template <> +struct __libcpp_is_integral : public true_type {}; +template <> +struct __libcpp_is_integral : public true_type {}; +template <> +struct __libcpp_is_integral : public true_type {}; +template <> +struct __libcpp_is_integral : public true_type {}; // NOLINT +template <> +struct __libcpp_is_integral : public true_type {}; // NOLINT +template <> +struct __libcpp_is_integral : public true_type {}; +template <> +struct __libcpp_is_integral : public true_type {}; +template <> +struct __libcpp_is_integral : public true_type {}; // NOLINT +template <> +struct __libcpp_is_integral : public true_type {}; // NOLINT +template <> +struct __libcpp_is_integral : public true_type {}; // NOLINT +template <> // NOLINTNEXTLINE +struct __libcpp_is_integral : public true_type {}; +template +struct is_integral + : public __libcpp_is_integral::type> {}; + +template +struct __libcpp_is_floating_point : public false_type {}; +template <> +struct __libcpp_is_floating_point : public true_type {}; +template <> +struct __libcpp_is_floating_point : public true_type {}; +template <> +struct __libcpp_is_floating_point : public true_type {}; +template +struct is_floating_point + : public __libcpp_is_floating_point::type> {}; + +template +struct is_arithmetic + : public integral_constant::value || + is_floating_point<_Tp>::value> {}; + +template +struct __libcpp_is_pointer : public false_type {}; +template +struct __libcpp_is_pointer<_Tp*> : public true_type {}; +template +struct is_pointer : public __libcpp_is_pointer::type> { +}; + +template +struct __libcpp_is_member_pointer : public false_type {}; +template +struct __libcpp_is_member_pointer<_Tp _Up::*> : public true_type {}; +template +struct is_member_pointer + : public __libcpp_is_member_pointer::type> {}; + +template +struct __libcpp_union : public false_type {}; +template +struct is_union : public __libcpp_union::type> {}; + +template +struct is_reference : false_type {}; +template +struct is_reference : true_type {}; +template +struct is_reference : true_type {}; + +template +inline constexpr bool is_reference_v = is_reference::value; + +struct __two { + char __lx[2]; +}; + +namespace __is_class_imp { +template +char __test(int _Tp::*); +template +__two __test(...); +} // namespace __is_class_imp +template +struct is_class + : public integral_constant(0)) == 1 && + !is_union<_Tp>::value> {}; + +template +struct __is_nullptr_t_impl : public false_type {}; +template <> +struct __is_nullptr_t_impl : public true_type {}; +template +struct __is_nullptr_t + : public __is_nullptr_t_impl::type> {}; +template +struct is_null_pointer + : public __is_nullptr_t_impl::type> {}; + +template +struct is_enum + : public integral_constant< + bool, !is_void<_Tp>::value && !is_integral<_Tp>::value && + !is_floating_point<_Tp>::value && !is_array<_Tp>::value && + !is_pointer<_Tp>::value && !is_reference<_Tp>::value && + !is_member_pointer<_Tp>::value && !is_union<_Tp>::value && + !is_class<_Tp>::value && !is_function<_Tp>::value> {}; + +template +struct is_scalar + : public integral_constant< + bool, is_arithmetic<_Tp>::value || is_member_pointer<_Tp>::value || + is_pointer<_Tp>::value || __is_nullptr_t<_Tp>::value || + is_enum<_Tp>::value> {}; +template <> +struct is_scalar : public true_type {}; + } // namespace std #endif // STD_TYPE_TRAITS_H @@ -442,6 +604,18 @@ _CheckOptionalLikeConstructor<_QualUp>, __check_tuple_constructor_fail>; + + template + using _CheckOptionalLikeAssign = _If< + _And< + _IsNotSame<_Up, _Tp>, + is_constructible<_Tp, _QualUp>, + is_assignable<_Tp&, _QualUp> + >::value, + _CheckOptionalLikeConstructor<_QualUp>, + __check_tuple_constructor_fail + >; + public: constexpr optional() noexcept {} constexpr optional(const optional&) = default; @@ -492,6 +666,30 @@ int> = 0> constexpr explicit optional(optional<_Up>&& __v); + constexpr optional& operator=(nullopt_t) noexcept; + + optional& operator=(const optional&); + + optional& operator=(optional&&); + + template , optional>, + _Or<_IsNotSame<__uncvref_t<_Up>, value_type>, + _Not>>, + is_constructible, + is_assignable>::value>> + constexpr optional& operator=(_Up&& __v); + + template :: + template __enable_assign<_Up>(), + int> = 0> + constexpr optional& operator=(const optional<_Up>& __v); + + template :: + template __enable_assign<_Up>(), + int> = 0> + constexpr optional& operator=(optional<_Up>&& __v); + const _Tp& operator*() const&; _Tp& operator*() &; const _Tp&& operator*() const&&; @@ -556,7 +754,6 @@ namespace optional_internal { -// Whether T is constructible or convertible from optional. template struct is_constructible_convertible_from_optional : std::integral_constant< @@ -569,6 +766,15 @@ std::is_convertible&, T>::value || std::is_convertible&&, T>::value> {}; +template +struct is_constructible_convertible_assignable_from_optional + : std::integral_constant< + bool, is_constructible_convertible_from_optional::value || + std::is_assignable&>::value || + std::is_assignable&&>::value || + std::is_assignable&>::value || + std::is_assignable&&>::value> {}; + } // namespace optional_internal template @@ -666,6 +872,44 @@ bool>::type = false> explicit optional(optional&& rhs); + optional& operator=(nullopt_t) noexcept; + + optional& operator=(const optional& src); + + optional& operator=(optional&& src); + + template < + typename U = T, + typename = typename std::enable_if, typename std::decay::type>>, + absl::negation< + absl::conjunction, + std::is_same::type>>>, + std::is_constructible, std::is_assignable>::value>::type> + optional& operator=(U&& v); + + template < + typename U, + typename = typename std::enable_if>, + std::is_constructible, std::is_assignable, + absl::negation< + optional_internal:: + is_constructible_convertible_assignable_from_optional< + T, U>>>::value>::type> + optional& operator=(const optional& rhs); + + template >, std::is_constructible, + std::is_assignable, + absl::negation< + optional_internal:: + is_constructible_convertible_assignable_from_optional< + T, U>>>::value>::type> + optional& operator=(optional&& rhs); + const T& operator*() const&; T& operator*() &; const T&& operator*() const&&; @@ -744,6 +988,15 @@ std::is_convertible&&, T>::value || std::is_convertible&&, T>::value> {}; +template +struct IsAssignableFromOptional + : std::integral_constant< + bool, IsConvertibleFromOptional::value || + std::is_assignable&>::value || + std::is_assignable&>::value || + std::is_assignable&&>::value || + std::is_assignable&&>::value> {}; + } // namespace internal template @@ -818,6 +1071,36 @@ bool>::type = false> constexpr explicit Optional(U&& value); + Optional& operator=(const Optional& other) noexcept; + + Optional& operator=(Optional&& other) noexcept; + + Optional& operator=(nullopt_t); + + template + typename std::enable_if< + !std::is_same, Optional>::value && + std::is_constructible::value && + std::is_assignable::value && + (!std::is_scalar::value || + !std::is_same::type, T>::value), + Optional&>::type + operator=(U&& value) noexcept; + + template + typename std::enable_if::value && + std::is_constructible::value && + std::is_assignable::value, + Optional&>::type + operator=(const Optional& other) noexcept; + + template + typename std::enable_if::value && + std::is_constructible::value && + std::is_assignable::value, + Optional&>::type + operator=(Optional&& other) noexcept; + const T& operator*() const&; T& operator*() &; const T&& operator*() const&&; @@ -904,6 +1187,7 @@ ReplaceAllOccurrences(SourceCode, "$optional", GetParam().TypeName); std::vector> Headers; + Headers.emplace_back("cstddef.h", CSDtdDefHeader); Headers.emplace_back("std_initializer_list.h", StdInitializerListHeader); Headers.emplace_back("std_type_traits.h", StdTypeTraitsHeader); Headers.emplace_back("std_utility.h", StdUtilityHeader); @@ -1475,9 +1759,161 @@ // FIXME: Add tests that call `reset` in conditional branches. } +TEST_P(UncheckedOptionalAccessTest, ValueAssignment) { + ExpectLatticeChecksFor(R"( + #include "unchecked_optional_access_test.h" + + struct Foo {}; + + void target() { + $ns::$optional opt; + opt = Foo(); + opt.value(); + /*[[check]]*/ + } + )", + UnorderedElementsAre(Pair("check", "safe"))); + + ExpectLatticeChecksFor(R"( + #include "unchecked_optional_access_test.h" + + struct Foo {}; + + void target() { + $ns::$optional opt; + (opt = Foo()).value(); + (void)0; + /*[[check]]*/ + } + )", + UnorderedElementsAre(Pair("check", "safe"))); + + ExpectLatticeChecksFor(R"( + #include "unchecked_optional_access_test.h" + + struct MyString { + MyString(const char*); + }; + + void target() { + $ns::$optional opt; + opt = "foo"; + opt.value(); + /*[[check]]*/ + } + )", + UnorderedElementsAre(Pair("check", "safe"))); + + ExpectLatticeChecksFor(R"( + #include "unchecked_optional_access_test.h" + + struct MyString { + MyString(const char*); + }; + + void target() { + $ns::$optional opt; + (opt = "foo").value(); + /*[[check]]*/ + } + )", + UnorderedElementsAre(Pair("check", "safe"))); +} + +TEST_P(UncheckedOptionalAccessTest, OptionalConversionAssignment) { + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo {}; + + struct Bar { + Bar(const Foo&); + }; + + void target() { + $ns::$optional opt1 = Foo(); + $ns::$optional opt2; + opt2 = opt1; + opt2.value(); + /*[[check]]*/ + } + )", + UnorderedElementsAre(Pair("check", "safe"))); + + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo {}; + + struct Bar { + Bar(const Foo&); + }; + + void target() { + $ns::$optional opt1; + $ns::$optional opt2; + if (opt2.has_value()) { + opt2 = opt1; + opt2.value(); + /*[[check]]*/ + } + } + )", + UnorderedElementsAre(Pair("check", "unsafe: input.cc:15:9"))); + + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + struct Foo {}; + + struct Bar { + Bar(const Foo&); + }; + + void target() { + $ns::$optional opt1 = Foo(); + $ns::$optional opt2; + (opt2 = opt1).value(); + (void)0; + /*[[check]]*/ + } + )", + UnorderedElementsAre(Pair("check", "safe"))); +} + +TEST_P(UncheckedOptionalAccessTest, NulloptAssignment) { + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = 3; + opt = $ns::nullopt; + opt.value(); + /*[[check]]*/ + } + )", + UnorderedElementsAre(Pair("check", "unsafe: input.cc:7:7"))); + + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = 3; + (opt = $ns::nullopt).value(); + /*[[check]]*/ + } + )", + UnorderedElementsAre(Pair("check", "unsafe: input.cc:6:7"))); +} + // FIXME: Add support for: // - constructors (copy, move) -// - assignment operators (default, copy, move, non-standard) +// - assignment operators (default, copy, move) // - swap // - invalidation (passing optional by non-const reference/pointer) // - `value_or(nullptr) != nullptr`, `value_or(0) != 0`, `value_or("").empty()`