C TCP sockets, echo server with file sending, hangs up after sending a file

Question

I wanted to write a simple TCP echo server application. I managed to do the echo part, but have some problems with sending files between client and server. The idea is simple: despite sending ordinary messages, client can send a special command to server (\SENDFILE filename.txt), and after receiving such command, server should ask client for this file, and get the file from client. (Further I would like to get a file from one client, and later send it to another one).

I think "the protocol" here is simple, however, after typing \SENDFILE at client's side, client hangs up, and does not receive any further messages from server. Moreover (server and client are in different directories) at server's side there's only an empty file from client, with no content inside.

Any ideas what can be wrong here?

client.c

#include<stdio.h> //printf
#include<string.h>    //
#include <sys/stat.h>
#include<sys/socket.h>    //socket
#include<arpa/inet.h> //inet_addr
#include <fcntl.h>
#define SERVER_PORT     9034
#define BUFF_SIZE       2000

int sendall(int s, char *buf, int len)
{
    int total = 0;
    int bytesleft = len;
    int n;

    while(total < len)
    {
        n = send(s, buf+total, bytesleft, 0);
        if (n == -1)
            break;
        total += n;
        bytesleft -= n;
    }

    return n==-1?-1:0;
}

void SendMsgToSender(char *msg, int connfd)
{
    write(connfd, msg, strlen(msg));
    memset(msg, 0, BUFF_SIZE);
}

int main(int argc , char *argv[])
{
    int sock;
    struct sockaddr_in server;
    char bufferOUT[BUFF_SIZE] , bufferIN[BUFF_SIZE];
    struct stat file_stat;

    memset(bufferOUT, 0, BUFF_SIZE);
    memset(bufferIN, 0, BUFF_SIZE);

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

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

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

    // puts("Connected\n");
    int read_size = 10;

    //keep communicating with server
    while(1)
    {
        printf("> ");
        fgets(bufferOUT, BUFF_SIZE, stdin);

        //Send some data
        if( send(sock , bufferOUT , BUFF_SIZE , 0) < 0)
        {
            perror("Send failed");
            return 1;
        }

        //Receive a reply from the server
        if( (read_size = recv(sock , bufferIN , BUFF_SIZE , 0)) < 0)
        {
            perror("Recv failed");
            break;
        }

        if(read_size == 0)
            break;

        if(bufferIN[0] == '\\')
        {
            char tmp[BUFF_SIZE], filename[BUFF_SIZE], *param;
            memset(filename, BUFF_SIZE, 0);
            strcpy(tmp, bufferIN);

            param = strtok(tmp, " ");
            if(param != NULL)
            {
                if(!strcmp(param, "\\GIVEMEFILE"))
                {
                    param = strtok(NULL, " ");
                    if(param != NULL)
                    {
                        strcpy(filename, param);
                        FILE * fp;
                        int nBytes;
                        char buffer[BUFF_SIZE], *s;
                        memset(buffer, 0, BUFF_SIZE);

                        fp = fopen(filename, "r");
                        if(fp == NULL)
                        {
                            perror("fopen");
                            fflush(stdout);
                            break;
                        }

                        int remain_data = file_stat.st_size;

                        do
                        {
                            s = fgets(buffer, BUFF_SIZE, fp);
                            if(s != NULL && buffer[0] != EOF)
                            {
                                nBytes = sendall(sock, buffer, BUFF_SIZE);
                                remain_data -= nBytes;
                            }
                            else
                                break;
                        }
                        while((s != NULL) && (nBytes > 0) && (remain_data > 0));
                        fclose(fp);

                        memset(bufferOUT, 0, BUFF_SIZE);
                        memset(bufferIN, 0, BUFF_SIZE);

                        continue;
                    }
                }
            }
        }
        else
        {
            printf("%s\n", bufferIN);
            fflush(stdout);
        }

        memset(bufferOUT, 0, BUFF_SIZE);
        memset(bufferIN, 0, BUFF_SIZE);
    }

    close(sock);
    return 0;
}

server.c

    #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 <arpa/inet.h>
#include <netdb.h>
#include <time.h>
#include <fcntl.h>

#define SERVER_PORT     9034
#define BUFF_SIZE       2000

void StripNewline(char *s)
{
    while(*s != '\0')
    {
        if(*s == '\r' || *s == '\n')
        {
            *s = '\0';
        }
        s++;
    }
}

void SendMsgToSender(char *msg, int connfd)
{
    write(connfd, msg, strlen(msg));
    memset(msg, 0, BUFF_SIZE);
}

// get sockaddr, IPv4 or IPv6:
void *get_in_addr(struct sockaddr *sa)
{
    if (sa->sa_family == AF_INET)
    {
        return &(((struct sockaddr_in*)sa)->sin_addr);
    }

    return &(((struct sockaddr_in6*)sa)->sin6_addr);
}

