Segmentation Fault when assigning value to a pointer C++

user_11913500 0
Viewed 147

When I run the following parallel code I get a segmentation fault at the assignment at row 18 (between the two prints). I don't really understand what is causing.

This is a minimal working example which describes the problem:

#include <iostream>
#include <numeric>
#include <vector>
#include <thread>


struct Worker{

    std::vector<int>* v;
    
    void f(){
            
        std::vector<int> a(20);
        std::iota(a.begin(), a.end(), 1);

        auto b = new std::vector<int>(a);
        std::cout << "Test 1" << std::endl;
        v = b;
        std::cout << "Test 2" << std::endl;
    }
};

int main(int argc, char** argv) {

    int nw = 1;
 
    std::vector<std::thread> threads(nw);
    std::vector<std::unique_ptr<Worker>> W;

    for(int i = 0; i < nw; i++){
        W.push_back(std::make_unique<Worker>());
        threads[i] = std::thread([&]() { W[i]->f(); } );

        // Pinning threads to cores
        cpu_set_t cpuset;
        CPU_ZERO(&cpuset);
        CPU_SET(i, &cpuset);
        pthread_setaffinity_np(threads[i].native_handle(), sizeof(cpu_set_t), &cpuset);
    }

    for (int i = 0; i < nw; i++) {
        threads[i].join();
        std::cout << (*(W[i]->v))[0] << std::endl;
    }
    
}

It seems that compiling it with -fsanitize=address the code works fine but I get worst performances. How can I make it work?

pointers parallel-processing segmentation-fault c++
3 Answers

std::vector is not thread-safe. None of the containers in the C++ library are thread safe.

threads[i] = std::thread([&]() { W[i]->f(); } );

The new execution thread captures the vector by reference and accesses it.

W.push_back(std::make_unique<Worker>());

The original execution thread continuously modifies the vector here, without synchronizing access to the W vector with any of the new execution threads. Any push_back may invalidate the existing contents of the vector in order to reallocate it, and if a different execution thread attempts to get W[i] at the same time, while it's being reallocated, hillarity ensues.

This is undefined behavior.

You must either synchronize access to the vector using a mutex, or make sure that the vector will never be reallocated, using any number of known techniques. A sufficiently-large reserve(), in advance, should do the trick.

Additionally, it's been pointed out that i is also captured by reference, so by the time each new execution thread starts, its value could be anything.

user_39433120

In addition to the vector synchronization problem mentioned by Sam, there is another problem.

This line:

threads[i] = std::thread([&]() { W[i]->f(); } );

captures i by reference. There is a good chance that i goes out of scope (and is destroyed) before the thread starts running. The statement W[i]->f(); is likely to read an invalid value of i which is negative or too large. Note that before i goes out of scope, the last value written to it is nw, so if even if the memory that previously contained i is still accessible, it's likely to have the value nw which is too large.

You could fix this problem by capturing i by value:

threads[i] = std::thread([&W, i]() { W[i]->f(); } );
//                        ^^^^^
// captures W by reference, and i by value
user_1061040

As noted by others, the capture is the problem.

I've added the i parameter to the f() call:

    void f(int i){
            
        std::vector<int> a(20);
        std::iota(a.begin(), a.end(), 1);

        auto b = new std::vector<int>(a);
        std::cout << "Test 1 " << i << std::endl;
        v = b;
        std::cout << "Test 2 " << v->size() << std::endl;
    }

and the output: Test 1 1

The call to f works however but it is called without a valid Worker instance and when you assign to v it is surely at a wrong memory.

user_9801290
Related