Socket client in C using threads

Question

I'm working on socket programming in C. I have no problem with usage the threads. This all works fine but I'm new in this area. I wrote this code in client.c but is there any misused code or something may cause problems in the future?

My client sends a message to the server and client can receive it. There are 2 major operations: recv and send.

CLIENT

#include<stdio.h>
#include<sys/socket.h>
#include<arpa/inet.h> // for inet_addr
#include <string.h>
#include <zconf.h>
#include <stdlib.h>
#include <pthread.h>

void *sendMessage(void *sock_desc) {
    //Send some data
    while (1) {
        char message[2000];
        printf("%s","> ");
        scanf("%[^\n]%*c", message);
        fflush(stdin);

        if (send(*((int *) sock_desc), message, strlen(message) + 1, 0) < 0) {
            puts("Send failed");
        }
    }
}

void *receiveMessage(void *sock_desc) {
    while (1) {
        char server_reply[2000];
        if (recv(*((int *) sock_desc), server_reply, 2000, 0) < 0) {
            puts("recv failed");

        }
        //Receive a reply from the server
        printf("\033[32;1m %s \033[0m\n", server_reply);
    }
}

int main(int argc, char *argv[]) {
    int                sock;
    struct sockaddr_in server;
    //char               message[2000], server_reply[2000];

    //Create socket
    sock = socket(AF_INET, SOCK_STREAM, 0);
    if (sock == -1) {
        printf("Could not create socket");
    }

    server.sin_addr.s_addr = inet_addr("127.0.0.1");
    server.sin_family      = AF_INET;
    server.sin_port        = htons(8888);

    //Connect to remote server
    if (connect(sock, (struct sockaddr *) &server, sizeof(server)) < 0) {
        perror("Connect failed. Error");
        return 1;
    }

    puts("Connected to server\n");
    int *new_sock;
    new_sock = malloc(1);
    *new_sock = sock;
    //keep communicating with server

    pthread_t send_thread, receive_thread;
    pthread_create(&send_thread, NULL, sendMessage, (void *) new_sock);
    pthread_create(&receive_thread, NULL, receiveMessage, (void *) new_sock);

    pthread_join(receive_thread, NULL);
    pthread_join(send_thread, NULL);

    close(sock);
    return 0;
}

Answer 1

You need to have provision to exit the infinite while loop on error conditions. Check errno and act upon it accordingly.

One such condition is, if recv returned zero, it means the other end had closed the connection. You can close the socket and exit the loop. The send will then fail with "Broken Pipe" error. You can quit that loop and hence enable the main thread have join the threads.

There can be multiple ways in which recv and send can return error. Read them up and improve your code.

Answer 2

A streaming protocol has no concept of a message. recv may receive any amount of data, which leads to two unpleasant scenarios:

• a terminating byte is not (yet) received. printf prints whatever garbage is in the buffer (maybe leftovers from previous receives, maybe uninitialized data). Technically UB (undefined behavior).

• recv gets two messages in a single go. printf prints the first one, and the second is lost forever.

Similarly, send is not guaranteed to send an entire amount.

It is very important to see how much data has been sent/received (both functions return this information), and account for incomplete transactions.

Answer 3

1. You have to allocate sizeof(int) bytes for socket file descriptor, not 1
2. fflush(stdin) has undefined behavior
3. its not good idea to access a file descriptor in two threads simultaneously, be careful
4. joins are never happens, because none of two threads have exit conditions, so the socket will never close gracefully

Answer 4

In addition to what others have said: using pointers doesn't force you to manually manage memory.

You don't need to allocate a new int (which you never free) just to pass it via pointer to your thread functions, simply use (void*)&sock as sock currently outlives the life of the threads so there's no risk of a dangling pointer.