multithreaded C++ read access violation 0XCDCDCDCD

Question

I'm writing a simple multithreaded md5 hash cracker using a task farm, except i keep getting read access violations seemingly at random. heres the function the threads run:

void Farm::run(){
    std::vector<thread> workers;
    std::mutex queuemtx;
    int maxthreads = thread::hardware_concurrency();

    for (int i = 0; i < 1; i++) {
        workers.emplace_back(thread([&]() {
            hashcrackertask* temp;
            queuemtx.lock();
            while (!taskqueue.empty()) {
                temp = taskqueue.front();
                
                queuemtx.unlock();
                temp->run();
                taskqueue.pop();
                queuemtx.lock();
            }
            queuemtx.unlock();
        }));
    }
    for_each(workers.begin(), workers.end(), [](thread& t) {
        t.join();
        });

}

and the section of code filling the queue:

thread readfileTH([&]() {
        //loops through every line in the file
        while (std::getline(rockyou, str)) {
            while (f.queue_len() >= 20000) {
                std::this_thread::sleep_for(std::chrono::milliseconds(5));
            }
            if (str.size() > 0) {
                f.add_task(new hashcrackertask(str, passhash));
            }
        }
        });

Any clue what could be throwing the error?

Answer 1

            temp = taskqueue.front();
            queuemtx.unlock();
            temp->run();

You release the lock to avoid contention while calling run, which is a good idea, but ... you left the task on the queue so another worker can run the same task concurrently.

            taskqueue.pop();

Here, if N workers did run the same task in parallel, the first N elements of the queue will be popped without synchronization. Even if the queue only had 1 element in the first place, so hopefully it checks that.

            queuemtx.lock();

Admittedly this is a theoretical problem since the posted code ignores the value of maxthreads and only starts a single worker.

The code should just look like

            temp = taskqueue.front();
            taskqueue.pop();
            // perform all queue accesses & mutations while holding the mutex

            queuemtx.unlock();
            temp->run();
            queuemtx.lock();

---

You have data races on both the task execution (since the same task can be fetched by multiple workers while remaining on the queue) and the task pop - these are both statically bugs and should be fixed ASAP.

I'd usually suggest writing (or finding) a proper threadsafe queue instead of interleaving the queue management and synchronization with your worker logic. I found an old example here.