mapbox / variant

C++11/C++14 Variant
BSD 3-Clause "New" or "Revised" License
371 stars 100 forks source link

Possible data corruption when assigning variant to itself #164

Open SergeyIvanov87 opened 6 years ago

SergeyIvanov87 commented 6 years ago

Hi, Thanks to you variant implementation! But i see, that there are issue in member

VARIANT_INLINE variant<Types...>& operator=(variant<Types...> const& other)
{
    copy_assign(other);
    return *this;
}

Now there are no check for &other ==this and copy_assign can destroy variant member data's before actual copy to itself

VARIANT_INLINE void copy_assign(variant<Types...> const& rhs)
{
    helper_type::destroy(type_index, &data);
    type_index = detail::invalid_value;
    helper_type::copy(rhs.type_index, &rhs.data, &data);
    type_index = rhs.type_index;
}

So, in result helper_type::copy(rhs.type_index, &rhs.data, &data); can operate with invalid data

Best Regards

springmeyer commented 6 years ago

/cc @artemp

artemp commented 6 years ago

@SergeyIvanov87 - It looks like you're correct but I vaguely recall having some reasons for not having a check. I'll need to dig through history to see why implementation ended up like this and if we need to fix it. Thanks!

mgambrell commented 5 years ago

This struck me as well. i'm upgrading it from "possible" to "definite". I fixed it by adding if(&rhs==this) return; to copy_assign() and move_assign() but I don't really have a lot of confidence that's technically correct. Nevertheless it did fix my manifestation of the problem and I didn't immediately see any other bad consequences.

artemp commented 5 years ago

@SergeyIvanov87 @mgambrell @springmeyer - I don't remember why there's no check for the self assignment. I can only think that at some point operaror= signature was :

VARIANT_INLINE variant<Types...>& operator=(variant<Types...>  other) // <--- pass by value

I'll take a look at adding the test tomorrow, thanks!