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

[libc++] std::ranges::__movable_box can overwrite data that comes after it #70506

Closed
huixie90 opened this issue Oct 27, 2023 · 0 comments · Fixed by #71314
Closed

[libc++] std::ranges::__movable_box can overwrite data that comes after it #70506

huixie90 opened this issue Oct 27, 2023 · 0 comments · Fixed by #71314
Assignees
Labels
ABI Application Binary Interface libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>`

Comments

@huixie90
Copy link
Contributor

huixie90 commented Oct 27, 2023

std::ranges::__movable_box can overwrite data that comes after it
this is similar to #68552

This example shows how user would hit this bug

int main() {
    auto f = [l = 0.0L, b = false](int i) { return i; };

    auto v1 = std::vector{1, 2, 3, 4, 5} | std::views::transform(f);
    auto v2 = std::vector{1, 2, 3, 4, 5} | std::views::transform(f);

    v1 = std::move(v2);

    for (auto x : v1) {  // SEGV here
        std::cout << x;
    }
}

This example explains why it happens

struct A {
    A() {}

    double x{};
    bool y{};

    A(const A&) = default;
    A& operator=(const A&) = delete;
};

struct Test {
    Test() {}

    [[no_unique_address]] std::ranges::__movable_box<A> a;
    bool b = true;
};


int main(){
    static_assert(std::ranges::__doesnt_need_empty_state<A>);
    Test test;
    assert(test.b);
    test.a = std::ranges::__movable_box<A>{};
    assert(test.b);  // boom
}

movable boxes are widely used in ranges.

It uses [[no_unique_address]] in its data member. But on operator=, it calls destroy_at followed by construct_at. If the data member has tail padding, the tail padding would be zeroed out. If the user put the next data in the tail padding, this would overwrite the next data member. We have done this by ourselves, e.g. in transform_view, the next data member is the underlying view and the first example shows that the underlying view's memory was zeroed out, causing it to crash

https://godbolt.org/z/GcYoer4W4
https://godbolt.org/z/hvh4v1EWK

@huixie90 huixie90 self-assigned this Oct 27, 2023
@huixie90 huixie90 added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 27, 2023
@EugeneZelenko EugeneZelenko added the ranges Issues related to `<ranges>` label Oct 27, 2023
@ldionne ldionne added the ABI Application Binary Interface label Nov 30, 2023
huixie90 added a commit that referenced this issue Jan 2, 2024
…ata that lives in the tail padding (#71314)

fixes #70506 

The detailed problem description is in #70506 

The original proposed fix was to remove `[[no_unique_address]]` except
when `_Tp` is empty.

Edit:
After the discussion in the comments below, the new fix here is to
remove the `[[no_unique_address]]` from `movable_box` in the cases where
we need to add our own assignment operator, which has contains the
problematic `construct_at`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI Application Binary Interface libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>`
Projects
None yet
3 participants