1

thing contains 2 vectors, one of foo and one of bar.

The bar instances contain references to the foos - the potentially dangling ones.

The foo vector is filled precisely once, in things's constructor initializer list, and the bar vector is filled precisely once in things's constructor body.

main() holds a std::vector<thing> but this vector is filled incrementally without .reserve(), and is therefore periodically reallocating.

I am struggling to reproduce it in the minimal example below, but in the more heavyweight complete code the f1 and f2 references trigger the address sanitizer with "use after free".

I find this "slightly" surprising, because yes, the "direct members" of std::vector<foo> in thing (ie the start_ptr, size, capacity), they get realloc'd when things in main() grows. But I would have thought that the "heap resource" of foos could (?) stay the same when the std::vector<thing> get's realloc'd because there is no need to move them.

Is the answer here, that: "Yes the foo heap objects may not move when things realloc's, but this is by no means guaranteed and that's why I am getting inconsistent results"?

What exactly is and isn't guaranteed here that I can rely on?

#include <iostream>
#include <vector>

struct foo {
    int x;
    int y;
    // more stuff
    friend std::ostream& operator<<(std::ostream& os, const foo& f) {
        return os << "[" << f.x << "," << f.y << "]";
    }
};

struct bar {
    foo& f1; // dangerous reference
    foo& f2; // dangerous reference
    // more stuff
    bar(foo& f1_, foo& f2_) : f1(f1_), f2(f2_) {}

    friend std::ostream& operator<<(std::ostream& os, const bar& b) {
        return os << b.f1 << "=>" << b.f2 << "  ";
    }
};

struct thing {
    std::vector<foo> foos;
    std::vector<bar> bars;

    explicit thing(std::vector<foo> foos_) : foos(std::move(foos_)) {
        bars.reserve(foos.size());
        for (auto i = 0UL; i != foos.size(); ++i) {
            bars.emplace_back(foos[i], foos[(i + 1) % foos.size()]); // last one links back to start
        }
    }

    friend std::ostream& operator<<(std::ostream& os, const thing& t) {
        for (const auto& f: t.foos) os << f;
        os << "  |  ";
        for (const auto& b: t.bars) os << b;
        return os << "\n";
    }
};

