vector incorrectly copies move-only objects using memcpy

[joseph.thomson@…]

The behaviour of __has_trivial_copy seems to have be corrected in GCC 4.9, such that deleted copy constructors are now considered trivial. This seems to be in-line with the C++ standard and Clang as explained in this issue:

​https://svn.boost.org/trac/boost/ticket/10389

This change seems to have exposed a bug in Boost.Container, where objects are copied using memcpy if they have a trivial copy constructor. However, memcpy should only be used as an optimization on trivially copyable objects, the requirements for which include no non-trivial move constructors or assignment operators.

Move-only classes like std::unique_ptr have trivial (but deleted) copy constructors, but are not trivially copyable, since they define move operations.

To fix this, I think that boost/container/details/utilities.hpp needs to be changed. Perhaps something like this:

template <typename I, typename O>
struct is_memtransfer_copy_assignable
{
   static const bool value = are_contiguous_and_same<I, O>::value &&
      boost::is_copy_assignable< typename ::std::iterator_traits<I>::value_type >::value &&
      boost::has_trivial_assign< typename ::std::iterator_traits<I>::value_type >::value;
};

template <typename I, typename O>
struct is_memtransfer_copy_constructible
{
   static const bool value = are_contiguous_and_same<I, O>::value &&
      boost::is_copy_constructible< typename ::std::iterator_traits<I>::value_type >::value &&
      boost::has_trivial_copy< typename ::std::iterator_traits<I>::value_type >::value;
};

This is a check for whether the class has a trivial copy constructor/assignment operator, which I believe should be fine. boost::is_copy_assignable used to exist in Boost 1.58.0, but seems to have been removed in the development trunk for some reason.

Note that _has_trivial_copy still behaves incorrectly in this circumstance in VC++12. In fact, VC++ seems to have broken boost::is_copy_assignable and boost::is_copy_constructible functions, as well as broken SFINAE detection for copy/move constructors and operators. Thus, the modification I have above will still work around this problem, but only because all of the functions return the wrong values.

[anonymous]

To clarify, I meant that the std::is_trivially_copyable is broken in VC++. In fact, it is a synonym for std::is_trivially_copy_constructible, which is just wrong. __has_trivial_copy is just one requirement of both std::is_trivially_copyable and std::is_trivially_copy_constructible. That said, I haven't even looked at how boost:has_trivial_copy and boost::has_trivial_assign are implemented on VC++.

[Ion Gaztañaga]

Could you check please if commit SHA-1: 32f4d91cecc331f8f6ccf8e4d04041c573f1f347

​https://github.com/boostorg/move/commit/32f4d91cecc331f8f6ccf8e4d04041c573f1f347

solves the problem?

[joseph.thomson@…]

Assuming that boost::move_detail::is_copy_constructible and boost::move_detail::is_copy_assignable work on VC++ (if the tests pass, it would seem this is the case), I still don't know if this change fixes the bug. The is_memtransfer_copy_constructible and is_memtransfer_copy_assignable functions, as far as I can tell, perform the same function as is_trivially_copy_constructible and is_trivially_copy_assignable, but they still have incorrect implementations which rely only on boost::has_trivial_copy and boost::has_trivial_assign. Surely you need to change the implementation of these functions to fix the problem?

[Ion Gaztañaga]

The trait is_memtransfer_copy_constructible, at least in boost 1.58, which is the version you've reported, does not depend on boost.TypeTraits but on Boost.Move, due to the dependency reduction work on that 1.58 version.

In Visual 2013, std::is_copy_constructible wrongly returns true for types with deleted copy constructors or assignments, but fortunately the intrinsic has_trivial_xxx returns false. E.g:

#include <boost/static_assert.hpp>
#include <boost/move/detail/type_traits.hpp>

struct asdf
{
   asdf(const asdf&) = delete;
   asdf & operator=(const asdf&) = delete;
};

int main()
{
   BOOST_STATIC_ASSERT(!(__has_trivial_copy(asdf)));
   BOOST_STATIC_ASSERT(!(__has_trivial_assign(asdf)));

   BOOST_STATIC_ASSERT(!(boost::move_detail::is_trivially_copy_constructible<asdf>::value));
   BOOST_STATIC_ASSERT(!(boost::move_detail::is_trivially_copy_assignable<asdf>::value));
}

So boost::container::vector should not try to memcpy a type with a deleted copy/assignment .

