-1

I have written a Server, which is supposed to be a Terminal chat application (exercise-only). To read incoming messages, I created a thread for each Client, which only purpose is to read the incoming text...

However, this function seems to do the following: If the terminals input/output is empty, the Server prints down: "Client [message]" without sending it back to other clients. If the terminal input/output, however, is not empty, it sends back the data, but does not print: "Client [message]". I cannot fully grasp this mistake. Furthermore the Server exits when a Clients disconnects.

This is the Server:

#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>

#define BUFLEN 255
#define MAX_CONNECTIONS 128
#define TRUE 1
#define FALSE 0

void* job_read(void * p);
void* job_write(void*);

//Global Variables
FILE* plogfile;
int socket_ids[MAX_CONNECTIONS];
char endprogramm = FALSE;
int open_cnncts = 0;
pthread_mutex_t mutex;

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

int main(int argc, char* argv[]) {
    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, portnum;

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

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

    portnum = 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.");
    }

    //Bind listening
    if(bind(sockfd, (struct sockaddr*) (&serv_add), sizeof(serv_add)) < 0){
        error("Binding failed.");
    }
    for(open_cnncts = 0; (!endprogramm) & (open_cnncts < MAX_CONNECTIONS); open_cnncts++){
        fprintf(plogfile,"Listening....");
        listen(sockfd, MAX_CONNECTIONS);
        socket_ids[open_cnncts] = accept(sockfd, (struct sockaddr*) &cli_adr, &clilen);
        fprintf(plogfile,"Client connected.\n");
        pthread_create(&readthreads[open_cnncts] , NULL, job_read, (void*)&socket_ids[open_cnncts]);
    }
    endprogramm = TRUE;
    close(sockfd);
    for(; open_cnncts != 0; open_cnncts--){
        close(socket_ids[open_cnncts]);
        pthread_join(readthreads[open_cnncts], NULL);
    }
    pthread_join(writethread, NULL);
    pthread_mutex_destroy(&mutex);
    return 0;

}

void* job_read(void * p){
    int* socketp = (int*)p;
    int newsockfd = (*socketp);
    size_t n;
    char buffer[BUFLEN];
    while(!endprogramm){
        bzero(buffer, BUFLEN);
        n = read(newsockfd, buffer, BUFLEN);
        if(n){
            error("Reading Failed");
        }
        pthread_mutex_lock(&mutex);
        for(int i = 0; i < open_cnncts; i++){
            if(socket_ids[i] == newsockfd)continue;
            n = write(socket_ids[i], buffer, strlen(buffer));
            if(n < 0){
                error("Writing failed");
            }
        }
        pthread_mutex_unlock(&mutex);
        printf("Client: %s\n", buffer);
    }
    return NULL;
}

void* job_write(void* args){
    fprintf(plogfile, "Started writing thread...\n");
    size_t n;
    char buffer[BUFLEN];
    while(!endprogramm) {
        bzero(buffer, BUFLEN);
        fgets(buffer, BUFLEN, stdin);

        pthread_mutex_lock(&mutex);
        for(int i = 0; i < open_cnncts; i++){
            n = write(socket_ids[i], buffer, strlen(buffer));
            if(n < 0){
                error("Writing failed");
            }
        }
        pthread_mutex_unlock(&mutex);
        if(strcmp("Bye", buffer) == 0){
            break;
        }
    }
    endprogramm = TRUE;
    return NULL;
}

I assume the bug is somewhere here:

void* job_write(void* args){
fprintf(plogfile, "Started writing thread...\n");
size_t n;
char buffer[BUFLEN];
while(!endprogramm) {
    bzero(buffer, BUFLEN);
    fgets(buffer, BUFLEN, stdin);

    pthread_mutex_lock(&mutex);
    for(int i = 0; i < open_cnncts; i++){
        n = write(socket_ids[i], buffer, strlen(buffer));
        if(n < 0){
            error("Writing failed");
        }
    }
    pthread_mutex_unlock(&mutex);
    if(strcmp("Bye", buffer) == 0){
        break;
    }
}
endprogramm = TRUE;
return NULL;
}