int main() {
    std::vector<thing> things;
    things.push_back(thing({{1, 2}, {3, 4}, {5, 6}}));
    std::cout << &things[0] << std::endl;
    for (const auto& t: things) std::cout << t;
    
    things.push_back(thing({{1, 2}, {3, 4}, {5, 6}, {7, 8}}));
    std::cout << &things[0] << std::endl;
    for (const auto& t: things) std::cout << t;
    
    things.push_back(thing({{1, 2}, {3, 4}, {5, 6}, {7, 8}, {9, 10}}));
    std::cout << &things[0] << std::endl;
    for (const auto& t: things) std::cout << t;
    
    things.push_back(thing({{1, 2}, {3, 4}, {5, 6}, {7, 8}, {9, 10}, {11, 12}}));
    std::cout << &things[0] << std::endl;
    for (const auto& t: things) std::cout << t;
}
Oliver Schönrock
  • 1,038
  • 6
  • 11
  • Yeah. What did you expect? – Taekahn Feb 25 '22 at 22:51
  • @Taekahn It's expained in the question. I expected (with some suspicion) that the foo heap objects might stay in the same location on the heap when the `std::vector` "direct members" (ie startptr, size, capacity) get "moved" to a different place. – Oliver Schönrock Feb 25 '22 at 22:53
  • Anything that modifies a dynamic container invalidates references to objects of that container. `If the new size() is greater than capacity() then all iterators and references (including the past-the-end iterator) are invalidated. Otherwise only the past-the-end iterator is invalidated.` https://en.cppreference.com/w/cpp/container/vector/push_back https://en.cppreference.com/w/cpp/container/vector (search for invalidate) – Taekahn Feb 25 '22 at 22:54
  • Sure...However, the container is not "being modified", it is being "moved". What constitutes being "modified". Where is that written? This is what I am interested in. – Oliver Schönrock Feb 25 '22 at 22:59
  • @Taekahn Yeah sure... but that is not what is happening here. Have another look at the code. – Oliver Schönrock Feb 25 '22 at 22:59
  • @Taekahn I am not calling `push_back` on the `std::vector`.. The problem is that the "entire vector" (but technically only the direct members) is being relocated... – Oliver Schönrock Feb 25 '22 at 23:02
  • @Taekahn The list of "Iterator invalidation Operations" on the page you linked to is interesting. I am not "really actually" doing any of those things, am I? Which one? – Oliver Schönrock Feb 25 '22 at 23:04
  • You're ultimately taking the address of a temporary from what I see. Other than that i'd have to super dig into the code. If you're really that curious, just print out the memory addresses of everything as you go along. You'll see the truth. Or prove me wrong. If you really want to go down the rabbit hole, you can add all the special member functions and make them print out when called. – Taekahn Feb 25 '22 at 23:08
  • @Taekahn Explain please. I am taking the addres of a temporary, because I am passing an initializer list to the `thing` constructor in `main()`? No, it's taken by value, and then `std::move`d. Totally legit. I have asked a serious question. If you can't be bothered answering it. Then cool. Bye. – Oliver Schönrock Feb 25 '22 at 23:12
  • lol. Ok. I could have explained further, but "cool. bye" works for me. – Taekahn Feb 25 '22 at 23:13
  • @Taekahn I can explain. I think you haven't actually understood the problem. But started with "What did you expect".. LOL – Oliver Schönrock Feb 25 '22 at 23:14
  • 1
    I think this question hinges on what the answer to [Does moving a vector invalidate iterators?](https://stackoverflow.com/questions/11021764/does-moving-a-vector-invalidate-iterators) is. Since the most likely answer seems to be "maybe", that might be an answer to your question. – user17732522 Feb 25 '22 at 23:26
  • @user17732522 Yes, that's close. Also: "when an objects gets "relocated because its parent container is growing", what kind of operation is that? Is that even a "move construct"? Whatever it is, I would strongly suspect it compiles to memcpy in this trivial case, which is why it should "just work".... And of course it does above. I am actually beginning to believe that the code above is fine, but that something in the more complex code is triggering the dangling.. – Oliver Schönrock Feb 25 '22 at 23:30
  • 1
    @OliverSchönrock When reallocation due to increase of the vector capacity happens, the new objects in the new allocation are move-constructed from the old objects in the old allocation (assuming the move constructor is usable, in particular it should be `noexcept`). A memcpy is a possible implementation of that if the type is trivially-copyable. And after reading the cppreference page on the vector constructor I agree with the posted answer, that the program is probably valid. The answers in the question I linked might not be good or out-dated. – user17732522 Feb 25 '22 at 23:35

1 Answers1

3

What you are guaranteed is, upon moving a std::vector, no iterator, pointer or reference will be invalidated. This would apply to the vectors inside thing. See notes in https://en.cppreference.com/w/cpp/container/vector/vector

When a std::vector grows, all iterators, pointers and references to it become invalid. So if you had a reference to a thing, those would be blown away, but you do not have that, so we are good.

While a std::vector grows, it will move the previous elements to the new allocation if the contained type has a noexcept move constructor. For std::vectors, this is the case after C++17. The automatically generated move constructor of thing therefore should also qualify.

Considering these, the code you have posted is correct. As we do not see all the code, there must be an issue somewhere else that interacts with the code you have. Perhaps you have a user defined move constructor in the real code that you did not mark as noexcept, or you push_back to one of the foo vectors.

Also, the reserve call is a no-op: foos.reserve(bars.size());. bars.size() here is 0. Did you mean bars.reserve(foos.size());?

Fatih BAKIR
  • 4,569
  • 1
  • 21
  • 27
  • This is good analysis. Thank you. I tend to agree. The code as posted is actually legal (and seems to work everywhere without triggering sanitizers) .. but something else is going on. Can you confirm (as per my comment above) what kind of operation is performed to each element when a vector reallocates? Is it a call "move constructor", but only if noexcept? And otherwise what? Copy construct? The latter would certainly invalidate. – Oliver Schönrock Feb 25 '22 at 23:34
  • Ooops... yes I am going to fix that.. it's corect in the original code – Oliver Schönrock Feb 25 '22 at 23:35
  • @OliverSchönrock, yes they would be copy constructed even if there is a move constructor but it is not `noexcept`, so it's critical to mark the move constructor as `noexcept`. – Fatih BAKIR Feb 25 '22 at 23:40
  • In the real code, I have another (suspicous) member in `thing` alongside `std::vector`. It's a type from a 3rd party library, so it will take me a while to find out, but if that type does not have a noexcept move constructor, will the auto generated move constructor of `thing` then not exist or not be `noexcept` and I am therefore falling back to copy construction for THE WHOLE of `thing` and therefore a deep copy of the vectors which leave the refs dangling? – Oliver Schönrock Feb 25 '22 at 23:45
  • 2
    I just noticed that `std::vector`'s move constructor is `noexcept` only since C++17 according to cppreference. If that is correct, then before C++17 `thing` vector's `push_back` would be copying, not moving. I think implementations are allowed to add noexcept, so this would be implementation-defined technically, but I haven't checked with the standard. – user17732522 Feb 25 '22 at 23:46
  • 1
    @OliverSchönrock, right, in that case your move constructor would become non-`noexcept` and the whole thing would be copied and you would have dangling references. You can `thing(const thing&) = delete;` to prevent copy construction, since doing it is essentially UB for your program. – Fatih BAKIR Feb 25 '22 at 23:48
  • 2
    @FatihBAKIR Needs `thing(thing&&) = default;` then as well though. – user17732522 Feb 25 '22 at 23:50
  • Right. Good. This is the suspicious type : https://github.com/SFML/SFML/blob/master/include/SFML/Graphics/Shape.hpp It does not declare a move constructor. Not sure if the implicit one will be deleted? https://en.cppreference.com/w/cpp/language/move_constructor – Oliver Schönrock Feb 25 '22 at 23:51
  • @OliverSchönrock Yes, that type is non-movable, already because it doesn't declare a move constructor explicitly, but does declare a destructor. It will result in `thing` not being movable at all and so the suggestion above, deleting the copy constructor won't help either. – user17732522 Feb 25 '22 at 23:52
  • @user17732522 Well I have just become MUCH more interested in correctly applied `noexcept` ... so that's a win. Thank you both. – Oliver Schönrock Feb 25 '22 at 23:53
  • 1
    @OliverSchönrock, to add some compile time safety, you can add a `static_assert` to make sure your type is noexcept move constructible like this: https://godbolt.org/z/es4E5qqbG – Fatih BAKIR Feb 25 '22 at 23:53
  • Yes, the static asserts are a good safeguard in future. So I was basically OK, writing that code (I knew it was sailing close to the wind, but should work) .. but I didn't consider that the included 3rd party type could be wrecking move constructibility .. Will have to use something else now... perhaps store the `bars` indeces (rather than references) in each `foo` instead? – Oliver Schönrock Feb 25 '22 at 23:57
  • @FatihBAKIR Are you sure about your first paragraph? It works in all known implementations if using `std::allocator`, but it it guranteed? I found this slightly older answer which seems to disagree: https://stackoverflow.com/a/40445603/1087626 Also, there is some opinion about move construction being different to move assignment. Looks like any clear wording was never actually adopted? – Oliver Schönrock Feb 27 '22 at 19:52
  • 1
    @OliverSchönrock, I am not sure. https://eel.is/c++draft/container.gen.reqmts#container.reqmts-16 states move construction for containers other than `std::array` must be constant time. A one-by-one move of the elements would not satisfy this requirement. I do not know how this plays with fancy allocators. – Fatih BAKIR Feb 27 '22 at 21:21
  • Yes, it's not 100% clear. In practice its' fine, but the standard is not 100% clear. Especially for move assignment where your linked page says "Complexity: Linear".. it's entirely possible that my usage of `thing` may need to include move assignment too. BTW.. I decided to solve the SFML type problem by putting it behind a `std::unique_ptr` which makes `thing` nothrow move constructible again. – Oliver Schönrock Feb 27 '22 at 23:31