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

• Berkin: Thank you for your reply, i changed malloc 1 to sizeof(int). I removed fflush option. I think, in your 3rd explanation, new_sock values are never changed, they are read only. Is it a problem? And your last comment, in this program, clients never stop until i give ctrl+c from the terminal. Should i give an exit condition? Thanks
• e.jahandar: Thats not a problem when the close are happening outside of threads and so synchronized. you can set an exit condition for example upon receiving CTRL+C Signal (SIGINTR) and try to set a flag and check it inside threads, so threads are closed and the socket will be closed gracefully. in your special case this approach make nothing sense but in applications which requires synchronized closing this comes around and should be handled properly

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.