Here the Inputs to the Terminal:

Terminal 1:
./Server 9999 
...
Client: "Hello"
...
...


Terminal 2:
./Client 127.0.0.1 9999
Hello
...
Hello
Server: Hello

If you want to reproduce the bug, this is the clients' code:

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

#define BUFLEN 255
#define TRUE 1
#define FALSE 0

char endprogram = 0;
int sockfd;


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

void* job_read(void* p){
    char buffer[BUFLEN];
    while(!endprogram){
        bzero(buffer, BUFLEN);
        size_t n = read(sockfd, buffer, (BUFLEN));
        if(n < 0){
            error("Error on reading");
        }
        printf("Server: %s", buffer);
        int i = strncmp("Bye", buffer, 3);
        if(i == 0){
            endprogram = TRUE;
            return NULL;
        }
    }
    return NULL;
}

int main(int argc, const char * argv[]) {
    pthread_t readt;

    int sockfd, portnum;
    struct sockaddr_in serveraddr;
    struct hostent* server;

    if(argc < 3){
        perror("You shall provide a port and a ip adress");
    }
    portnum = atoi(argv[2]);
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if(sockfd < 0){
        error("Error opening socket");
    }

    server = gethostbyname(argv[1]);
    if(!server){
        error("No such host");
    }

    bzero((char*)&serveraddr, sizeof(serveraddr));
    serveraddr.sin_family = AF_INET;
    bcopy((char *)server->h_addr, (char *)&serveraddr.sin_addr.s_addr, sizeof(server->h_length));
    serveraddr.sin_port = htons(portnum);

    if(connect(sockfd, (struct sockaddr *)&serveraddr, sizeof(serveraddr))<0){
        error("Connection failed");
    }

    pthread_create(&readt, NULL, &job_read, NULL);

    size_t n;
    char buffer[BUFLEN];
    while(!endprogram){
        bzero(buffer, BUFLEN);
        fgets(buffer, BUFLEN, stdin);
        n = write(sockfd, buffer, strlen(buffer));
        if(n < 0){
            error("Error on writing");
        }
        n = strcmp(buffer, "Bye");
        if(n == 0){
            endprogram = TRUE;
        }
    }
    pthread_join(readt, NULL);
    close(sockfd);
    return 0;
}

EDIT: The server is supposed to print the client's message and write it back to all other clients...

EDIT EDIT:

I hope this code is better for you to compile and better to read:

#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


void* job_read(void * p);
void* job_write(void*);

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

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

int main(int argc, char* argv[]) {
    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(&mutex, NULL)<0){
        error("Could not initialize Mutex");
    }
    //Initialzing threads and create writethread
    pthread_t readthreads[MAX_CONNECTIONS];
    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.");
    }

    //Bind listening
    if(bind(sockfd, (struct sockaddr*) (&serv_add), sizeof(serv_add)) < 0){
        error("Binding failed.");
    }
    for(open_cnncts = 0; (!endprogramm) & (open_cnncts < MAX_CONNECTIONS); open_cnncts++){
        fprintf(plogfile,"Listening....");
        listen(sockfd, MAX_CONNECTIONS);
        socket_ids[open_cnncts] = accept(sockfd, (struct sockaddr*) &cli_adr, &clilen);
        fprintf(plogfile,"Client connected.\n");
        pthread_create(&readthreads[open_cnncts] , NULL, job_read, (void*)&socket_ids[open_cnncts]);
    }
    endprogramm = true;
    close(sockfd);
    for(; open_cnncts != 0; open_cnncts--){
        close(socket_ids[open_cnncts]);
        pthread_join(readthreads[open_cnncts], NULL);
    }
    pthread_join(writethread, NULL);
    pthread_mutex_destroy(&mutex);
    return 0;

}

