heap corruption when sharing vector between two threads

Question

In below code, the sv object in main function is created on stack but doesn't go out of scope. But I am getting occasional heap corruptions. When run under valgrind I see illegal read/write warnings

However if I create sv object on heap, I didn't see any issues.

CODE BELOW

#include <iostream>
#include <thread>
#include <functional>
#include <mutex>

mutex m;

struct SharedVec
{
public:
std::vector<int> v;
};

void check(SharedVec * sv)
{
    while (true) {
        m.lock();
        sv->v.push_back(2);
        m.unlock();
    }
}

int main()
{
    SharedVec sv; // this shared object which has vector member causing heap corruption
//although in scop
//SharedVec * sv = new SharedVec(); // if i create on heap, no issues seen

    std::thread t(check, std::bind(&sv));
    while (true)
    {
        std::vector<int> tmp;
        m.lock();
        tmp = sv.v;
        sv.v.clear();
        m.unlock(); 
    }

    t.join();  // Wait for the thread to finish
    return 0;
}

This code does not compile, therefore I guess this is not a [mcve]. — 273K, Jan 03 '23 at 03:58
`std::bind(&sv)` doesn't look right. Normally you `bind` variables to a function, but no function has been provided to `bind`. Shouldn't need this at all. The thread constructor should take care of it for you. Looks like you may have ruined the example while minimizing . — user4581301, Jan 03 '23 at 03:58
Tactical note: manually locking and unlocking a mutex is prone to failure. Use a [RAII wrapper](https://stackoverflow.com/q/2321511/4581301) like [`std::scoped_lock`](https://en.cppreference.com/w/cpp/thread/scoped_lock) to ensure the mutex is unlocked unless the code does something awesomely silly. — user4581301, Jan 03 '23 at 04:00
thanks..i know code doesn't compile but conveys the issue I am facing..do you see any issue in sharing a pointer to the stack object which has a vector with another thread ? — Youli Luo, Jan 03 '23 at 04:22
just trying to provide a stripped down code of some closed source ...to convey the idea — Youli Luo, Jan 03 '23 at 04:24
Please read the link in the first comment for how to provide a stripped down version of the source. — HolyBlackCat, Jan 03 '23 at 05:18

score 1 · Answer 1 · answered Jan 03 '23 at 04:15

1

Not really an answer since your code did not compile. But here is some feedback on your code to help you with multithreading/locking of data in a safer way.

#include <iostream>
#include <future>
#include <functional>
#include <mutex>
#include <vector>

// don't use globals, specialy not a mutex
// so put mutex in SharedVec
// and give SharedVec some threadsafe operations

struct SharedVec
{
public:
    void push_back(int value)
    {
        // never use naked lock/unlock on mutex it is not exception safe
        std::scoped_lock<std::mutex> lock{ m }; 
        v.push_back(value);
    }

    void clear()
    {
        std::scoped_lock<std::mutex> lock{ m }; 
        v.clear();
    }

    auto get_copy()
    {
        std::scoped_lock<std::mutex> lock{ m };
        return std::vector<int>{v};
    }

private:
    // keep the lock with the object that you want to be threadsafe
    std::mutex m;
    std::vector<int> v;
};

int main()
{
    // when sharing data between threads (that do not synchronize
    // with each other use a shared pointer to the shared data)
    auto sv = std::make_shared<SharedVec>();

    // async has better abstraction then thread
    // capture shared pointer by value, extending its lifetime
    auto future = std::async(std::launch::async, [sv]
    {
        while (true)
        {
            sv->push_back(2);
        }
    });

    while (true)
    {
        // by making a get_copy function it is more clear what your code is doing
        std::vector<int> tmp = sv->get_copy();
        sv->clear();
    }

    future.get(); // synchronize
    return 0;
}

answered Jan 03 '23 at 04:15

Pepijn Kramer

9,356
2
8
19

thanks..i know code doesn't compile but conveys the issue I am facing..do you see any issue in sharing a pointer to the stack object which has a vector with another thread ? – Youli Luo Jan 03 '23 at 04:22
just trying to provide a stripped down code of some closed source ...to convey the idea – Youli Luo Jan 03 '23 at 04:24
1

When people try to help you what they often want to do is to run code in a debugger and or online compiler (like https://godbolt.org/). So we often ask for a minimum reproducible example. – Pepijn Kramer Jan 03 '23 at 04:25
got it..was not aware – Youli Luo Jan 03 '23 at 04:25
any comment on my first comment? – Youli Luo Jan 03 '23 at 04:26
And no I did not spot the exact error in your code. I suspect it has something to do with bind, but I've never had to use that since I always use lambda functions when dealing with threads. – Pepijn Kramer Jan 03 '23 at 04:26
@YouliLuo There were too many errors in the code you gave to pinpoint exactly which one you were struggling with. When making an example, you want there to be one and only one error so that there is no ambiguity about the problem. It is probable that the only changes necessary were `mutex m;` to `std::mutex m;` and `std::thread t(check, std::bind(&sv));` to `std::thread t(check, &sv);`. That would compile and run. Would it do what you wanted? Dunno, so I didn't answer. – user4581301 Jan 03 '23 at 04:53
Side note: That one and only one error message could run for several pages due to templates and overloads. – user4581301 Jan 03 '23 at 04:54

heap corruption when sharing vector between two threads

1 Answers1