Unix chat server making communication between terminals possible

Question

This Server handles clients communicating with each other. Because I was not willing to actually give every client a name, they are named by their socket's file descriptor. This is my first application which actually is able to communicate using the INET. It is also my first really multithreaded application.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <pthread.h>
#include <stdbool.h>

#define BUFLEN 255
#define MAX_CONNECTIONS 128
#define MAX_NAME_LENGTH 30

void delete_socket(int socketid);
void* job_read(void*);
void* job_write(void*);

//Global Variables
FILE* plogfile;
int socket_ids[MAX_CONNECTIONS];
bool endprogramm = false;
int open_cnncts = 0;
pthread_mutex_t socketids_changingMutex;

void error(const char* msg){
    perror(msg);
    exit(1);
}

int main(int argc, char* argv[]) {
    printf("Server started...\n");
    if(argc < 2){
        fprintf(stderr, "You must provide a port number");
        exit(EXIT_FAILURE);
    }
    if(argc == 3){
        plogfile = fopen(argv[2], "w");
    } else {
        plogfile = fopen("logfile.txt", "w");
    }
    stderr = plogfile;

    int sockfd;
    uint16_t portnum;

    //Create nmutthread
    if(pthread_mutex_init(&socketids_changingMutex, NULL) < 0){
        error("Could not initialize Mutex");
    }
    //Initialzing threads and create writethread
    pthread_t writethread;
    pthread_create(&writethread, NULL, job_write, NULL);

    //Setup for connections
    struct sockaddr_in serv_add;
    struct sockaddr_in cli_adr;
    socklen_t clilen;
    clilen = sizeof(cli_adr);
    bzero((char*)&serv_add, sizeof(struct sockaddr_in));

    portnum = (uint16_t)atoi(argv[1]);
    serv_add.sin_family = AF_INET;
    serv_add.sin_addr.s_addr = INADDR_ANY;
    serv_add.sin_port = htons(portnum);

    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if(sockfd < 0){
        error("Error opening socket.");
    }

    if(bind(sockfd, (struct sockaddr*) (&serv_add), sizeof(serv_add)) < 0){
        error("Binding failed.");
    }
    fprintf(plogfile,"Listening....");
    listen(sockfd, MAX_CONNECTIONS);

    for(open_cnncts = 0; (!endprogramm); /*mutex needs to be set*/ ){
        socket_ids[open_cnncts] = accept(sockfd, (struct sockaddr*) &cli_adr, &clilen);
        pthread_mutex_lock(&socketids_changingMutex);
        fprintf(plogfile,"Client connected.\n");
        pthread_t thread;
        pthread_create(&thread , NULL, job_read, (void*)&socket_ids[open_cnncts]);
        open_cnncts++;
        pthread_mutex_unlock(&socketids_changingMutex);
    }

    endprogramm = true;
    close(sockfd);

    pthread_join(writethread, NULL);
    pthread_mutex_destroy(&socketids_changingMutex);
    return EXIT_SUCCESS;
}

void* job_read(void * p){
    int* socketp = (int*)p;
    int newsockfd = (*socketp);
    ssize_t n; //Error catching variable
    ssize_t m; //Error catching variable
    char buffer[BUFLEN];
    char name[MAX_NAME_LENGTH];
    sprintf(name, "Client %d: ", newsockfd);

    while(!endprogramm){
        bzero(buffer, BUFLEN);
        n = read(newsockfd, buffer, BUFLEN);

        if(n<0){
            printf("Buffer: %s", buffer);
            error("Reading Failed");
        }
        if(n == 0){
            delete_socket(newsockfd);
            pthread_exit(NULL);
        }

        pthread_mutex_lock(&socketids_changingMutex);
        for(int i = 0; i < open_cnncts; i++){
            if(socket_ids[i] == newsockfd){
                continue;
            }
            m = write(socket_ids[i], name, strlen(name));
            n = write(socket_ids[i], buffer, strlen(buffer));
            if(n < 0 | m < 0){
                error("Writing failed");
            }
        }
        pthread_mutex_unlock(&socketids_changingMutex);
        printf("%s%s", name, buffer);
    }
    delete_socket(newsockfd);
    pthread_exit( NULL );
}

void* job_write(void* args){
    (void)args;
    fprintf(plogfile, "Started writing thread...\n");
    ssize_t n; //Error catching variable
    ssize_t m; //Error catching variable
    char buffer[BUFLEN];
    char* name = "Server: \0";

    while(!endprogramm) {
        fgets(buffer, BUFLEN, stdin);

        pthread_mutex_lock(&socketids_changingMutex);
        for(int i = 0; i < open_cnncts; i++){
            m = write(socket_ids[i], name,  strlen(name));
            n = write(socket_ids[i], buffer, strlen(buffer));
            if(n < 0 | m < 0){
                printf("Writing failed");
            }
        }
        pthread_mutex_unlock(&socketids_changingMutex);
        if(strcmp("Bye\n", buffer) == 0){
            exit(EXIT_SUCCESS);
        }
    }
    endprogramm = true;
    pthread_exit( NULL );
}