void* job_read(void * p){
    int* socketp = (int*)p;
    int newsockfd = (*socketp);
    ssize_t n;
    char buffer[BUFLEN];
    while(!endprogramm){
        bzero(buffer, BUFLEN);
        n = read(newsockfd, buffer, BUFLEN);
        if(n){
            error("Reading Failed");
        }
        pthread_mutex_lock(&mutex);
        for(int i = 0; i < open_cnncts; i++){
            if(socket_ids[i] == newsockfd){
                continue;
            }
            n = write(socket_ids[i], buffer, strlen(buffer));
            if(n < 0){
                error("Writing failed");
            }
        }
        pthread_mutex_unlock(&mutex);
        printf("Client: %s\n", buffer);
    }
    pthread_exit( NULL );
}

void* job_write(void* args){
    (void)args;
    fprintf(plogfile, "Started writing thread...\n");
    ssize_t n;
    char buffer[BUFLEN];
    while(!endprogramm) {
        fgets(buffer, BUFLEN, stdin);

        pthread_mutex_lock(&mutex);
        for(int i = 0; i < open_cnncts; i++){
            n = write(socket_ids[i], buffer, strlen(buffer));
            if(n < 0){
                error("Writing failed");
            }
        }
        pthread_mutex_unlock(&mutex);
        if(strcmp("Bye", buffer) == 0){
            break;
        }
    }
    endprogramm = true;
    pthread_exit( NULL );
}

Client:

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

#define BUFLEN 255


bool endprogram = false;
int sockfd;


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

void* job_read(void* p){
    (void)p;
    char buffer[BUFLEN];
    while(!endprogram){
        bzero(buffer, BUFLEN);
        size_t n = read(sockfd, buffer, (BUFLEN));
        if(n < 0){
            error("Error on reading");
        }
        printf("Server: %s", buffer);
        int i = strncmp("Bye", buffer, 3);
        if(i == 0){
            endprogram = true;
            return NULL;
        }
    }
    return NULL;
}

int main(int argc, const char * argv[]) {
    pthread_t readt;

    int sockfd;
    int16_t portnum;
    struct sockaddr_in serveraddr;
    struct hostent* server;

    if(argc < 3){
        perror("You shall provide a port and a ip adress");
    }
    portnum = atoi(argv[2]);
    sockfd = socket(AF_INET, SOCK_STREAM, 0);
    if(sockfd < 0){
        error("Error opening socket");
    }

    server = gethostbyname(argv[1]);
    if(!server){
        error("No such host");
    }

    bzero((char*)&serveraddr, sizeof(serveraddr));
    serveraddr.sin_family = AF_INET;
    bcopy((char *)server->h_addr, (char *)&serveraddr.sin_addr.s_addr, sizeof(server->h_length));
    serveraddr.sin_port = htons(portnum);

    if(connect(sockfd, (struct sockaddr *)&serveraddr, sizeof(serveraddr))<0){
        error("Connection failed");
    }

    pthread_create(&readt, NULL, &job_read, NULL);

    ssize_t n;
    char buffer[BUFLEN];
    while(!endprogram){
        fgets(buffer, BUFLEN, stdin);
        n = write(sockfd, buffer, strlen(buffer));
        if(n < 0){
            error("Error on writing");
        }
        n = strcmp(buffer, "Bye");
        if(n == 0){
            endprogram = false;
        }
    }
    pthread_join(readt, NULL);
    close(sockfd);
    return 0;
}

EDIT EDIT EDIT:

The Error I get is in the readings thread: "Error Reading: Undefined Error". If I start the server using xCode it appears that the Server crashes many times without writing the console.

Known Bugs:

  • If the client disconnects what happens to the threads and the filedescriptor?