int GetFileFromClient(int connfd, char *filename)
{
    FILE * fp = NULL;
    int bytes;
    char buffer[BUFF_SIZE];
    memset(buffer, 0, BUFF_SIZE);

    fp = fopen(filename, "w");
    if(fp == NULL)
        return 0;

    memset(buffer, 0, BUFF_SIZE);
    sprintf(buffer, "\\GIVEMEFILE %s \r\n", filename);
    SendMsgToSender(buffer, connfd);

    while(1)
    {
        memset(buffer ,0 , BUFF_SIZE);
        if((bytes =  recv(connfd , buffer , BUFF_SIZE , 0) ) <= 0)
            return 0;
        else
            fprintf(fp, "%s\n", buffer);
    }

    fclose(fp);
    sleep(1);

    memset(buffer, 0, BUFF_SIZE);
    sprintf(buffer, "\r\n");
    SendMsgToSender(buffer, connfd);

    return 1;
}

int main(void)
{
    fd_set master;
    fd_set read_fds;
    int fdmax;

    int listener;
    int client_sock;
    struct sockaddr_storage remoteaddr;
    socklen_t addrlen;

    char bufferIN[BUFF_SIZE], bufferOUT[BUFF_SIZE], tmp[BUFF_SIZE], *datetime;
    int nbytes;

    char remoteIP[INET6_ADDRSTRLEN];

    int yes=1;
    int i, j, rv;

    struct addrinfo hints, *ai, *p;

    FD_ZERO(&master);
    FD_ZERO(&read_fds);

    memset(bufferIN, 0, BUFF_SIZE);
    memset(bufferOUT, 0, BUFF_SIZE);
    memset(tmp, 0, BUFF_SIZE);

    memset(&hints, 0, sizeof hints);
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_flags = AI_PASSIVE;

    char port[16] = "9034";
    if (getaddrinfo(NULL, port, &hints, &ai) < 0)
    {
        fprintf(stderr, "selectserver: %s\n", gai_strerror(rv));
        exit(1);
    }

    for(p = ai; p != NULL; p = p->ai_next)
    {
        listener = socket(p->ai_family, p->ai_socktype, p->ai_protocol);
        if (listener < 0)
        {
            continue;
        }

        setsockopt(listener, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(int));

        if (bind(listener, p->ai_addr, p->ai_addrlen) < 0)
            continue;

        break;
    }

    if (p == NULL)
        exit(2);

    freeaddrinfo(ai);

    if (listen(listener, 10) == -1)
    {
        perror("listen");
        exit(3);
    }

    FD_SET(listener, &master);
    fdmax = listener;

    printf("Server is running ...\n\n");

    for(;;)
    {
        read_fds = master;
        if (select(fdmax+1, &read_fds, NULL, NULL, NULL) == -1)
        {
            perror("select");
            exit(4);
        }

        for(i = 0; i <= fdmax; i++)
        {
            if (FD_ISSET(i, &read_fds))
            {
                if (i == listener)
                {
                    addrlen = sizeof remoteaddr;
                    client_sock = accept(listener,
                                         (struct sockaddr *)&remoteaddr,
                                         &addrlen);

                    if (client_sock == -1)
                    {
                        perror("accept");
                    }
                    else
                    {
                        FD_SET(client_sock, &master);
                        if (client_sock > fdmax)
                            fdmax = client_sock;
                    }
                }
                else
                {
                    if ((nbytes = recv(i, bufferIN, BUFF_SIZE, 0)) <= 0)
                    {
                        if (nbytes == 0)
                            close(i);

                        else if(nbytes == -1)
                        {
                            perror("recv");
                            fflush(stdout);
                        }

                        close(i);
                        FD_CLR(i, &master);
                    }
                    else
                    {
                        bufferIN[nbytes-1] = '\0';
                        StripNewline(bufferIN);
                        strcpy(tmp, bufferIN);

                        if(bufferIN[0] == '\\')
                        {
                            char *command, *param;
                            command = strtok(bufferIN, " ");

                            if(!strcmp(command, "\\QUIT"))
                            {
                                close(i);
                                FD_CLR(i, &master);
                                break;
                            }

                            else if(!strcmp(command, "\\SENDFILE"))
                            {
                                param = strtok(tmp, " ");
                                if(param != NULL)
                                {
                                    param = strtok(NULL, " ");
                                    if(param != NULL)
                                    {
                                        printf("Client is sending me a file '%s'...\n", param);
                                        GetFileFromClient(i, param);
                                    }
                                }
                            }
                            else
                            {
                                SendMsgToSender(bufferIN, i);
                            }

                            memset(bufferIN, 0, BUFF_SIZE);
                            memset(bufferOUT, 0, BUFF_SIZE);
                        }
                        else
                        {
                            SendMsgToSender(bufferIN, i);
                        }
                    }
                } // END handle data from client
            } // END got new incoming connection
        } // END looping through file descriptors
    } // END for(;;)

    memset(bufferIN, 0, BUFF_SIZE);
    memset(bufferOUT, 0, BUFF_SIZE);

    return 0;
}

