Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible data corruption when assigning variant to itself #164

Open
SergeyIvanov87 opened this issue Jan 11, 2018 · 4 comments
Open

Possible data corruption when assigning variant to itself #164

SergeyIvanov87 opened this issue Jan 11, 2018 · 4 comments
Assignees

Comments

@SergeyIvanov87
Copy link

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
Copy link
Contributor

/cc @artemp

@artemp
Copy link
Contributor

artemp commented Jan 12, 2018

@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
Copy link

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
Copy link
Contributor

artemp commented Aug 12, 2019

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants