C++ MultiThreading Mutex Locks Segmentation Fault

Question

** This is for a college class, I am not actually trying to crack passwords ** Below is my source code, but essentially what I want to have happen is the parent process en queues passwords into an std::list<> attemptList. Then child threads grab from the front of the queue and, currently, just print out the value.

As you can see by the code below I try using std::mutex to block the attemptList when the front is being popped, but both threads are blowing past the lock at the same time and reading from the front. Then one of them segmentation faults and the program crashes.

The specific code section that is seg faulting is...

mutex.lock();
password = attemptList.front();
attemptList.pop_front();
size = attemptList.size();
std::cout << password << std::endl;
            mutex.unlock();

#include <cmath>
#include <cstdlib>
#include <cstring>
#include <string>
#include <iostream>
#include <chrono>
#include <shared_mutex>
#include <unistd.h>
#include <sys/ipc.h>
#include <mutex>
#include <sys/shm.h>
#include <sys/wait.h>
#include <thread>
#include <vector>
#include <algorithm>
#include <list>

#define MAX_LENGTH 4
#define MAX_QUEUE_SIZE 1000
#define CHARACTER_LIST "abcdefghijklmnopqrstuvwxyz"

void enqueue_passwords(const std::string& charList);
void bruteforce();
void do_join(std::thread& t);
void join_all(std::vector<std::thread>& v);

std::list<std::string> attemptList;
std::mutex mutex;
bool conclude = false;

int main(int argc, char* argv[]) {
    auto start = std::chrono::high_resolution_clock::now();

    int index;
    std::vector<std::thread> threads;
    for (index = 0; index < 2; index++) {
        threads.emplace_back(std::thread(bruteforce));
    }

    enqueue_passwords(CHARACTER_LIST);

    join_all(threads);

    auto stop = std::chrono::high_resolution_clock::now();
    auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(stop - start);
    std::cout << duration.count() << " milliseconds" << std::endl;

    return 0;
}

void bruteforce() {
    double size = 0;
    std::string password;

    while (!conclude) {
        do {
            mutex.lock();
            size = attemptList.size();
            mutex.unlock();

            if (size == 0) {
                usleep(300);
            }
        } while (size == 0);

        while(size != 0) {
            mutex.lock();
            password = attemptList.front();
            attemptList.pop_front();
            size = attemptList.size();

            std::cout << password << std::endl;
            mutex.unlock();
        }
    }
}

void enqueue_passwords(const std::string& charList) {
    const int maxLength = MAX_LENGTH;
    const int charListLength = charList.length();

    char password[MAX_LENGTH + 1];
    memset(password, '\0', MAX_LENGTH + 1);

    int index;
    int number;

    double permutations = 0;

    double count = 0;
    double passPosition = 0;
    double size = 0;

    // Calculate number of permutations possible
    for (index = 0; index < maxLength; index++) {
        permutations += charListLength * powl(charList.length(), maxLength - index - 1);
    }

    std::cout << "Permutations:  " << permutations << std::endl << std::endl;

    password[0] = charList[0];
    while (count < permutations) {
        do {
            mutex.lock();
            size = attemptList.size();
            mutex.unlock();

            if (size > MAX_QUEUE_SIZE) {
                usleep(250);
            }
        } while (size > MAX_QUEUE_SIZE);

        // Loop over current set of characters ,changing the last one
        for (index = 0; index < charListLength; index++) {
            password[int(passPosition)] = charList[index];

            // ENQUEUE HERE //
            mutex.lock();
            attemptList.push_back(std::string(password));
            mutex.unlock();
            // ENQUEUE HERE //

            if (count > permutations) {
                break;
            }
            count++;
        }

        // Iterate over remaining indexes, except for the last one
        for (number = int(passPosition); number >= 0; number--) {
            if (password[number] != charList[charListLength - 1]) {
                password[number]++;
                break;
            } else {
                if (number == 0) {
                    passPosition++;
                    for (index = 0; index < passPosition + 1; index++) {
                        password[index] = charList[0];
                    }
                    break;
                }
                password[number] = charList[0];
            }
        }
    }

    conclude = true;
}

void do_join(std::thread& t) {
    t.join();
}

void join_all(std::vector<std::thread>& v) {
    std::for_each(v.begin(), v.end(), do_join);
}

Answer 1

   do {
        mutex.lock();
        size = attemptList.size();
        mutex.unlock();

        if (size == 0) {
            usleep(300);
        }
    } while (size == 0);

Setting aside the efficiency issue, this code locks the mutex, obtains the size of the list and unlocks the mutex.

Let's say that the thread concluded that the size of the list is 1, and, therefore, the thread leaves this while loop. size is 1. The list happens to have just one value at this point. The while loop terminates.

But before it proceeds any further, one of your other threads, that does exactly the same thing, at this point, concurrently: it locks the mutex, obtains the size of the list, unlocks the mutex, determines that the size of the list is 1, and exits its while loop too, just like the first thread. Let's see what happens next, with both of our threads now at this point:

    while(size != 0) {
        mutex.lock();
        password = attemptList.front();
        attemptList.pop_front();

Ok, so the first thread now wakes up, enters this while loop, locks the mutex, grabs the only entry in the list, removes it from the list, and the list is now empty.

Your second thread now does the same thing, and blocks on its mutex.lock() call, because the first thread has it locked.

The first thread eventually unlocks the mutex. The second thread now proceeds; it locked the mutex, and it's operating under the illusion that the list is not empty, because it still think its size is 1 (because that's what it was when it locked it), in the initial while loop, and attempts to remove the first element from an empty list.

Undefined behavior.

This is the reason for your crash.

Answer 2

As already explained by Sam double mutex lock makes your code less efficient and creates race condition at the same time. Possible solution could be (inside loop for pulling data):

while( !conclude ) {
    std::string password;
    {
        std::lock_guard<std::mutex> lock( mutex );
        if( attemptList.size() ) {
           password = std::move( attemptList.front() );
           attemptList.pop_front();
        }
    }
    if( password.empty() ) {
        usleep(300);
        continue;
    }
    // work with password here
}

this way you lock mutex only once and pull data while it still locked preventing race condition and making it more efficient. Same should be done for code pushing to the queue.

This code should work but doing sleep is not the best way to synchronize threads in this case, you should use std::condition_variable instead. And your conclude variable should be checked under mutex lock or should be at least std::atomic<bool>.