0

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?

Nico
  • 101
  • 9
  • 2
    `0XCDCDCDCD` means uninitialized heap memory. [https://stackoverflow.com/questions/127386/in-visual-studio-c-what-are-the-memory-allocation-representations](https://stackoverflow.com/questions/127386/in-visual-studio-c-what-are-the-memory-allocation-representations) – drescherjm May 13 '22 at 13:45
  • @drescherjm ok yeah I know that, but I want to know what is causing it. I suspect its to do with the way I'm creating the tasks, but I'm not sure. – Nico May 13 '22 at 13:48
  • I don't see the reason. You will have to use your debugger and investigate the call stack after the crash. When it breaks into the debugger use the "Stack Frame" combobox on your debug toolbar to point to your code and look at the variables and the part of the code that is running. – drescherjm May 13 '22 at 13:49
  • It looks like your threads share data but don't synchronize. – molbdnilo May 13 '22 at 13:50
  • 1
    `taskqueue.pop();` could be a problem. You unlocked the mutex before this. – drescherjm May 13 '22 at 13:51
  • btw I wonder if you know that all the workers are going to try and run the same task. – user253751 May 13 '22 at 14:00

1 Answers1

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.

Useless
  • 64,155
  • 6
  • 88
  • 132
  • fixed that and now im getting errors in code i didnt write... – Nico May 13 '22 at 16:22
  • ***now im getting errors in code i didnt write.*** Maybe there are bugs in that code. Or perhaps you still have UB in your code that causes it. – drescherjm May 13 '22 at 17:04