Search code examples
csocketsread-write

C Socket Write Too many times


After the user as entered data it writes out a part of the data entered before the data just enetered if that makes sense? I've only included a snippet, but can anyone see any reason why it would reply multiple times?

#include<stdio.h>
#include<string.h>
#include<stdlib.h>
#include<sys/socket.h>
#include<arpa/inet.h> //inet_addr
#include<unistd.h> // write

#include<pthread.h> // For Threading

#include<wiringPi.h>
void *connection_handler(void *);
void lightLED(int pin,int status);
int maxConnections = 1;
int totalConnections = 0;
int main(int argc , char *argv[])
{
    int socket_desc , new_socket , c, *new_sock;
    struct sockaddr_in server , client;
    char *message;     
    //Create socket
    socket_desc = socket(AF_INET , SOCK_STREAM , 0);
    if (socket_desc == -1)
    {
        printf("Could not create socket");
    }

    //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)
    {
        puts("bind failed");
    }
    puts("bind done");

    //Listen
    listen(socket_desc , 3);

    //Accept and incoming connection
    puts("Waiting for incoming connections...");
    c = sizeof(struct sockaddr_in);
    while( (new_socket = accept(socket_desc, (struct sockaddr *)&client, (socklen_t*)&c)) )
    {
     if(new_socket > 0)
     {
      if(totalConnections < maxConnections){
      totalConnections++;
     }
      else
      {
       message = "Sorry Maximum Users Reached\n";
       write(new_socket,message,strlen(message));
       puts("Too many Users");
       close(new_socket);
       continue;
      }
    }
     puts("Connection Accepted");
    char *client_ip = inet_ntoa(client.sin_addr);
    int client_port = ntohs(client.sin_port);
    printf("ClientIP:%s\n",client_ip);
     message = "Hello you have been accepted!\n";
     write(new_socket, message , strlen(message));

    pthread_t sniffer_thread;
    new_sock = malloc(1);
    *new_sock = new_socket;

    if(pthread_create( &sniffer_thread, NULL , connection_handler , (void*) new_sock) <0)
    {
     perror("Could not create thread");
    return 1;
    }
    puts("Handler Assigned");
    }

    if (new_socket<0)
    {
        perror("accept failed");
    return 1;
    }
    return 0;
}

void *connection_handler(void *socket_desc)
{
    int sock = *(int*)socket_desc;
    int read_size;
    char *message , client_message[2000];

    message = "Greeting! I am your Connection Handler\n";
    write(sock , message,strlen(message));

    message =  "What do you want to do\n";
    write(sock,message,strlen(message));

    while( (read_size = recv(sock , client_message , 2000 , 0)) > 0)
    {
     write(sock , client_message , strlen(client_message));
     printf("User Entered:%s\n",client_message);
     int pin = client_message[0]-'0';
     int status = client_message[1]-'0';
     lightLED(pin,status);
    }
    if(read_size == 0)
    {
    puts("Client Disconnected\n");
    fflush(stdout);
    totalConnections--;
    }else if(read_size == -1)
    {
     perror("recv Failed");
    }

    free(socket_desc);
    return 0;
}

void lightLED(int pin,int status)
{
    char message;
    if(wiringPiSetup() == -1){
    puts("wiringPi Error");
        exit(1);
    }

    //pinMode(pin,OUTPUT);
    printf("Changing LED Pin- %d Status- %d\n",pin,status);
    //digitalWrite(pin,status);
}

