1
\$\begingroup\$

This is the code I am using for a distributed system. The code will be used to allow the server to send parts of a (really big) document which will be processed by the connected clients. Please tell me what things would make it more efficient and if there are any security issues that I can correct without addition of an auth library.

#include<stdio.h>
#include<string.h> //strlen
#include<stdlib.h> //strlen
#include<sys/socket.h>
#include<arpa/inet.h> //inet_addr
#include<unistd.h> //write
#include<pthread.h> //for threading , link with lpthread

void *connection_handler(void *);

int main(int argc , char *argv[]) {
    int socket_desc , client_sock , c;
    struct sockaddr_in server , client;
    //Create socket
    socket_desc = socket(AF_INET , SOCK_STREAM , 0);
    if (socket_desc == -1) {
        printf("Could not create socket");
    }
    puts("Socket created");
    //Prepare the sockaddr_in structure
    server.sin_family = AF_INET;
    server.sin_addr.s_addr = INADDR_ANY;
    server.sin_port = htons( 8888 );
    //Bind
    if( bind(socket_desc,(struct sockaddr *)&server , sizeof(server)) < 0) {
        //print the error message
        perror("bind failed. Error");
        return 1;
    }
    puts("bind done");
    //Listen
    listen(socket_desc , 3);
    //Accept and incoming connection
    puts("Waiting for incoming connections...");
    c = sizeof(struct sockaddr_in);

    pthread_t thread_id;

    while( (client_sock = accept(socket_desc, (struct sockaddr *)&client, (socklen_t*)&c)) ) {
        puts("Connection accepted");
        if (pthread_create( &thread_id , NULL , connection_handler , (void*) &client_sock) < 0) {
            perror("could not create thread");
            return 1;
        }
        //Now join the thread , so that we dont terminate before the thread
        //pthread_join( thread_id , NULL);
        puts("Handler assigned");
    }

    if (client_sock < 0) {
        perror("accept failed");
        return 1;
    }
    return 0;
}
/*
* This will handle connection for each client
* */
void *connection_handler(void *socket_desc) {
    //Get the socket descriptor
    int sock = *(int*)socket_desc;
    int read_size;
    char *message , client_message[2000];
    //Send some messages to the client
    message = "Greetings! I am your connection handler\n";
    write(sock , message , strlen(message));
    message = "Now type something and i shall repeat what you type \n";

    write(sock , message , strlen(message));
    //Receive a message from client
    while( (read_size = recv(sock , client_message , 2000 , 0)) > 0 ) {
        //end of string marker
        client_message[read_size] = '\0';
        puts (client_message);
        //Send the message back to client
        write(sock , client_message , strlen(client_message));
        //clear the message buffer
        memset(client_message, 0, 2000);
    }

    // system ("cd; chmod +x script.sh;./script");

    if(read_size == 0) {
        puts("Client disconnected");
        fflush(stdout);
    }
    else if(read_size == -1) {
        perror("recv failed");
    }
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Looks like a copy-pasta error. Accept and incoming connection and the following line are duplicated. \$\endgroup\$ – Jerry Coffin Jan 22 '16 at 16:51
  • \$\begingroup\$ sorry about that! the code doesn't show any error on runtime though. I have put the code for review not for error checking though. \$\endgroup\$ – kneelb4darth Jan 22 '16 at 16:56
  • \$\begingroup\$ That particular duplication would cause a real problem, beyond the fact that it would print "Waiting for incoming connections..." twice as it started up. \$\endgroup\$ – Jerry Coffin Jan 22 '16 at 16:58
1
\$\begingroup\$

I'm not really into networking, but I can say a little about your coding style:

Missing return

if (socket_desc == -1) {
    printf("Could not create socket");
}
puts("Socket created");

If socket_desc is equal to -1. Could not create socket will be printed as well as Socket created, which is misleading. I think:

if (socket_desc == -1) {
    printf("Could not create socket");
    return -1;                           /* or some other error code */
}
puts("Socket created");

is appropriate and the code doesn't proceeds.

Outsource
I'm also not sure if it would be better to outsource this part (as void prepareServer(sockaddr_in *server):

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

Unnecessary comments
I don't know who else will read this code but comments like //print the error message followed by this perror("bind failed. Error"); will just make the code more obscure, since the goal of the error message is pretty obvious.

Calling strlen for no reason
Also it's not really clear to me, why you need to call strlen(client_message) here

write(sock , client_message , strlen(client_message));

when you store assign read_size a few lines above in the while-condition. strlen will just search for a '\0', which you assign by yourself with read_size...

Besides these points I can't see any major mistakes. You should just think about your comments a little more (Do I really need this information? Is this self explaining? Is a single line comment enough here?).
As I already said: I can't say much about the networking part, maybe someone else will jump here.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.