Possible stale pointer usage in extract_bvh()?

[tksuoran]

In this code

template <typename Node>
Bvh<Node> Bvh<Node>::extract_bvh(size_t root_id) const {
    assert(root_id != 0);

    Bvh bvh;
    bvh.nodes.emplace_back();

    std::stack<std::pair<size_t, size_t>> stack;
    stack.emplace(root_id, 0);
    while (!stack.empty()) {
        auto [src_id, dst_id] = stack.top();
        stack.pop();
        auto& src_node = nodes[src_id]; // 1.
        auto& dst_node = bvh.nodes[dst_id]; // 2.
        dst_node = src_node;
        if (src_node.is_leaf()) {
            dst_node.index.set_first_id(bvh.prim_ids.size());
            std::copy_n(
                prim_ids.begin() + src_node.index.first_id(),
                src_node.index.prim_count(),
                std::back_inserter(bvh.prim_ids));
        } else {
            dst_node.index.set_first_id(bvh.nodes.size());
            bvh.nodes.emplace_back(); // 4.
            bvh.nodes.emplace_back(); // 5.
            stack.emplace(src_node.index.first_id() + 0, dst_node.index.first_id() + 0); // 6.
            stack.emplace(src_node.index.first_id() + 1, dst_node.index.first_id() + 1);
        }
    }
    return bvh;
}

What I see happen when I debug this code (Windows 11, MSVC 17.8.8):

1. auto& src_node = nodes[src_id]; // src_node points into vector data
2. auto& dst_node = bvh.nodes[dst_id]; // dst_node points into vector data
3. ...
4. bvh.nodes.emplace_back(); // vector data may be reallocated, invalidating both src_node and dst_node
5. bvh.nodes.emplace_back(); // vector data may be reallocated, invalidating both src_node and dst_node
6. dst_node.index.first_id() // may read via stale src_node and dst_node pointers

[tksuoran]

Correction, I'm not sure if src_node is invalidated, it might be somewhere else than bvh.nodes.

[tksuoran]

Adding bvh.nodes.reserve(nodes.size()); between Bvh bvh; and bvh.nodes.emplace_back(); seems to avoid the issue, although I am not sure if that is 100% valid/correct/best fix.

[madmann91]

Hello, and good catch! This code was in fact correct before I decided to "rework" it. I will just revert that "simplification" I did earlier, with a comment to be very careful. I also don't think this modifies or invalidates src_node, since the method is const and is not supposed to modify the input BVH. I will also make that clearer with a const marker on that variable.

[madmann91]

Fixed in ed708a2