Unverified Commit 134c9159 authored by Jan Kokemüller's avatar Jan Kokemüller Committed by GitHub
Browse files

[libc++] Fix UB in <expected> related to "has value" flag (#68552) (#68733) 

The calls to std::construct_at might overwrite the previously set
__has_value_ flag in the case where the flag is overlapping with
the actual value or error being stored (since we use [[no_unique_address]]).
To fix this issue, this patch ensures that we initialize the
__has_value_ flag after we call std::construct_at.

Fixes #68552
parent 849f963e

libcxx/include/__expected/expected.h

+82 −95
Original line number Diff line number Diff line
@@ -119,9 +119,7 @@ public: 
  _LIBCPP_HIDE_FROM_ABI constexpr expected()

    noexcept(is_nothrow_default_constructible_v<_Tp>) // strengthened

    requires is_default_constructible_v<_Tp>

      : __has_val_(true) {

    std::construct_at(std::addressof(__union_.__val_));

  }

      : __union_(std::in_place), __has_val_(true) {}



  _LIBCPP_HIDE_FROM_ABI constexpr expected(const expected&) = delete;
@@ -136,14 +134,7 @@ public: 
    noexcept(is_nothrow_copy_constructible_v<_Tp> && is_nothrow_copy_constructible_v<_Err>) // strengthened

    requires(is_copy_constructible_v<_Tp> && is_copy_constructible_v<_Err> &&

             !(is_trivially_copy_constructible_v<_Tp> && is_trivially_copy_constructible_v<_Err>))

      : __has_val_(__other.__has_val_) {

    if (__has_val_) {

      std::construct_at(std::addressof(__union_.__val_), __other.__union_.__val_);

    } else {

      std::construct_at(std::addressof(__union_.__unex_), __other.__union_.__unex_);

    }

  }



      : __union_(__other.__has_val_, __other.__union_), __has_val_(__other.__has_val_) { }



  _LIBCPP_HIDE_FROM_ABI constexpr expected(expected&&)

    requires(is_move_constructible_v<_Tp> && is_move_constructible_v<_Err>
@@ -154,13 +145,7 @@ public: 
    noexcept(is_nothrow_move_constructible_v<_Tp> && is_nothrow_move_constructible_v<_Err>)

    requires(is_move_constructible_v<_Tp> && is_move_constructible_v<_Err> &&

             !(is_trivially_move_constructible_v<_Tp> && is_trivially_move_constructible_v<_Err>))

      : __has_val_(__other.__has_val_) {

    if (__has_val_) {

      std::construct_at(std::addressof(__union_.__val_), std::move(__other.__union_.__val_));

    } else {

      std::construct_at(std::addressof(__union_.__unex_), std::move(__other.__union_.__unex_));

    }

  }

      : __union_(__other.__has_val_, std::move(__other.__union_)), __has_val_(__other.__has_val_) { }



private:

  template <class _Up, class _OtherErr, class _UfQual, class _OtherErrQual>
@@ -200,26 +185,14 @@ public: 
  expected(const expected<_Up, _OtherErr>& __other)

    noexcept(is_nothrow_constructible_v<_Tp, const _Up&> &&

             is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened

      : __has_val_(__other.__has_val_) {

    if (__has_val_) {

      std::construct_at(std::addressof(__union_.__val_), __other.__union_.__val_);

    } else {

      std::construct_at(std::addressof(__union_.__unex_), __other.__union_.__unex_);

    }

  }

      : __union_(__other.__has_val_, __other.__union_), __has_val_(__other.__has_val_) {}



  template <class _Up, class _OtherErr>

    requires __can_convert<_Up, _OtherErr, _Up, _OtherErr>::value

  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_Up, _Tp> || !is_convertible_v<_OtherErr, _Err>)

  expected(expected<_Up, _OtherErr>&& __other)

    noexcept(is_nothrow_constructible_v<_Tp, _Up> && is_nothrow_constructible_v<_Err, _OtherErr>) // strengthened

      : __has_val_(__other.__has_val_) {

    if (__has_val_) {

      std::construct_at(std::addressof(__union_.__val_), std::move(__other.__union_.__val_));

    } else {

      std::construct_at(std::addressof(__union_.__unex_), std::move(__other.__union_.__unex_));

    }

  }

      : __union_(__other.__has_val_, std::move(__other.__union_)), __has_val_(__other.__has_val_) {}



  template <class _Up = _Tp>

    requires(!is_same_v<remove_cvref_t<_Up>, in_place_t> && !is_same_v<expected, remove_cvref_t<_Up>> &&
@@ -227,61 +200,47 @@ public: 
             (!is_same_v<remove_cv_t<_Tp>, bool> || !__is_std_expected<remove_cvref_t<_Up>>::value))

  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_Up, _Tp>)

      expected(_Up&& __u) noexcept(is_nothrow_constructible_v<_Tp, _Up>) // strengthened

      : __has_val_(true) {

    std::construct_at(std::addressof(__union_.__val_), std::forward<_Up>(__u));

  }

      : __union_(std::in_place, std::forward<_Up>(__u)), __has_val_(true) {}



  template <class _OtherErr>

    requires is_constructible_v<_Err, const _OtherErr&>

  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<const _OtherErr&, _Err>)

  expected(const unexpected<_OtherErr>& __unex)

    noexcept(is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened

      : __has_val_(false) {

    std::construct_at(std::addressof(__union_.__unex_), __unex.error());

  }

      : __union_(std::unexpect, __unex.error()), __has_val_(false) {}



  template <class _OtherErr>

    requires is_constructible_v<_Err, _OtherErr>

  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherErr, _Err>)

  expected(unexpected<_OtherErr>&& __unex)

    noexcept(is_nothrow_constructible_v<_Err, _OtherErr>) // strengthened

      : __has_val_(false) {

    std::construct_at(std::addressof(__union_.__unex_), std::move(__unex.error()));

  }

      : __union_(std::unexpect, std::move(__unex.error())), __has_val_(false) {}



  template <class... _Args>

    requires is_constructible_v<_Tp, _Args...>

  _LIBCPP_HIDE_FROM_ABI constexpr explicit expected(in_place_t, _Args&&... __args)

    noexcept(is_nothrow_constructible_v<_Tp, _Args...>) // strengthened

      : __has_val_(true) {

    std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);

  }

      : __union_(std::in_place, std::forward<_Args>(__args)...), __has_val_(true) {}



  template <class _Up, class... _Args>

    requires is_constructible_v< _Tp, initializer_list<_Up>&, _Args... >

  _LIBCPP_HIDE_FROM_ABI constexpr explicit

  expected(in_place_t, initializer_list<_Up> __il, _Args&&... __args)

    noexcept(is_nothrow_constructible_v<_Tp, initializer_list<_Up>&, _Args...>) // strengthened

      : __has_val_(true) {

    std::construct_at(std::addressof(__union_.__val_), __il, std::forward<_Args>(__args)...);

  }

      : __union_(std::in_place, __il, std::forward<_Args>(__args)...), __has_val_(true) {}



  template <class... _Args>

    requires is_constructible_v<_Err, _Args...>

  _LIBCPP_HIDE_FROM_ABI constexpr explicit expected(unexpect_t, _Args&&... __args)

    noexcept(is_nothrow_constructible_v<_Err, _Args...>) // strengthened

      : __has_val_(false) {

    std::construct_at(std::addressof(__union_.__unex_), std::forward<_Args>(__args)...);

  }

      : __union_(std::unexpect, std::forward<_Args>(__args)...), __has_val_(false) {}



  template <class _Up, class... _Args>

    requires is_constructible_v< _Err, initializer_list<_Up>&, _Args... >

  _LIBCPP_HIDE_FROM_ABI constexpr explicit

  expected(unexpect_t, initializer_list<_Up> __il, _Args&&... __args)

    noexcept(is_nothrow_constructible_v<_Err, initializer_list<_Up>&, _Args...>) // strengthened

      : __has_val_(false) {

    std::construct_at(std::addressof(__union_.__unex_), __il, std::forward<_Args>(__args)...);

  }

      : __union_(std::unexpect, __il, std::forward<_Args>(__args)...), __has_val_(false) {}



  // [expected.object.dtor], destructor