void delete_socket(int socketid){
    bool found = false;
    for(int i = 0; i < open_cnncts; i++){
        if(found){
            socket_ids[i-1] = socket_ids[i];
        }
        if(socket_ids[i] == socketid){
            close(socketid);
            found = true;
        }
    }
    if(found){
        open_cnncts--;
    }
}

Obviously, this code needs improvement, so just feel free to criticize everything criticizable, especially about array-, thread-, and mutex-handling.

Answer 1

Portability
The code uses pthread which is a POSIX (Portable Operating System Interface) structure. If the code uses POSIX for one thing, it might be better to use POSIX for all things to remain POSIX compliant. This means that instead of using bzero((char*)&serv_add, sizeof(struct sockaddr_in)); to zero out the serv_add variable it might be better use memset(&serv_add, 0, sizeof(serv_add));. The function memset() is part of the POSIX standard, the bzero() is not.

There may be other non-POSIX functions that should be changed as well.

Redirecting Output
Most operating systems provide mechanisms for redirecting output and errors, it might be better to create a shell script to start this program that redirects errors to a log file that changing the value of the system variable stderr. One of the values of this is that the person starting this chat server can name the log file and put it in a certain location on the system. This would also simplify the first part of the code in main().

It's also not clear what side affects there are when the variable stderr is over written.

Avoid Global Variables When Possible
Global variables make it much harder to write, read and debug code, they can be considered a maintenance nightmare, especially when code is modularised into multiple files, it is better to pass parameters whenever possible.

It is possible to pass arguments to a function used when creating a thread using pthread_create() by passing a pointer to a struct in the 4th parameter. The code already uses this mechanism in job_read() but both job_read() and job_write() could receive pointers to variables which are currently global.

struct JOB_INFO {
    int socket_ids[MAX_CONNECTIONS];
    bool endprogram;
    int open_cnncts;
    pthread_mutex_t *socketids_changingMutex;
};

int main(int argc, char* argv[])
{
    struct JOB_INFO job_info;
    job_info.endprogram = false;
    job_info.open_cnncts = 0;
    job_info.socketids_changingMutex = ...;

...

    pthread_create(&writethread, NULL, job_write, &job_info);


void* job_write(void* args) {
    struct JOB_INFO *job_info = (struct JOB_INFO*) args;
     ...
}

Complexity
The main() function is overly complex, it might be better to break it up into sub-functions.

• Parse arguments
• Set up for connections
• Open connections loop

return over exit in main
There is no reason to call exit() in main(), return EXIT_FAILURE; will achieve the same result, exit(int status) is only necessary when trying to exit the program from sub functions.

Answer 2

regarding:

if(n<0){
        printf("Buffer: %s", buffer);
        error("Reading Failed");
    }

the call to printf() will change the value in errno. The result is the call to perror() in function: error() will print the wrong message.

---

regarding:

void* job_read(void * p){
    int* socketp = (int*)p;
    int newsockfd = (*socketp);

the variable socketp is not used elsewhere in this function. Suggest:

void* job_read(void * p){
int newsockfd = *(int*)p;

Also, the correct type for a socket is sock_t, not int

---

regarding:

#define MAX_CONNECTIONS 128

It would be advisable to check your OS to assure it will allow 128 simultaneous connections. Many 'user' OS versions will not allow anywhere near that many simultaneous connections.

---

regarding:

m = write(socket_ids[i], name, strlen(name));
n = write(socket_ids[i], buffer, strlen(buffer));
if(n < 0 | m < 0){
    error("Writing failed");
}

If the first call to write() fails, it will have set errno indicating the reason the system thinks the error occurred. However, the second call to write() will overlay the value in errno, so the call to error()/perror() will output the wrong message.

suggest:

if( (m = write(socket_ids[i], name, strlen(name)) ) < 0 )
{
    error( "write of user name failed" );
}

if( (n = write(socket_ids[i], buffer, strlen(buffer)) ) < 0)
{
    error("Writing of user message failed");
}

Answer 3

• pthread_create returns value which is needed to be checked too; if your system runs out of resources, the new thread might not start.
• You are listening on all IPs available; considering that there is no visible authentication, this opens possibility for unauthorized access, impersonation etc. Consider listening on localhost only (enough within one server);
• The socket_ids is modified in delete_socket while the mutex is not locked. This creates a race condition if for example a new connection is accepted and at the same time the existing one is closed. Access to socket_ids has to be guarded by mutex.
• Same in access call - you're modifying socket_ids without holding mutex.
• Unix terminals generally send CRLF (\r\n) as linefeed, so the check for "Bye\n" will not work for those.

Answer 4

There is a lot of code duplication between job_read and job_write, and all the work is being done from a lot of threads inside a big mutex that will only allow one thread ever to actually do something. Servicing all clients from a single thread using select(2) would be more efficient.

The most annoying failure mode for this setup is that if one of the clients does not acknowledge data and the write buffer gets full, the writer thread will be stuck while holding the mutex, so all other threads that need to access the global data will get stuck as well.