In any case, if a class has a public copy constructor which is trivial, and also a move constructor, boost::container is free to select the copy constructor as semantically should be consistent with the move construtor. Or are there scenarios where this could be a problem?

[joseph.thomson@…]

Sorry, I incorrectly assumed that trunk would contain the latest version of the source code (I was using 1.57.0, tested 1.58.0 briefly, and then looked to fix the change in trunk).

I have just tested your update, and it seems conceptually fine. However, it fails to compile on MSVC because yes_type and no_type are undefined. Copy-paste error I think. Here is the test program I used:

#include <boost/container/vector.hpp>

#include <iostream>
#include <memory>

int main() {
    auto deleter = [](int*) {
        std::cout << "delete\n";
    };

    using T = std::unique_ptr<int, decltype(deleter)>;

    std::cout << std::boolalpha;
    std::cout << "is_copy_constructible: " << boost::move_detail::is_copy_constructible<T>::value << "\n";
    std::cout << "is_copy_assignable: " << boost::move_detail::is_copy_assignable<T>::value << "\n";
    std::cout << "is_trivially_copy_constructible: " << boost::move_detail::is_trivially_copy_constructible<T>::value << "\n";
    std::cout << "is_trivially_copy_assignable: " << boost::move_detail::is_trivially_copy_assignable<T>::value << "\n";

    boost::container::vector<T> values;
    
    values.push_back(T(new int(), deleter));
    values.push_back(T(new int(), deleter));
}

You assessment about the MSVC behaviour agrees with mine. Boost.Container should still work thanks to the interaction of those two bugs :) I have looked at the 2015 preview, and it seems that they have fixed std::is_copy_constructible and std::is_copy_assignable, but not __has_trivial_copy, __has_trivial_assign and std::is_trivially_copyable. I have filed a bug about the latter, but whether or not it is fixed should not affect Boost.Container, as the std::is_copy_constructible and std::is_copy_assignable not correctly return false in this case.

[anonymous]

The above discussion evolves around MSVC, but for GCC 4.9 and up Boost is currently still broken, but for that the fix that is used for Clang should be applicable as well as far as I can tell. So the resolution for #10389 should also be applied for GCC 4.9 and up.

So this simple program has different behaviour:

#include <memory>
#include <boost/type_traits.hpp>

static_assert(!boost::has_trivial_copy<std::unique_ptr<int>>::value, 
    "Should not be trivially copyable");

• On GCC 4.7 the assert does not fire (using Boost 1.59), i.e. it is not considered trivially copyable.
• On GCC 4.9 the assert does fire
• On Clang 3.7 the assert does not fire (because Boost has a workaround for clang that doesn't use the has_trivial_copy intrinsic only but also tests for copy_constructible).

For 4.9 this Clang workaround should be used as well in my opinion.

[anonymous]

Note that more recent GCC versions have is_trivially_copyable as an intrinsic, which correctly marks a unique_ptr as not being trivially copyable (the difference with has_trivial_cop) seems to be that the newer intrinsic does take the requirement of a non-trivial destructor into account).

[joseph.thomson@…]

Replying to anonymous:

Note that more recent GCC versions have __is_trivially_copyable as an intrinsic, which correctly marks a unique_ptr as not being trivially copyable (the difference with __has_trivial_copy) seems to be that the newer intrinsic does take the requirement of a non-trivial destructor into account).

A trivially copyable class must have:

• No non-trivial copy constructors
• No non-trivial copy assignment operators
• No non-trivial move constructors
• No non-trivial move assignment operators
• A trivial destructor

__has_trivial_copy simply reports whether the class has a trivial copy constructor.

[anonymous]

Replying to joseph.thomson@…:

A trivially copyable class must have:

• No non-trivial copy constructors
• No non-trivial copy assignment operators
• No non-trivial move constructors
• No non-trivial move assignment operators
• A trivial destructor

__has_trivial_copy simply reports whether the class has a trivial copy constructor.

I get that, and I see that for clang there's a workaround so that boost::has_trivial_copy *also* checks whether it is copy constructible (boost/type_traits/has_trivial_copy.hpp )