@@ -440,9 +399,10 @@ public: 
      std::destroy_at(std::addressof(__union_.__val_));

    } else {

      std::destroy_at(std::addressof(__union_.__unex_));

      __has_val_ = true;

    }

    return *std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);

    std::construct_at(std::addressof(__union_.__val_), std::forward<_Args>(__args)...);

    __has_val_ = true;

    return __union_.__val_;

  }



  template <class _Up, class... _Args>
@@ -452,9 +412,10 @@ public: 
      std::destroy_at(std::addressof(__union_.__val_));

    } else {

      std::destroy_at(std::addressof(__union_.__unex_));

      __has_val_ = true;

    }

    return *std::construct_at(std::addressof(__union_.__val_), __il, std::forward<_Args>(__args)...);

    std::construct_at(std::addressof(__union_.__val_), __il, std::forward<_Args>(__args)...);

    __has_val_ = true;

    return __union_.__val_;

  }
@@ -893,11 +854,15 @@ public: 
  }



private:

  struct __empty_t {};



  template <class _ValueType, class _ErrorType>

  union __union_t {

    _LIBCPP_HIDE_FROM_ABI constexpr __union_t() {}

    template <class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(std::in_place_t, _Args&&... __args)

        : __val_(std::forward<_Args>(__args)...) {}



    template <class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(std::unexpect_t, _Args&&... __args)

        : __unex_(std::forward<_Args>(__args)...) {}



    template <class _Func, class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(
@@ -909,6 +874,14 @@ private: 
        std::__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)

        : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}



    template <class _Union>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(bool __has_val, _Union&& __other) {

      if (__has_val)

        std::construct_at(std::addressof(__val_), std::forward<_Union>(__other).__val_);

      else

        std::construct_at(std::addressof(__unex_), std::forward<_Union>(__other).__unex_);

    }



    _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()

      requires(is_trivially_destructible_v<_ValueType> && is_trivially_destructible_v<_ErrorType>)

    = default;