Solution

  • First pass

    I'm not sure what problem you're seeing. I've taken your code (which was in pretty good shape — well done; I've looked at a lot of code with many worse problems in it) and compiled it and run it and it seemed to work for me:

    $ nc localhost 8888
    Connection Accepted
    ClientIP:127.0.0.1
    Handler Assigned
    Hello you have been accepted!
    Greeting! I am your Connection Handler
    What do you want to do
    01
    User Entered:01
    
    Changing LED Pin- 0 Status- 1
    01
    21
    User Entered:21
    
    Changing LED Pin- 2 Status- 1
    21
    31
    User Entered:31
    
    Changing LED Pin- 3 Status- 1
    31
    we wish you a merry Christmas
    User Entered:we wish you a merry Christmas
    
    Changing LED Pin- 71 Status- 53
    we wish you a merry Christmas
    Client Disconnected
    
    $
    

    The code as run was:

    #include <stdio.h>
    #include <string.h>
    #include <stdlib.h>
    #include <sys/socket.h>
    #include <arpa/inet.h>
    #include <unistd.h>
    #include <pthread.h>
    //#include <wiringPi.h>
    
    void *connection_handler(void *);
    void lightLED(int pin, int status);
    
    int maxConnections = 1;
    int totalConnections = 0;
    
    int main(void)
    {
        int socket_desc, new_socket, c, *new_sock;
        struct sockaddr_in server, client;
        char *message;
    
        socket_desc = socket(AF_INET, SOCK_STREAM, 0);
        if (socket_desc == -1)
        {
            printf("Could not create socket");
            return 1;
        }
    
        server.sin_family = AF_INET;
        server.sin_addr.s_addr = INADDR_ANY;
        server.sin_port = htons(8888);
    
        if ( bind(socket_desc, (struct sockaddr *)&server, sizeof(server)) < 0)
        {
            puts("bind failed");
            return 1;
        }
        puts("bind done");
    
        if (listen(socket_desc, 3) != 0)
        {
            perror("listen() failed");
            return 1;
        }
    
        puts("Waiting for incoming connections...");
        c = sizeof(struct sockaddr_in);
        while ((new_socket = accept(socket_desc, (struct sockaddr *)&client, (socklen_t*)&c)))
        {
            if (new_socket > 0)
            {
                if (totalConnections < maxConnections)
                    totalConnections++;
                else
                {
                    message = "Sorry Maximum Users Reached\n";
                    write(new_socket, message, strlen(message));
                    puts("Too many Users");
                    close(new_socket);
                    continue;
                }
            }
            puts("Connection Accepted");
            char *client_ip = inet_ntoa(client.sin_addr);
            //int client_port = ntohs(client.sin_port);
    
            printf("ClientIP:%s\n", client_ip);
            message = "Hello you have been accepted!\n";
            write(new_socket, message, strlen(message));
    
            pthread_t sniffer_thread;
            new_sock = malloc(1 * sizeof(int));       // Oops!
            if (new_sock == 0) { perror("out of memory"); return 1; }
            *new_sock = new_socket;
    
            if (pthread_create( &sniffer_thread, NULL, connection_handler, (void*) new_sock) <0)
            {
                perror("Could not create thread");
                return 1;
            }
            puts("Handler Assigned");
        }
    
        if (new_socket<0)
        {
            perror("accept failed");
            return 1;
        }
        return 0;
    }
    
    void *connection_handler(void *socket_desc)
    {
        int sock = *(int*)socket_desc;
        int read_size;
        char *message, client_message[2000];
    
        message = "Greeting! I am your Connection Handler\n";
        write(sock, message, strlen(message));
    
        message =  "What do you want to do\n";
        write(sock, message, strlen(message));
    
        while ((read_size = recv(sock, client_message, 2000, 0)) > 0)
        {
            write(sock, client_message, strlen(client_message));
            printf("User Entered:%s\n", client_message);
            int pin = client_message[0]-'0';
            int status = client_message[1]-'0';
            lightLED(pin, status);
        }
        if (read_size == 0)
        {
            puts("Client Disconnected\n");
            fflush(stdout);
            totalConnections--;
        }
        else if (read_size == -1)
        {
            perror("recv Failed");
        }
    
        free(socket_desc);
        return 0;
    }
    
    void lightLED(int pin, int status)
    {
    //    if (wiringPiSetup() == -1)
    //    {
    //        puts("wiringPi Error");
    //        exit(1);
    //    }
    
        printf("Changing LED Pin- %d Status- %d\n", pin, status);
    }
    

    If you still have problems, maybe the trouble is in your client code. As you can see, I used netcat (nc) as a surrogate for your client. Note that 'we wish you a merry Christmas' was accepted as a valid command, though the pin was 73 and the status was 53. That might not work with the real LEDs.

    Note that I added an error check for the malloc() and allocated a more correct amount of space (sizeof(int) instead of just 1 byte). I also made sure that reported error conditions are followed by a more-or-less appropriate error return, rather than continuing as if no error had occurred.

    Also, I've not fixed some of the issues highlighted in the comments — checking write() and not relying on null termination, etc. These should still be addressed.

    My test was on Mac OS X 10.7.5 with GCC 4.7.1:

    gcc -O3 -g -std=c99 -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes -Wold-style-definition server.c -o server
    

    Second pass

    Another test run — showing the problem with inputs that are not null terminated:

    $ nc localhost 8888
    Connection Accepted
    ClientIP:127.0.0.1
    Hello you have been accepted!
    Handler Assigned
    Greeting! I am your Connection Handler
    What do you want to do
    this is a long string - what will you do with it?
    User Entered:this is a long string - what will you do with it?
    
    Changing LED Pin- 68 Status- 56
    this is a long string - what will you do with it?
    01
    User Entered:01
    s is a long string - what will you do with it?
    
    Changing LED Pin- 0 Status- 1
    01
    s is a long string - what will you do with it?
    Client Disconnected
    
    $
    

    When I ran it with telnet instead of nc, I got the misbehaviour you were seeing, I think:

    $ telnet localhost 8888
    Trying 127.0.0.1...
    Connection Accepted
    ClientIP:127.0.0.1
    Handler Assigned
    Connected to localhost.
    Escape character is '^]'.
    Hello you have been accepted!
    Greeting! I am your Connection Handler
    What do you want to do
    Would you like a biscuit?
    User Entered:Would you like a biscuit?
    
    Changing LED Pin- 39 Status- 63
    Would you like a biscuit?
    93
    User Entered:93
    d you like a biscuit?
    
    Changing LED Pin- 9 Status- 3
    93
    d you like a biscuit?
    Intriguing
    User Entered:Intriguing
    ke a biscuit?
    
    Changing LED Pin- 25 Status- 62
    Intriguing
    ke a biscuit?
    Bye
    User Entered:Bye
    guing
    ke a biscuit?
    
    Changing LED Pin- 18 Status- 73
    Bye
    guing
    ke a biscuit?
    User Entered:ye
    guing
    ke a biscuit?
    
    Changing LED Pin- -44 Status- 73
    ye
    guing
    ke a biscuit?
    ^CUser Entered:????guing
    ke a biscuit?
    
    Changing LED Pin- -49 Status- -60
    User Entered:???guing
    ke a biscuit?
    
    Changing LED Pin- -49 Status- -53
    ?guing
    ke a biscuit?
    User Entered:??guing
    ke a biscuit?
    
    ...continued attempts with control-C (interrupt)...
    ...and control-D (EOF) not producing anything useful...
    
    ^]
    telnet> qConnection closed.
    Client Disconnected
    
    $
    

    So, telnet may have been misleading you...nothing up with your server, just the client (telnet) not behaving as you expected.

    Updated code

    Conversation with updated server code:

    $ nc localhost 8888
    Connection Accepted
    ClientIP: 127.0.0.1
    Handler Assigned
    Hello you have been accepted!
    Greetings! I am your Connection Handler
    What do you want to do
    13
    User Entered:13
    
    Changing LED Pin 1 status 3
    13
    21
    User Entered:21
    
    Changing LED Pin 2 status 1
    21
    elephants?
    User Entered:elephants?
    
    Changing LED Pin 53 status 60
    elephants?
    21
    User Entered:21
    
    Changing LED Pin 2 status 1
    21
    quit 
    User Entered:quit
    
    Changing LED Pin 65 status 69
    quit
    Client Disconnected
    
    $
    

    Updated server code

    This version pays attention to lengths and ensures that strings are null terminated.

    #include <stdio.h>
    #include <string.h>
    #include <stdlib.h>
    #include <sys/socket.h>
    #include <arpa/inet.h>
    #include <unistd.h>
    #include <pthread.h>
    //#include <wiringPi.h>
    
    void *connection_handler(void *);
    void lightLED(int pin, int status);
    static void write_sock(int sock, const char *msg);
    
    int maxConnections = 1;
    int totalConnections = 0;
    
    int main(void)
    {
        int socket_desc, new_socket, c, *new_sock;
        struct sockaddr_in server, client;
    
        socket_desc = socket(AF_INET, SOCK_STREAM, 0);
        if (socket_desc == -1)
        {
            printf("Could not create socket");
            return 1;
        }
    
        server.sin_family = AF_INET;
        server.sin_addr.s_addr = INADDR_ANY;
        server.sin_port = htons(8888);
    
        if (bind(socket_desc, (struct sockaddr *)&server, sizeof(server)) < 0)
        {
            puts("bind failed");
            return 1;
        }
        puts("bind done");
    
        if (listen(socket_desc, 3) != 0)
        {
            perror("listen() failed");
            return 1;
        }
    
        puts("Waiting for incoming connections...");
        c = sizeof(struct sockaddr_in);
        while ((new_socket = accept(socket_desc, (struct sockaddr *)&client, (socklen_t*)&c)))
        {
            if (new_socket > 0)
            {
                if (totalConnections < maxConnections)
                    totalConnections++;
                else
                {
                    write_sock(new_socket, "Sorry Maximum Users Reached\n");
                    puts("Too many Users");
                    close(new_socket);
                    continue;
                }
            }
    
            puts("Connection Accepted");
            char *client_ip = inet_ntoa(client.sin_addr);
            //int client_port = ntohs(client.sin_port);
    
            printf("ClientIP: %s\n", client_ip);
            write_sock(new_socket, "Hello you have been accepted!\n");
    
            pthread_t sniffer_thread;
            new_sock = malloc(1 * sizeof(int));       // Oops!
            if (new_sock == 0) { perror("out of memory"); return 1; }
            *new_sock = new_socket;
    
            if (pthread_create(&sniffer_thread, NULL, connection_handler, (void *)new_sock) < 0)
            {
                perror("Could not create thread");
                return 1;
            }
            puts("Handler Assigned");
        }
    
        if (new_socket < 0)
        {
            perror("accept failed");
            return 1;
        }
        return 0;
    }
    
    // Avoid repetition - use functions!
    static void write_sock(int sock, const char *msg)
    {
        int len = strlen(msg);
        if (write(sock, msg, len) != len)
        {
            perror("short write on socket");
            exit(1);
        }
    }
    
    void *connection_handler(void *socket_desc)
    {
        int sock = *(int*)socket_desc;
        int read_size;
        char client_message[2000];
    
        write_sock(sock, "Greetings! I am your Connection Handler\n");
        write_sock(sock, "What do you want to do\n");
    
        while ((read_size = recv(sock, client_message, 2000, 0)) > 0)
        {
            client_message[read_size] = '\0';
            write_sock(sock, client_message);
            printf("User Entered:%s\n", client_message);
            int pin = client_message[0]-'0';
            int status = client_message[1]-'0';
            lightLED(pin, status);
        }
    
        if (read_size == 0)
        {
            puts("Client Disconnected\n");
            fflush(stdout);
            totalConnections--;
        }
        else if (read_size == -1)
        {
            perror("recv Failed");
        }
    
        free(socket_desc);
        return 0;
    }
    
    void lightLED(int pin, int status)
    {
    //    if (wiringPiSetup() == -1)
    //    {
    //        puts("wiringPi Error");
    //        exit(1);
    //    }
        printf("Changing LED Pin %d status %d\n", pin, status);
    }
    

    Note the use of the write_sock() function to encapsulate repeated code (which has the side benefit of only requiring the code to be written once, so it can be correct every time it is used).

    #include <stdio.h>
    #include <string.h>
    #include <stdlib.h>
    #include <sys/socket.h>
    #include <arpa/inet.h>
    #include <unistd.h>
    #include <pthread.h>
    //#include <wiringPi.h>
    
    void *connection_handler(void *);
    void lightLED(int pin, int status);
    static void write_sock(int sock, const char *msg);
    
    int maxConnections = 1;
    int totalConnections = 0;
    
    int main(void)
    {
        int socket_desc, new_socket, c, *new_sock;
        struct sockaddr_in server, client;
    
        socket_desc = socket(AF_INET, SOCK_STREAM, 0);
        if (socket_desc == -1)
        {
            printf("Could not create socket");
            return 1;
        }
    
        server.sin_family = AF_INET;
        server.sin_addr.s_addr = INADDR_ANY;
        server.sin_port = htons(8888);
    
        if (bind(socket_desc, (struct sockaddr *)&server, sizeof(server)) < 0)
        {
            puts("bind failed");
            return 1;
        }
        puts("bind done");
    
        if (listen(socket_desc, 3) != 0)
        {
            perror("listen() failed");
            return 1;
        }
    
        puts("Waiting for incoming connections...");
        c = sizeof(struct sockaddr_in);
        while ((new_socket = accept(socket_desc, (struct sockaddr *)&client, (socklen_t*)&c)))
        {
            if (new_socket > 0)
            {
                if (totalConnections < maxConnections)
                    totalConnections++;
                else
                {
                    write_sock(new_socket, "Sorry Maximum Users Reached\n");
                    puts("Too many Users");
                    close(new_socket);
                    continue;
                }
            }
    
            puts("Connection Accepted");
            char *client_ip = inet_ntoa(client.sin_addr);
            //int client_port = ntohs(client.sin_port);
    
            printf("ClientIP: %s\n", client_ip);
            write_sock(new_socket, "Hello you have been accepted!\n");
    
            pthread_t sniffer_thread;
            new_sock = malloc(1 * sizeof(int));       // Oops!
            if (new_sock == 0) { perror("out of memory"); return 1; }
            *new_sock = new_socket;
    
            if (pthread_create(&sniffer_thread, NULL, connection_handler, (void *)new_sock) < 0)
            {
                perror("Could not create thread");
                return 1;
            }
            puts("Handler Assigned");
        }
    
        if (new_socket < 0)
        {
            perror("accept failed");
            return 1;
        }
        return 0;
    }
    
    // Avoid repetition - use functions!
    static void write_sock(int sock, const char *msg)
    {
        int len = strlen(msg);
        if (write(sock, msg, len) != len)
        {
            perror("short write on socket");
            exit(1);
        }
    }
    
    void *connection_handler(void *socket_desc)
    {
        int sock = *(int*)socket_desc;
        int read_size;
        char client_message[2000];
    
        write_sock(sock, "Greetings! I am your Connection Handler\n");
        write_sock(sock, "What do you want to do\n");
    
        while ((read_size = recv(sock, client_message, 2000, 0)) > 0)
        {
            client_message[read_size] = '\0';
            write_sock(sock, client_message);
            printf("User Entered:%s\n", client_message);
            int pin = client_message[0]-'0';
            int status = client_message[1]-'0';
            lightLED(pin, status);
        }
    
        if (read_size == 0)
        {
            puts("Client Disconnected\n");
            fflush(stdout);
            totalConnections--;
        }
        else if (read_size == -1)
        {
            perror("recv Failed");
        }
    
        free(socket_desc);
        return 0;
    }
    
    void lightLED(int pin, int status)
    {
    //    if (wiringPiSetup() == -1)
    //    {
    //        puts("wiringPi Error");
    //        exit(1);
    //    }
        printf("Changing LED Pin %d status %d\n", pin, status);
    }