template <typename T>
struct has_trivial_copy_impl
{
#ifdef BOOST_HAS_TRIVIAL_COPY
#  ifdef __clang__ // Why not do this for GCC 4.9 too?
   BOOST_STATIC_CONSTANT(bool, value = BOOST_HAS_TRIVIAL_COPY(T) && boost::is_copy_constructible<T>::value);
#  else
   BOOST_STATIC_CONSTANT(bool, value = BOOST_HAS_TRIVIAL_COPY(T));
#  endif
#else
   BOOST_STATIC_CONSTANT(bool, value =
      (::boost::type_traits::ice_and<
         ::boost::is_pod<T>::value,
         ::boost::type_traits::ice_not< ::boost::is_volatile<T>::value >::value
      >::value));
#endif

but this is only the case for Clang, not for GCC 4.9 and up; even though the semantics of the intrinsic for Clang and GCC are the same; so that's the problem I think needs addressing (and was the original intend of the ticket).

[joseph.thomson@…]

Actually, as far as I can see, the workaround will result in incorrect behaviour. It's been a while since I filed the original ticket, so I can't remember all the details, but I assume that the workaround exists to emulate the old (incorrect) behaviour of GCC on Clang.

Implicitly generated, defaulted and deleted copy constructors are all trivial. Thus, boost::has_trivial_copy should be true for a std::unique_ptr. This reflects the new, correct behaviour of __has_trivial_copy in GCC 4.9. However, to be trivially copy constructible, a class must:

• have no non-trivial copy constructors, and
• be copy constructible.

A std::unique_ptr is not copy constructible, even thought it has a trivial (deleted) copy constructor, so the standard library function std::is_trivially_copy_constructible should be false for a std::unique_ptr.

What I'm saying is that, if what you report is accurate, the behaviour of boost::has_trivial_copy is currently wrong for both GCC and Clang. It seemingly behaves like std::is_trivially_copy_constructible.

[joseph.thomson@…]

To avoid confusion:

behaviour of GCC on Clang.

[anonymous]

Cool, that clears up what boost::has_trivial_copy means, and but then we still have your comment here right:

Replying to joseph.thomson@…:

The is_memtransfer_copy_constructible and is_memtransfer_copy_assignable functions, as far as I can tell, perform the same function as is_trivially_copy_constructible and is_trivially_copy_assignable, but they still have incorrect implementations which rely only on boost::has_trivial_copy and boost::has_trivial_assign. Surely you need to change the implementation of these functions to fix the problem?

Because now although I'm happy I understand what has_trivial_copy should do I'm still a bit at a loss for which combinations of boost and compiler a boost::vector of unique_ptr will result in double free.

As far as I understand:

• GCC 4.7 & 4.8 are fine (because their intrinsic is broken)
• GCC 4.9 with boost 1.58 is susceptible to the problem
• Clang in fact is fine (but for the wrong reason)

• GCC 4.9 with boost 1.59 (fixed or not?)

[joseph.thomson@…]

Following the trail:

boost/container/detail/copy_move_algo.hpp

template <typename I, typename O>
struct is_memtransfer_copy_constructible
   : boost::move_detail::and_
      < are_contiguous_and_same<I, O>
      , container_detail::is_trivially_copy_constructible< typename ::boost::container::iterator_traits<I>::value_type >
      >
{};

boost/container/detail/type_traits.hpp

using ::boost::move_detail::is_trivially_copy_constructible;

boost/move/detail/type_traits.hpp

template<class T>
struct is_trivially_copy_constructible
{
   //In several compilers BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE return true even with
   //deleted copy constructors so make sure the type is copy constructible.
   static const bool value = ::boost::move_detail::is_pod<T>::value ||
                             ( ::boost::move_detail::is_copy_constructible<T>::value &&
                               BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE(T) );
};

#ifdef BOOST_MOVE_HAS_TRIVIAL_COPY
   #define BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE(T)   BOOST_MOVE_HAS_TRIVIAL_COPY(T)
#else
   #define BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE(T)   ::boost::move_detail::is_pod<T>::value
#endif

MSVC

#   define BOOST_MOVE_HAS_TRIVIAL_COPY(T)          (__has_trivial_copy(T)|| ::boost::move_detail::is_pod<T>::value)

Clang

#   if __has_feature(has_trivial_copy)
#     //There are problems with deleted copy constructors detected as trivially copyable.
#     //http://stackoverflow.com/questions/12754886/has-trivial-copy-behaves-differently-in-clang-and-gcc-whos-right
#     define BOOST_MOVE_HAS_TRIVIAL_COPY(T) (__has_trivial_copy(T) && ::boost::move_detail::is_copy_constructible<T>::value)
#   endif

GCC

#   define BOOST_MOVE_HAS_TRIVIAL_COPY(T) ((__has_trivial_copy(T) BOOST_MOVE_INTEL_TT_OPTS))

Note that Boost.Container no longer depends on Boost.TypeTraits, so the code you cited is not actually being used.

The original bug existed because Boost.Container only required boost::has_trivial_copy to be true to enable the memcpy optimization, and in GCC 4.9 the behaviour of __has_trivial_copy was fixed, meaning that boost::has_trivial_copy worked correctly in Boost 1.58 with GCC 4.9, meaning that Boost.Containers thought that std::unique_ptr was memcpy-able.

As you can see above, Boost.Container fixed the bug by depending on boost::move_detail::is_trivially_copy_constructible instead, which is the correct thing to do (std::unique_ptr should give false for this). As you can see, Boost.Move implements this by explicitly checking whether the class is copy constructible, so this bug should be gone on all compilers in Boost 1.59.

But looking at Boost.Move, BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE is incorrectly implemented as BOOST_MOVE_HAS_TRIVIAL_COPY. However, BOOST_MOVE_HAS_TRIVIAL_COPY behaves incorrectly on GCC 4.8 and MSVC, due to __has_trivial_move being broken, which means that BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE actually behaves correctly. This is also the case on Clang, but only because that "fix" has been applied to make it behave like GCC 4.8. Conversely, BOOST_MOVE_HAS_TRIVIAL_COPY behaves correctly on GCC 4.9, while BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE behaves incorrectly.

Thus, Boost.Move needs to do the following to fix these problems:

• Remove the "fix" for Clang
• Implement BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE correctly
• Document that BOOST_MOVE_HAS_TRIVIAL_COPY doesn't work on GCC 4.8 or MSVC

It is possible that they may be able to use SFINAE on GCC 4.8 to detect the lack of copy constructor, but I have already tried this on MSVC, and the bug exists with the SFINAE (alas, this may be the case with GCC 4.8 as well).

p.s. I have no access to GCC or Clang, so I was unable to test by conclusions. Perhaps you could?

[anonymous]

So, boost container indeed seems fixed, it behaves correct for Boost 1.59 with any compiler I threw at it (GCC 4.7, 4.9, 5.1 and Clang 3.7); the type traits themselves are indeed broken in the way you describe.

So test program is this:

#include <memory>
#include <boost/type_traits.hpp>

static_assert(boost::has_trivial_copy<std::unique_ptr<int>>::value, "unique ptr should have a trivial copy constructor");

This fails for GCC 4.7 and Clang 3.7 (due to the workaround in boost) but succeeds for GCC 4.9 and GCC 5.1.

Since has_trivial_copy_constructor is synonymous, you'll find that the assert fails for GCC 4.7 and Clang 3.7 again, but is true for GCC 4.9 and 5.1; indeed showing the converse behaviour.

When it comes to boost vector, the following dies badly for Boost 1.58 and GCC 4.9 / 5.1 but is fine for Boost 1.59 with any compiler (or for boost 1.58 with GCC 4.7 / Clang):

#include <memory>
#include <boost/container/vector.hpp>

int main() {
        auto v = boost::container::vector<std::unique_ptr<int>>{};
        for (auto i = 0u; i < 1000*1000; ++i)
                v.emplace_back(new int(i));
}

Thanks for your help, it has been quite useful!

[joseph.thomson@…]

No problem.

I'm not sure about boost::has_trivial_copy though, because that is part of Boost.TypeTraits, not Boost.Move. I was describing how BOOST_MOVE_HAS_TRIVIAL_COPY and BOOST_MOVE_IS_TRIVIALLY_COPY_CONSTRUCTIBLE are variously broken on different compilers. These are internal macros, so perhaps it isn't too big of a deal.

A separate bug report needs to be filed for boost::has_trivial_copy.

[Ion Gaztañaga]

Boost.Move's intrinsic macros are internal so there is no requirement to implement the standard wording. In any case the special Clang case will be removed and internally the library will assume that BOOST_MOVE_HAS_TRIVIAL_COPY does not guarantee the copy constructor is callable. Change committed:

​https://github.com/boostorg/move/commit/e7d24400cb986efaefa0acbff5de6e74eae876d9