@@ -927,10 +900,17 @@ private: 
  template <class _ValueType, class _ErrorType>

    requires(is_trivially_move_constructible_v<_ValueType> && is_trivially_move_constructible_v<_ErrorType>)

  union __union_t<_ValueType, _ErrorType> {

    _LIBCPP_HIDE_FROM_ABI constexpr __union_t() : __empty_() {}

    _LIBCPP_HIDE_FROM_ABI constexpr __union_t(const __union_t&) = default;

    _LIBCPP_HIDE_FROM_ABI constexpr __union_t& operator=(const __union_t&) = default;



    template <class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(std::in_place_t, _Args&&... __args)

        : __val_(std::forward<_Args>(__args)...) {}



    template <class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(std::unexpect_t, _Args&&... __args)

        : __unex_(std::forward<_Args>(__args)...) {}



    template <class _Func, class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(

        std::__expected_construct_in_place_from_invoke_tag, _Func&& __f, _Args&&... __args)
@@ -941,6 +921,14 @@ private: 
        std::__expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)

        : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}



    template <class _Union>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(bool __has_val, _Union&& __other) {

      if (__has_val)

        std::construct_at(std::addressof(__val_), std::forward<_Union>(__other).__val_);

      else

        std::construct_at(std::addressof(__unex_), std::forward<_Union>(__other).__unex_);

    }



    _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()

      requires(is_trivially_destructible_v<_ValueType> && is_trivially_destructible_v<_ErrorType>)

    = default;
@@ -950,7 +938,6 @@ private: 
      requires(!is_trivially_destructible_v<_ValueType> || !is_trivially_destructible_v<_ErrorType>)

    {}



    _LIBCPP_NO_UNIQUE_ADDRESS __empty_t __empty_;

    _LIBCPP_NO_UNIQUE_ADDRESS _ValueType __val_;

    _LIBCPP_NO_UNIQUE_ADDRESS _ErrorType __unex_;

  };
@@ -998,11 +985,7 @@ public: 
  _LIBCPP_HIDE_FROM_ABI constexpr expected(const expected& __rhs)

    noexcept(is_nothrow_copy_constructible_v<_Err>) // strengthened

    requires(is_copy_constructible_v<_Err> && !is_trivially_copy_constructible_v<_Err>)

      : __has_val_(__rhs.__has_val_) {

    if (!__rhs.__has_val_) {

      std::construct_at(std::addressof(__union_.__unex_), __rhs.__union_.__unex_);

    }

  }

      : __union_(__rhs.__has_val_, __rhs.__union_), __has_val_(__rhs.__has_val_) {}



  _LIBCPP_HIDE_FROM_ABI constexpr expected(expected&&)

    requires(is_move_constructible_v<_Err> && is_trivially_move_constructible_v<_Err>)
