(C) Using mutex in multithreaded client and server

Question

I need help getting mutex to work the way I want. I am making a simple bank system with a server and multiple clients.

The server has two threads. One thread listens for connections. The second thread is created when a client connects to the server.

The client has 3 threads. There is the main thread that makes the other two. The second thread just receives messages from the server and outputs them. The third thread only takes input and sends them to the server.

I am trying to use mutex on a customer session after they login. For example, if one client logs into the server with their account, I want to lock that account session with mutex. So if any other client tries to login to the same account they would have to wait on the client that's already logged into it.

The above scenario works perfectly with my code. The problem I have currently is with multiple accounts. Lets say client1 logs into their account and starts doing stuff. Then client2 tries to login into an account different from client1. It will not let client2 inside because of the lock from mutex on client1's account.

I want to make it so if client1 logs into their account, no other client can login to client1's account until client1 is done. Though, other clients should be able to login to other accounts and have separate locks on them.

Here is my server code (lock inside curr_acc()):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <ctype.h>
#include <unistd.h>
#include <pthread.h>

// Mutex var
pthread_mutex_t lock;

int main(int argc, char *argv[])
{
    pthread_mutex_init(&lock, NULL);
    server_listen();
    pthread_mutex_destroy(&lock);
    return 0;
}

int server_listen()
{
    int socket_desc, client_sock, c, *new_sock;
    int portno = 8888;
    struct sockaddr_in server, client;

    socket_desc = socket(AF_INET, SOCK_STREAM, 0);

    server.sin_family = AF_INET;
    server.sin_addr.s_addr = INADDR_ANY;
    server.sin_port = htons(portno);

    bind(socket_desc, (struct sockaddr *) &server, sizeof(server))
    listen(socket_desc, 5);
    c = sizeof(struct sockaddr_in);

    // Accept connection
    while((client_sock = accept(socket_desc, (struct sockaddr *) &client, (socklen_t *) &c)))
    {
        // Create new thread and sock
        pthread_t sniffer_thread;
        new_sock = malloc(1);
        *new_sock = client_sock;

        pthread_create(&sniffer_thread, NULL, handle_client, (void *) new_sock)
    }
}

void *handle_client(void *new_sock)
{
    // Convert void
    int sock = *((int *) new_sock);

    // Prepare to receive and send
    int read_size, index;
    char *msg = malloc(10);

    while(recv(sock, msg, 10, 0) > 0)
    {
        if(strcmp(msg, "open") == 0)
            new_acc(sock);
        else if(strcmp(msg, "start") == 0)
            curr_acc(sock);
        else
            write(sock, "Invalid input", 25);

        memset(msg, 0, sizeof(msg));
    }

    free(new_sock);
    free(msg);

    // Exit the thread
    pthread_exit(0);
}

void curr_acc(int sock)
{
    int lock_ret;

    // Get account name here and check if it's in array of accounts.
    // Then try below

    // Try and lock the thread then go inside
    lock_ret = pthread_mutex_trylock(&lock);
    if(!lock_ret)
    {
        // Show customer menu
        write(sock, "\n Welcome!", 20);
        cus_menu(sock);

        // Unlock the thread
        lock_ret = pthread_mutex_unlock(&lock);
    }
    else
    {
        printf("Cannot open account");
        write(sock, "Cannot open account. Try again or wait...", 50);
    }
}

This is a stripped down version of my server, but it should be enough to understand what I'm doing. If you want more, let me know.

My client sends a char pointer to the server to check the input. If you need to see the client code, let me know as well. But it is pretty straight forward as described above.

Any help would be appreciated. I really want to understand mutex better. Thank you!

Answer 1

What bank? I'm a bit concerned some of your code might be passing around my money. For example:

char *msg = malloc(10);

while(recv(sock, msg, 10, 0) > 0)
{
    if(strcmp(msg, "open") == 0)

Obvious security hole - you aren't guaranteeing that msg is null terminated before doing the strcmp.

And this line:

write(sock, "Invalid input", 25);

Is just plain wrong. How is the client supposed to interpret the 11 garbage characters that get sent by that line? If you get invalid protocol from a client, just terminate the connection.

And oh no...

    new_sock = malloc(1);
    *new_sock = client_sock;

Your client_sock value is writing on top of my bank balance because you should be saying: new_sock = malloc(sizeof(socket_t))

Seriously, if your code is running in a bank and manages customer data, please get it reviewed by as many experienced developers and security expert before deploying it.

Now, to answer your original question.

There's lots of ways to do this. A database would be better suited for this, but I suspect what you need is a mapping between client accounts and a corresponding mutex in addition to a mapping between sockets and accounts.

I actually have to take off, but I will finish writing a proper answer for you later tonight that addresses your original problem of managing simultaneous access. To be continued...

Update

Ok, now to answer your original question.

First, it's not a good idea for a socket thread to infinitely block on a mutex that's dependent on another client's behavior. I could write a rouge client continually makes connections and requests transactions for a the same client id. That would cause your server to continually spin up threads that are blocked on the same mutex. A better approach would be to consider "logging out" the older client connection such that the new connection can proceed. Or, if you want to be really robust, consider the possibility of allowing multiple clients for the same account to be logged in - and then have a database (or object model) that supports thread safe transaction operations.

Now, if you want to have a mutex for each client account, I would suggest this pattern. Start by having this struct defined:

struct ClientAccount
{
    int account_number; // could be any type, I chose "int" for brevity
    pthread_mutex_t mutex; // the mutex that guards access to this client account  
};

In our simple example, I'm going to stake the claim that ClientAccount instances are always referenced by pointer and that once a ClientAccount instance is created, it never goes away. That will make the following example a bit simpler.

Then have a global table of ClientAccount instances. Could be an array or linked list, but preferably a hash table that can grow. But regardless, this table that holds all ClientAccount instances has it's own mutex. Keeping it simple, it could look like this:

struct ClientAccountTable
{
    ClientAccount* accounts[MAX_NUMBER_OF_ACCOUNTS]; // an array of pointers - not an array of instances!
    int accounts_size; // number of items in accounts
};

And then a global instance with a global mutex;

ClientAccountTable g_account_table;
pthread_mutex_t g_table_mutex; // guards access to g_account_table;

You'll have to define the operations on this table, but each would acquire the table's mutex before doing something on the table. For example:

ClientAccount* find_or_create_account(int account_number)
{
    ClientAccount* account = NULL;

    // acquire the global lock before accessing the global table
    pthread_mutex_lock(&g_table_mutex);

    for (int i = 0; i < g_account_table; i++)
    {
        if (account_number == g_account_table.accounts[i].account_number)
        {
             account = g_account_table.accounts[i];
        }
    }

    if (account == NULL)
    {
       // not found in the table, so create a new one
       pAccount = malloc(sizeof(ClientAccount));
       g_account_table.accounts[g_account_table.accounts_size] = account;
       g_account_table.accounts_size++;
    }

    // release the lock
    pthread_mutex_unlock(&g_table_mutex);
    return account;
}

Then each one of your threads on the server could do this after the client identifies which account it wants to use:

 void* handle_client(void* )
 {
    // socket code to read client's account number
    ClientAccount* account = find_or_create_account(account_number);

    // lock the mutex for this client account
    pthread_mutex_lock(&account->mutex);

    // not shown:  socket code and the code to do transactions for this account


    // unlock the account's mutex after this client session is done
    pthread_mutex_unlock(&account->mutex);

 }

You could also experiment with using pthread_mutex_trylock instead of pthread_mutex_lock. Such that if the lock acquisition fails, you could tell the remote client that the server is servicing another client.