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

Bug in environment::key_char_traits::move on Windows #462

Open
js-nano opened this issue Jan 29, 2025 · 2 comments
Open

Bug in environment::key_char_traits::move on Windows #462

js-nano opened this issue Jan 29, 2025 · 2 comments

Comments

@js-nano
Copy link

js-nano commented Jan 29, 2025

There's a bug in environment::key_char_traits::move() on Windows:

return std::move_backward(s2, s2 + n, s1);

The final parameter should point one-past-the-end of the range to be moved to -- https://en.cppreference.com/w/cpp/algorithm/move_backward

The correct code is:

    return std::move_backward(s2, s2 + n, s1 + n);

Analysis

I saw segfaults when adding elements to a std::unordered_map<env::key, env::value> from an env::key_view/env::value_view

While investigating, I noticed that, occasionally, a newly constructed key would be empty, despite all the parameters looking plausible. I eventually narrowed it down to the use of move_backward when moving the string contents -- the final parameter should point to the end of the range to be moved to.

A minimal demonstration of the underlying problem:

#include <boost/process/v2/environment.hpp>
#include <array>
#include <cassert>

int main() {
    std::array<boost::process::v2::environment::key::string_type, 2> arr;
    arr[0] = L"def";
    arr[1].assign(arr[0].data(), arr[0].size());
    assert(arr[1] == L"def");
}

In this demonstration:

  • The source string at arr[0] has an address before the destination string arr[1]
  • The small string optimization is in effect, so the addresses of the buffers are in the same order as the strings themselves
  • Therefore, in char_traits::move(), s1 (dest) is after s2 (src)
  • So the move_backward() branch is taken
  • move_backward() moves the chars before the start of the destination string, due to the 3rd parameter
  • Either you get a segfault (due to writing before the start of the string memory), or the assert fails (due to the contents being written before the target string buffer)
@klemens-morgenstern
Copy link
Collaborator

Do you want to create a PR?

@js-nano
Copy link
Author

js-nano commented Feb 3, 2025

Very happy to if that makes things easier for you; equally happy for you to make the change if that works for you

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

2 participants