@@ -1011,51 +994,35 @@ public: 
  _LIBCPP_HIDE_FROM_ABI constexpr expected(expected&& __rhs)

    noexcept(is_nothrow_move_constructible_v<_Err>)

    requires(is_move_constructible_v<_Err> && !is_trivially_move_constructible_v<_Err>)

      : __has_val_(__rhs.__has_val_) {

    if (!__rhs.__has_val_) {

      std::construct_at(std::addressof(__union_.__unex_), std::move(__rhs.__union_.__unex_));

    }

  }

      : __union_(__rhs.__has_val_, std::move(__rhs.__union_)), __has_val_(__rhs.__has_val_) {}



  template <class _Up, class _OtherErr>

    requires __can_convert<_Up, _OtherErr, const _OtherErr&>::value

  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<const _OtherErr&, _Err>)

  expected(const expected<_Up, _OtherErr>& __rhs)

    noexcept(is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened

      : __has_val_(__rhs.__has_val_) {

    if (!__rhs.__has_val_) {

      std::construct_at(std::addressof(__union_.__unex_), __rhs.__union_.__unex_);

    }

  }

      : __union_(__rhs.__has_val_, __rhs.__union_), __has_val_(__rhs.__has_val_) {}



  template <class _Up, class _OtherErr>

    requires __can_convert<_Up, _OtherErr, _OtherErr>::value

  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherErr, _Err>)

  expected(expected<_Up, _OtherErr>&& __rhs)

    noexcept(is_nothrow_constructible_v<_Err, _OtherErr>) // strengthened

      : __has_val_(__rhs.__has_val_) {

    if (!__rhs.__has_val_) {

      std::construct_at(std::addressof(__union_.__unex_), std::move(__rhs.__union_.__unex_));

    }

  }

      : __union_(__rhs.__has_val_, std::move(__rhs.__union_)), __has_val_(__rhs.__has_val_) {}



  template <class _OtherErr>

    requires is_constructible_v<_Err, const _OtherErr&>

  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<const _OtherErr&, _Err>)

  expected(const unexpected<_OtherErr>& __unex)

    noexcept(is_nothrow_constructible_v<_Err, const _OtherErr&>) // strengthened

      : __has_val_(false) {

    std::construct_at(std::addressof(__union_.__unex_), __unex.error());

  }

      : __union_(std::unexpect, __unex.error()), __has_val_(false) {}



  template <class _OtherErr>

    requires is_constructible_v<_Err, _OtherErr>

  _LIBCPP_HIDE_FROM_ABI constexpr explicit(!is_convertible_v<_OtherErr, _Err>)

  expected(unexpected<_OtherErr>&& __unex)

    noexcept(is_nothrow_constructible_v<_Err, _OtherErr>) // strengthened

      : __has_val_(false) {

    std::construct_at(std::addressof(__union_.__unex_), std::move(__unex.error()));

  }

      : __union_(std::unexpect, std::move(__unex.error())), __has_val_(false) {}



  _LIBCPP_HIDE_FROM_ABI constexpr explicit expected(in_place_t) noexcept : __has_val_(true) {}
@@ -1063,17 +1030,13 @@ public: 
    requires is_constructible_v<_Err, _Args...>

  _LIBCPP_HIDE_FROM_ABI constexpr explicit expected(unexpect_t, _Args&&... __args)

    noexcept(is_nothrow_constructible_v<_Err, _Args...>) // strengthened

      : __has_val_(false) {

    std::construct_at(std::addressof(__union_.__unex_), std::forward<_Args>(__args)...);

  }

      : __union_(std::unexpect, std::forward<_Args>(__args)...), __has_val_(false) {}



  template <class _Up, class... _Args>

    requires is_constructible_v< _Err, initializer_list<_Up>&, _Args... >

  _LIBCPP_HIDE_FROM_ABI constexpr explicit expected(unexpect_t, initializer_list<_Up> __il, _Args&&... __args)

    noexcept(is_nothrow_constructible_v<_Err, initializer_list<_Up>&, _Args...>) // strengthened

      : __has_val_(false) {

    std::construct_at(std::addressof(__union_.__unex_), __il, std::forward<_Args>(__args)...);

  }

      : __union_(std::unexpect, __il, std::forward<_Args>(__args)...), __has_val_(false) {}



private:

  template <class _Func>