Niclas Schwalbe
  • 23
  • 7
  • 1
    OT: regarding: `#define TRUE 1 #define FALSE 0` The C standard library contains the header file: `stdbool.h` which exposes: `true` `false` and `bool` – user3629249 Jul 14 '19 at 16:13
  • 1
    regarding: `size_t n; ... n = read(newsockfd, buffer, BUFLEN); if(n){ error("Reading Failed"); }` the value in `n` can be a positive number (the number of bytes read --or-- 0 (the other end of the connection closed the connection --or-- <0 (some error occurred) Suggest code similar to: `ssize_t n; ... n = read(newsockfd, buffer, BUFLEN); if(n < 0){ error("Reading Failed"); } – user3629249 Jul 14 '19 at 16:17
  • OT: regarding: `if(socket_ids[i] == newsockfd)continue;` For ease of readability and understanding:, Please follow the axiom: *only one statement per line and (at most) one variable declaration per statement.* – user3629249 Jul 14 '19 at 16:21
  • regarding: `size_t n;` The values returned from `write()` and/or `read()` can be <0, ==0, >0. So the variable `n` must be of type `ssize_t` not `size_t` – user3629249 Jul 14 '19 at 16:23
  • I will fix it, thank you! – Niclas Schwalbe Jul 14 '19 at 16:23
  • OT: regarding: `void* job_write(void* args)` since the body of the thread function does not use the parameter: `args`, the first line in the body should be: `(void)args;` – user3629249 Jul 14 '19 at 16:35
  • regarding: `int sockfd, portnum;` The variable: `portnum` is being passed to `htons()` which is expecting a `uint16_t` value, not a `int32_t` value. Suggest: `int sockfd; uint16_t portnum;` – user3629249 Jul 14 '19 at 16:36
  • OT: regarding: `bzero(buffer, BUFLEN); fgets(buffer, BUFLEN, stdin);` Since the function: `fgets()` will NUL terminate the buffer, there is no need to call `bzero()` – user3629249 Jul 14 '19 at 16:39
  • OT: regarding: `return NULL;` at the end of a thread function. This is acceptable, however, `pthread_exit( NULL );` is better – user3629249 Jul 14 '19 at 16:41
  • Thank you for your critique. I am still searching a way to make my compiler notice the warnings. It seems that Clang just gives a shit. https://s18.directupload.net/images/190714/3hww4nk8.png – Niclas Schwalbe Jul 14 '19 at 16:45
  • regarding: `n = read(newsockfd, buffer, BUFLEN); if(n < 0) { error("Reading Failed"); }` This will exit the server without letting the user(s) know why. Suggest sending a message to all the connected users warning them of the 'abrupt' exit. then exiting – user3629249 Jul 14 '19 at 16:45
  • how to enable/disable compiler warnings in xCode: *Open your project in XCode, then right click on your target in the Targets folder. Select "Get Info" form the drop down menu and then scroll down to the section for the compiler warnings (GCC 4.0 Warnings on my box). Here you can disable the checkboxes for the various warnings you have active.* [compiler Warnings](https://stackoverflow.com/questions/4238597/disabling-and-enabling-warning-in-xcode) – user3629249 Jul 14 '19 at 16:55
  • To the ´job_write´ function, what is the problem of not using the argument? What does (void)args do? – Niclas Schwalbe Jul 14 '19 at 16:59
  • Not using the argument (in this case to `job_write()`) causes the compiler to output a warning about a 'unused parameter'. The use of: `(void)args` kills the warning without doing anything else to your code. Remember, there are a few important objectives to every source code file. 1) human readability 2) cleanly compiles and the most important one 3) performs the desired functionality – user3629249 Jul 14 '19 at 17:05
  • Thank you! I fixed it - as far as Clang is concerned... – Niclas Schwalbe Jul 14 '19 at 17:16
  • Do you have a idea what could cause the bug? I am new to network programming. – Niclas Schwalbe Jul 15 '19 at 12:21
  • This looks like far too much code for a relatively simple bug. When you write code, it behooves you to start with something small and simple, add complexity a little at a time and test at every step. *Never add to code that doesn't work.* And when you go hunting for a bug, remove complexity until you get down to a [minimal complete example](https://stackoverflow.com/help/mcve) before you post here. – Beta Jul 15 '19 at 13:18
  • The problem is: I cannot not minimize the code... I do not know where bad things start happening. I need to set all these things up - without the possibility the validate them - before I can go on. – Niclas Schwalbe Jul 15 '19 at 13:27
  • regarding: `listen(sockfd, MAX_CONNECTIONS);` This should only be done once, not once for each connection. BTW: both the `listen()` and the `accept()` functions can fail. Therefore their returned values should always be checked for an error. – user3629249 Jul 15 '19 at 15:50
  • But what when a new Client enters after 10min? – Niclas Schwalbe Jul 15 '19 at 17:42
  • regarding: *If the client disconnects what happens to the threads and the filedescriptor?* when the client disconnects, the server, when it calls `read()` for that client. The call to `read()` will return 0; When that happens, the server needs to close the socket for that (individual) client, the exit that (individual) thread – user3629249 Jul 15 '19 at 22:19

2 Answers2

1

The posted code does not cleanly compile!.

The following lists the main problems.

When compiling, always enable the warnings, then fix those warnings.

For gcc, at a minimum use: -Wall -Wextra -Wconversion -pedantic -std=gnu11

Note: other compilers use different options to perform the same actions.

untitled.c: In function ‘main’:
untitled.c:61:31: warning: conversion to ‘uint16_t {aka short unsigned int}’ from ‘int’ may alter its value [-Wconversion]
     serv_add.sin_port = htons(portnum);
                               ^~~~~~~

untitled.c: In function ‘job_read’:
untitled.c:98:13: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘ssize_t {aka long int}’ may change the sign of the result [-Wsign-conversion]
         n = read(newsockfd, buffer, BUFLEN);
             ^~~~

untitled.c:105:17: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘ssize_t {aka long int}’ may change the sign of the result [-Wsign-conversion]
             n = write(socket_ids[i], buffer, strlen(buffer));
                 ^~~~~

untitled.c:106:18: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
             if(n < 0){
                  ^

untitled.c: In function ‘job_write’:
untitled.c:126:17: warning: conversion to ‘size_t {aka long unsigned int}’ from ‘ssize_t {aka long int}’ may change the sign of the result [-Wsign-conversion]
             n = write(socket_ids[i], buffer, strlen(buffer));
                 ^~~~~

untitled.c:127:18: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
             if(n < 0){
                  ^

untitled.c:116:23: warning: unused parameter ‘args’ [-Wunused-parameter]
 void* job_write(void* args){

Once you have the code cleanly compiling, then please update (add an EDIT) and we can help you

user3629249
  • 16,402
  • 1
  • 16
  • 17
0

Your code has several problems related to the use of the return value of read, as has been mentioned in the comments. In general, you should use the ssize_t data type for the return value, and check separately for the three cases n > 0 (success), n == 0 (the other side closed the connection) and n < 0 (error).

Also, bcopy and bzero are deprecated in POSIX and should be replaced by memcpy and memset. Furthermore, I had to replace server->h_addr by server->h_addr_list[0] to get the code to compile. The manpage mentions that h_addr is only for backwards compatibility.

Now to the main problem causing the weirdness in messaging: You never initialize the global sockfd variable in client.c, since it is shadowed by the declaration inside main. Since it has static scope, it is automatically initialized to zero, i.e. to standard input. Therefore the read(sockfd, ...) in job_read in client.c actually reads from standard input, and not from the server. The clients never read from the server. Instead, every second line you enter is dealt with in main, and sent to the server. The other lines are dealt with in job_read, and are then printed with the incorrect prefix Server:.

The fix is simple: Remove the line int sockfd; in main in client.c.

rtoijala
  • 1,200
  • 10
  • 20