Answer 1

strcpy(tmp, bufferIN);

Here you are assuming that whatever was read was null-terminated.

            param = strtok(tmp, " ");
            if(param != NULL)
            {
                if(!strcmp(param, "\\GIVEMEFILE"))

Here you are assuming that an entire message has been received.

                strcpy(filename, param);

Ditto.

                        memset(buffer, 0, BUFF_SIZE);

Pointless. Remove.

                        do
                        {
                            s = fgets(buffer, BUFF_SIZE, fp);

Here you are assuming that the file consists of lines.

                            if(s != NULL && buffer[0] != EOF)

Testing buffer[0] !=EOF is meaningless. If you had reached EOF, s would have been null, assuming the file consists of lines, but there is nothing about a line that says anything about what its first character can be, other than that it isn't a line terminator.

                        memset(bufferOUT, 0, BUFF_SIZE);
                        memset(bufferIN, 0, BUFF_SIZE);

Both pointless. Remove.

        memset(bufferOUT, 0, BUFF_SIZE);
        memset(bufferIN, 0, BUFF_SIZE);

Ditto.

void StripNewline(char *s)

This method appears completely pointless. Remove.

void SendMsgToSender(char *msg, int connfd)
{
    write(connfd, msg, strlen(msg));

Here you are sending a string to the peer without the trailing null, which the peer is looking for at strlen() above. Have a good think about what your application protocol actually entails.

    memset(msg, 0, BUFF_SIZE);

Pointless. Remove.

int GetFileFromClient(int connfd, char *filename)
{
    FILE * fp = NULL;
    int bytes;
    char buffer[BUFF_SIZE];
    memset(buffer, 0, BUFF_SIZE);

Pointless. Remove.

    memset(buffer, 0, BUFF_SIZE);

Ditto.

    sprintf(buffer, "\\GIVEMEFILE %s \r\n", filename);
    SendMsgToSender(buffer, connfd);

    while(1)
    {
        memset(buffer ,0 , BUFF_SIZE);

Pointless. Remove.

        if((bytes =  recv(connfd , buffer , BUFF_SIZE , 0) ) <= 0)
            return 0;

Here you need to distinguish between (1) bytes == 0, which means the peer disconnected, and (2) byte == -1, which indicates an error, which you need to log, via errno, strerror(), and friends.

        else
            fprintf(fp, "%s\n", buffer);

Change to fprintf(fp, "%.*s\n", bytes, buffer). You are assuming throughout that all messages are null-terminated by TCP. They aren't.

    sleep(1);

Pointless. Remove.

    memset(buffer, 0, BUFF_SIZE);

Ditto.

    sprintf(buffer, "\r\n");
    SendMsgToSender(buffer, connfd);

Sending a line terminator to the peer appears completely pointless.

    memset(bufferIN, 0, BUFF_SIZE);
    memset(bufferOUT, 0, BUFF_SIZE);
    memset(tmp, 0, BUFF_SIZE);

All pointless. Remove.

        if (bind(listener, p->ai_addr, p->ai_addrlen) < 0)
            continue;

Here you need to print an error mesage instead of just ignoring the condition.

        if (select(fdmax+1, &read_fds, NULL, NULL, NULL) == -1)

You haven't put the listening socket into non-blocking mode. Using select() is therefore pointless.

                        bufferIN[nbytes-1] = '\0';
                        StripNewline(bufferIN);

Why?

                        strcpy(tmp, bufferIN);

Why? What's wrong with continuing to use bufferIN?

                        if(bufferIN[0] == '\\')
                        {
                            char *command, *param;
                            command = strtok(bufferIN, " ");

Here again you are assuming a complete command was received, complete with trailing null.

                            memset(bufferIN, 0, BUFF_SIZE);
                            memset(bufferOUT, 0, BUFF_SIZE);

Both pointless. Remove. This is jut cargo-cult programming. recv() returns a length. Use it.

    memset(bufferIN, 0, BUFF_SIZE);
    memset(bufferOUT, 0, BUFF_SIZE);

Ditto, in spades.

Basically you have an application protocol problem. Specifically, you don't have an application protocol. Just a whole lot of unwarranted assumptions. If you want a trailing null, (a) send a trailing null, and (b) loop reading until you receive it. You also have an assumption about the content of the files being sent, which is completely unnecessary. Just read bytes from the file and send them to the server. No assumption about lines or line terminators necessary. If you're sending multiple files over the same connection you will need to send the file size ahead of the file, so the receiver will know exactly how many bytes to read and copy to the file.

In essence, you need to rethink this completely.