@@ -1507,11 +1470,23 @@ private: 
  union __union_t {

    _LIBCPP_HIDE_FROM_ABI constexpr __union_t() : __empty_() {}



    template <class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(std::unexpect_t, _Args&&... __args)

        : __unex_(std::forward<_Args>(__args)...) {}



    template <class _Func, class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(

        __expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)

        : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}



    template <class _Union>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(bool __has_val, _Union&& __other) {

      if (__has_val)

        std::construct_at(std::addressof(__empty_));

      else

        std::construct_at(std::addressof(__unex_), std::forward<_Union>(__other).__unex_);

    }



    _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()

      requires(is_trivially_destructible_v<_ErrorType>)

    = default;
@@ -1534,11 +1509,23 @@ private: 
    _LIBCPP_HIDE_FROM_ABI constexpr __union_t(const __union_t&) = default;

    _LIBCPP_HIDE_FROM_ABI constexpr __union_t& operator=(const __union_t&) = default;



    template <class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(std::unexpect_t, _Args&&... __args)

        : __unex_(std::forward<_Args>(__args)...) {}



    template <class _Func, class... _Args>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(

        __expected_construct_unexpected_from_invoke_tag, _Func&& __f, _Args&&... __args)

        : __unex_(std::invoke(std::forward<_Func>(__f), std::forward<_Args>(__args)...)) {}



    template <class _Union>

    _LIBCPP_HIDE_FROM_ABI constexpr explicit __union_t(bool __has_val, _Union&& __other) {

      if (__has_val)

        std::construct_at(std::addressof(__empty_));

      else

        std::construct_at(std::addressof(__unex_), std::forward<_Union>(__other).__unex_);

    }



    _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()

      requires(is_trivially_destructible_v<_ErrorType>)

    = default;

libcxx/test/std/utilities/expected/expected.expected/assign/emplace.intializer_list.pass.cpp

+8 −0
Original line number Diff line number Diff line
@@ -81,6 +81,14 @@ constexpr bool test() { 
    assert(e.value().i == 10);

  }



  // TailClobberer

  {

    std::expected<TailClobberer<0>, bool> e(std::unexpect);

    auto list = {4, 5, 6};

    e.emplace(list);

    assert(e.has_value());

  }



  return true;

}

libcxx/test/std/utilities/expected/expected.expected/assign/emplace.pass.cpp

+7 −0
Original line number Diff line number Diff line
@@ -73,6 +73,13 @@ constexpr bool test() { 
    assert(e.value() == 10);

  }



  // TailClobberer

  {

    std::expected<TailClobberer<0>, bool> e(std::unexpect);

    e.emplace();

    assert(e.has_value());

  }



  return true;

}

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.convert.copy.pass.cpp

+9 −2
Original line number Diff line number Diff line
@@ -45,6 +45,7 @@ 
#include <utility>



#include "test_macros.h"

#include "../../types.h"



// Test Constraints:

template <class T1, class Err1, class T2, class Err2>
@@ -161,13 +162,19 @@ constexpr bool test() { 
    assert(e1.error() == 5);

  }



  // convert TailClobberer

  {

    const std::expected<TailClobbererNonTrivialMove<0>, char> e1;

    std::expected<TailClobberer<0>, char> e2 = e1;

    assert(e2.has_value());

    assert(e1.has_value());

  }



  return true;

}



void testException() {

#ifndef TEST_HAS_NO_EXCEPTIONS

  struct Except {};



  struct ThrowingInt {

    ThrowingInt(int) { throw Except{}; }

  };

libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.convert.move.pass.cpp

+9 −2
Original line number Diff line number Diff line
@@ -46,6 +46,7 @@ 


#include "MoveOnly.h"

#include "test_macros.h"

#include "../../types.h"



// Test Constraints:

template <class T1, class Err1, class T2, class Err2>
@@ -160,13 +161,19 @@ constexpr bool test() { 
    assert(e1.error().get() == 0);

  }



  // convert TailClobberer

  {

    std::expected<TailClobbererNonTrivialMove<0>, char> e1;

    std::expected<TailClobberer<0>, char> e2 = std::move(e1);

    assert(e2.has_value());

    assert(e1.has_value());

  }



  return true;

}



void testException() {

#ifndef TEST_HAS_NO_EXCEPTIONS

  struct Except {};



  struct ThrowingInt {

    ThrowingInt(int) { throw Except{}; }

  };