Assessment of client-server program source code

Question

Can you assess source code of my client-server program?

I would like to get an advice about design, code security etc. Thank you very much.

The program does the following:

1. Show files in directories;
2. Download and upload files (for now, only one file at a time).

Syntax:

./client <Server IP> <Server PORT> dir <PATH> 
./client <Server IP> <Server PORT> upload localfile.txt remotefile.txt
./client <Server IP> <Server PORT> download remotefile.txt localfile.txt

Client source code:

client.c

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

#define BUFFSIZE 4096 // Size of receive buffer

int parseARGS(char **args, char *line);
void DieWithError(char *errorMessage); // Error handling function
void UploadFile(int sock, char *lfile, char *rfile);
void DownloadFile(int sock, char *lfile, char *rfile);
void SysCmd(int sock, char *myCommand, char *myArgs);
void ShowExamples(void);

int main(int argc, char *argv[])
{
   int i, percent_sent, sock;                      // Socket descriptor
   struct sockaddr_in servAddr;  // Echo server address
   unsigned short servPort;      // Echo server port
   char *servIP;                    // Server IP addr
   char *myCommand, *myArgs, *commandOpt, *lfile, *rfile;

   // Check cmd args
   if(argc == 6 || argc == 5){
      servIP = argv[1];                // Server IP addr
      servPort = atoi(argv[2]);
      myCommand = argv[3];

        if(!strcmp(myCommand, "dir"))
            myArgs = argv[4];

      //if(argc == 6){
        if(!strcmp(myCommand, "upload") || !strcmp(myCommand, "download")){
         lfile = argv[4];
         rfile = argv[5];
      }
   } else {
        ShowExamples();
      exit(1);
   }

   // Create a reliable, stream socket using TCP
   if((sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
      DieWithError("socket() failed!");

   // Construct the server address structure
   memset(&servAddr, 0, sizeof(servAddr));   // Zero out structure
   servAddr.sin_family = AF_INET;                  // Internet addr family
   servAddr.sin_addr.s_addr = inet_addr(servIP);   // Server IP addr. inet_addr transforms IP addr to bin format
   servAddr.sin_port = htons(servPort);         // Server port. htons make sure bytes are stored in big endian 

   // Establish connection to echo server
   if(connect(sock, (struct sockaddr*) &servAddr, sizeof(servAddr)) < 0)
      DieWithError("connect() failed!");

   // Select a cmd command
   if(!strcmp(myCommand, "upload") && argc == 6)
      UploadFile(sock, lfile, rfile);
   else if(!strcmp(myCommand, "download") && argc == 6)
      DownloadFile(sock, lfile, rfile);
   else if(!strcmp(myCommand, "dir") && argc == 5)
      SysCmd(sock, myCommand, myArgs);
   else
        ShowExamples();


   // Close the socket
   close(sock);
   exit(0);

}

void ShowExamples(void)
{
    printf("Available commands: dir, upload, download.\nExamples:\n-----\n");
    printf("./client <Server IP> <Server PORT> dir <PATH>\n");
    printf("./client <Server IP> <Server PORT> upload localfile.txt remotefile.txt\n");
    printf("./client <Server IP> <Server PORT> download remotefile.txt localfile.txt\n-----\n");
}

void DieWithError(char *errorMsg)
{
   perror(errorMsg);
   exit(1);
}

void UploadFile(int sock, char *lfile, char *rfile)
{  
   int percent_sent, chunk_size = 256;
   long all_bytes_sent, bytes_sent, bytes_read, bytes_left, file_size;
   char buffer[BUFFSIZE];      // Buffer for echo string
   FILE *aFile;

   // Open file for read in binary mode
   aFile = fopen(lfile, "rb");
   if(!aFile)
      DieWithError("fopen() failed!\n");

   // Set pos indicator to the END of file 
   // and return curr pos      
   fseek(aFile, 0, SEEK_END);
   file_size = ftell(aFile);
   rewind(aFile);

   bytes_left = file_size;

   // Init buffer with 0s
   // then fill it with file data
   memset(buffer, 0, sizeof(buffer));
   sprintf(buffer, "UPLOAD:%s:%ld\r\n", rfile, file_size);   

    // Send UPLOAD stat msg with file name and size.
    // Repeat until amount of all sent Bytes equals 4096 Bytes
   all_bytes_sent = 0;
   while(all_bytes_sent != sizeof(buffer)){
      bytes_sent = send(sock, (buffer + all_bytes_sent), (sizeof(buffer) - all_bytes_sent), 0);

      if(bytes_sent < 0)
         DieWithError("send() UPLOAD msg failed!\n");

      all_bytes_sent += bytes_sent;
   }

   //// Allocate dyn memmory to store file before sending 
   //char *file_buffer = (char *)malloc(chunk_size * sizeof(char));  
   //if(NULL == file_buffer){
   // fprintf(stderr, "malloc failed\n");
   // return(-1);
   //}

   printf("\n-----\n");

   // Loop until all Bytes of the file will be send
   while(1){
      char file_buffer[chunk_size];
      memset(file_buffer, 0, sizeof(file_buffer));

      // Read file to file_buffer
      if((bytes_read = fread(file_buffer, sizeof(char), chunk_size, aFile)) < 0)
         DieWithError("fread() failed!\n");

      // Send file over socket
      if(bytes_read > 0){
         if((bytes_sent = send(sock, file_buffer, bytes_read, 0)) < 0)
            DieWithError("send() after fread() failed!");

         // Calc percentage and display status
         bytes_left -= bytes_sent;
         percent_sent = ((file_size - bytes_left) * 100) / file_size;
         printf("Sent %d%% (%ld B), remaining = %ld B\n", percent_sent, bytes_sent, bytes_left);
      }

      // Check the end of file
      // and if it is the end - break loop
      if(bytes_read < chunk_size){
         if(feof(aFile))
            printf("End of file.\n");
         if(ferror(aFile))
            printf("Error reading!\n");
         break;
      }
   }

   // Close the file
   fclose(aFile);
   //free(file_buffer);

   printf("\n");
}

void DownloadFile(int sock, char *lfile, char *rfile)
{
   //printf("Download func is under construction!\n");
   //exit(1);
    int percent_recvd;
   long all_bytes_sent, bytes_sent, bytes_read, bytes_left, file_size;
   long all_bytes_recvd, bytes_recvd, bytes_written;
   char buffer[BUFFSIZE];      // Buffer for echo string
    char *file_name, *header[BUFFSIZE];
   FILE *aFile;

    // Create status message for DOWNLOAD
    memset(buffer, 0, sizeof(buffer));
    sprintf(buffer, "DOWNLOAD:%s:%d\r\n", lfile, 1);

    // Send status msg to request file for DOWNLOAD.
    // Repeat until amount of all received Bytes equals 4096 Bytes.
    all_bytes_sent = 0;
   while(all_bytes_sent != sizeof(buffer)){
      bytes_sent = send(sock, (buffer + all_bytes_sent), (sizeof(buffer) - all_bytes_sent), 0);

      if(bytes_sent < 0)
         DieWithError("send() DOWNLOAD msg failed!\n");

      all_bytes_sent += bytes_sent;
   }

    // Receive stat message to request file size for DOWNLOAD.
    // Repeat until amount of all received Bytes equals 4096 Bytes.
    memset(buffer, 0, sizeof(buffer));
    all_bytes_recvd = 0;
   while(all_bytes_recvd != sizeof(buffer)){
      bytes_recvd = recv(sock, (buffer + all_bytes_recvd), (sizeof(buffer) - all_bytes_recvd), 0);

      if(bytes_recvd < 0)
         DieWithError("recv() STAT msg failed!\n");
      if(bytes_recvd == 0){
         printf("Received STAT msg!\n");
            break;
        }

      all_bytes_recvd += bytes_recvd;
      //printf("STAT: Received %ld B, remaining data = %ld B\n", bytes_recvd, (sizeof(buffer) - all_bytes_recvd));
   }

    // Parse received stat msg to get file name and size
    parseARGS(header, buffer);  
    //file_name = header[1];                        // It is not used. Instead "rfile" used.
    file_size = atoi(header[2]);
    bytes_left = file_size;

    // Open file stream to write in bin mode
    aFile = fopen(rfile, "wb");
    if(aFile == NULL)
        DieWithError("failed to open the file!\n");

    // Receive file via socket, place it in 4096 Byte array 
    // than write buffer content into file.
    // Repeat until amount of all received Bytes equals file size. 
    memset(buffer, 0, sizeof(buffer));
    all_bytes_recvd = 0;
    while(all_bytes_recvd != file_size){
        bytes_recvd = recv(sock, buffer, sizeof(buffer), 0);
        if(bytes_recvd < 0)
            DieWithError("failed to receive the file!\n");      

        if((bytes_written = fwrite(buffer, sizeof(char), bytes_recvd, aFile)) < 0)
            DieWithError("failed to write into the file!\n");

        all_bytes_recvd += bytes_recvd;
        //printf("Received %ld B, remaining data = %ld B\n", bytes_recvd, (file_size - all_bytes_recvd));

      // Calc percentage and display status
      bytes_left -= bytes_recvd;
      percent_recvd = ((file_size - bytes_left) * 100) / file_size;
      printf("Received %d%% (%ld B), remaining = %ld B\n", percent_recvd, bytes_recvd, bytes_left);
    }   

    // Close file stream
    fclose(aFile);
}

// EXEC commands
void SysCmd(int sock, char *myCommand, char *myArgs)
{
    int i = 0;
    long bytes_recvd, all_bytes_recvd, bytes_sent, all_bytes_sent;
    char buffer[BUFFSIZE], *header[BUFFSIZE];

    // Create status message for EXEC command
    memset(buffer, 0, sizeof(buffer));
    sprintf(buffer, "EXEC:%s:%s:1\r\n", myCommand, myArgs); // 1 - to delimit args from \r\n

    // Send the status message
    all_bytes_sent = 0;
    while(all_bytes_sent != sizeof(buffer)){
        if((bytes_sent = send(sock, (buffer + all_bytes_sent), (sizeof(buffer) - all_bytes_sent), 0)) < 0)
            DieWithError("send() failed!\n");

        all_bytes_sent += bytes_sent;
    }

    memset(buffer, 0, sizeof(buffer));

    // Receive response for EXEC command
    all_bytes_recvd = 0;
    while(all_bytes_recvd != sizeof(buffer)){
        if((bytes_recvd = recv(sock, (buffer + all_bytes_recvd), (sizeof(buffer) - all_bytes_recvd), 0)) < 0)
            DieWithError("recv() failed!\n");

        if(bytes_recvd < 0)
            DieWithError("failed to receive the message from server!\n");       

        all_bytes_recvd += bytes_recvd;
    }

    memset(header, 0, BUFFSIZE);

    // Parse buffer to copy members to header array 
    parseARGS(header, buffer);

    // Print output
    while(header[i] != NULL){
        printf("%s\n", header[i]);
        i++;
    }

    // Clean both arrays
    memset(buffer, 0, sizeof(buffer));
    memset(header, 0, BUFFSIZE);
}

int parseARGS(char **args, char *line)
{
   int tmp = 0;
    // Parse line to get args[] elements by ":" delimeter
   args[tmp] = strtok(line, ":");
   while ((args[++tmp] = strtok(NULL, ":")) != NULL);
   return tmp - 1;
}

Server source code:

server.c

#include "lib/TCPServer.h"
#include <pthread.h>

void *ThreadMainRoutine(void *arg);         // Main program of thread

struct ThreadArgs                           // Structure of args to pass to client thread 
{
    int clntSock;
};

int main(int argc, char *argv[])
{
    int servSock, clntSock;             // Sockets descriptors
    unsigned    short servPort;     // Server port
    pthread_t threadID;                 // Thread ID
    struct ThreadArgs *threadArgs;

    if(argc != 2){                          // Test for correct number of args
        fprintf(stderr, "Usage %s <Server Port>\n", argv[0]);
        exit(1);
    }

    servPort = atoi(argv[1]);

    servSock = CreateTCPServerSocket(servPort);

    for(;;){
        clntSock = AcceptTCPConnection(servSock);

        // Create separate memory for client arg
        if((threadArgs = (struct ThreadArgs*) malloc(sizeof(struct ThreadArgs))) == NULL)
            DieWithError("malloc() failed!");
        threadArgs -> clntSock = clntSock;

        // Create client thread
        if(pthread_create(&threadID, NULL, ThreadMainRoutine, (void *) threadArgs) != 0)
            DieWithError("pthread_create() failed!");

        printf("with thread %ld\n", (long int) threadID);
    }
}

void *ThreadMainRoutine(void *threadArgs)
{
    int clntSock;

    // Thread resources are deallocated upon return
    pthread_detach(pthread_self());

    //  Extract socket descriptor from arg
    clntSock = ((struct ThreadArgs*) threadArgs) -> clntSock;
    free(threadArgs);

    HandleTCPClient(clntSock);

    return (NULL);
}

TCPServer.h

#include <stdio.h>
#include <sys/socket.h>
#include <arpa/inet.h>  // for sockaddr_in and inet_ntoa()
#include <stdlib.h>     // for atoi()
#include <string.h>     //  for memset
#include <unistd.h>     //  for clode()
#include <string.h>
#include <netdb.h>
#include <netinet/in.h>


void DieWitError(char *errMSG);                     // Error handling function
void HandleTCPClient(int clntSocket);               // TCP client handling 
void HandleTCPClientFull(int clntSocket);               // TCP client handling 
int CreateTCPServerSocket(unsigned short port); // Create TCP server socket 
int AcceptTCPConnection(int servSock);              // Accept TCP connection req

CreateTCPServerSocket.c

#include <sys/socket.h>
#include <arpa/inet.h>
#include <string.h>

#define MAXPENDING 5                    // Max outstanding connection req

void DieWithError(char *errMsg);

int CreateTCPServerSocket(unsigned short port)
{
    int sock;
    struct sockaddr_in servAddr;

    // Create socket for incomming connection
    if((sock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
        DieWithError("socket() failed!");

    // Construct local addr structure
    memset(&servAddr, 0, sizeof(servAddr));         // Zero out structure
    servAddr.sin_family = AF_INET;                      // Internet addr family
    servAddr.sin_addr.s_addr = htonl(INADDR_ANY);   // Any incomming interface
    servAddr.sin_port = htons(port);                        //  Local port

    // Bind to local addr
    if(bind(sock, (struct sockaddr*) &servAddr, sizeof(servAddr)) < 0)
        DieWithError("bind() failed!");

    // Mark the socket to listen for incomming connections
    if(listen(sock, MAXPENDING) < 0)
        DieWithError("listen() failed!");

    return sock;
}

AcceptTCPConnection.c

#include <stdio.h>
#include <sys/socket.h>
#include <arpa/inet.h>

void DieWithError(char *errMsg);

int AcceptTCPConnection(int servSock)
{
    int clntSock;
    struct sockaddr_in clntAddr;
    unsigned int clntLen;

    // Set size of the in-out parameter
    clntLen = sizeof(clntAddr);

    // Wait for a client to connect
    if((clntSock = accept(servSock, (struct sockaddr*) &clntAddr, &clntLen)) < 0)
        DieWithError("accept() failed!");

    // clntSock is connected to a client
    printf("Handling client %s\n", inet_ntoa(clntAddr.sin_addr));

    return clntSock; 
}

DieWithError.c

#include <stdio.h>

void DieWithError(char *errMsg)
{
    printf("%s\n", errMsg);
}

HandleTCPClient.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <arpa/inet.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <dirent.h>

#define BUFFSIZE 4096

int parseARGS(char **args, char *line);
void UploadFile(int clntSock, char *buffer);
void DownloadFile(int clntSock, char *buffer);
void SysCmd(int clntSock, char *buffer);

void HandleTCPClient(int clntSock)
{
    char buffer[BUFFSIZE];
    long bytes_recvd, all_bytes_recvd;

    // Get initital client stat msg.
    // Repeat until amount of all received Bytes equals 4096 Byte.
    memset(buffer, 0, sizeof(buffer));
    all_bytes_recvd = 0;
    while(all_bytes_recvd != sizeof(buffer)){
        bytes_recvd = recv(clntSock, (buffer + all_bytes_recvd), (sizeof(buffer) - all_bytes_recvd), 0);

        if(bytes_recvd < 0)
            DieWithError("recv() STAT msg failed!\n");
        if(bytes_recvd == 0){
            printf("Received STAT msg!\n");
            break;
        }

        all_bytes_recvd += bytes_recvd;
        printf("STAT: Received %ld B, remaining data = %ld B\n", bytes_recvd, (sizeof(buffer) - all_bytes_recvd));
    }

    // Read first characters of stat msg to determine action:
    // Upload, Download or Exec.
    if(!strncmp(buffer, "UPLOAD", 6))
        UploadFile(clntSock, buffer);
    else if(!strncmp(buffer, "DOWNLOAD", 8))
        DownloadFile(clntSock, buffer);
    else if(!strncmp(buffer, "EXEC", 4))
        SysCmd(clntSock, buffer);
    else
        printf("Wrong STAT message!\n");

}

int parseARGS(char **args, char *line)
{
   int tmp=0;
    // Parse line to get args[] elements by ":" delimeter
   args[tmp] = strtok( line, ":" );
   while ( (args[++tmp] = strtok(NULL, ":" ) ) != NULL );
   return tmp - 1;
}

void UploadFile(int clntSock, char *buffer)
{
    char *file_name, *header[BUFFSIZE];
    long bytes_recvd, bytes_written, file_size, all_bytes_recvd;
    FILE *aFile;

    // Parse received buffer for file name and file size
    parseARGS(header, buffer);
    file_name = header[1];
    file_size = atoi(header[2]);

    // Open a file stream in wite bin mode
    aFile = fopen(file_name, "wb");
    if(aFile == NULL)
        DieWithError("failed to open the file!\n");

   // Receive file via socket, place it in 4096 Byte array 
   // than write buffer content into file.
   // Repeat until amount of all received Bytes equals file size. 
    memset(buffer, 0, BUFFSIZE);
    all_bytes_recvd = 0;
    while((bytes_recvd = recv(clntSock, buffer, BUFFSIZE, 0)) > 0){
        all_bytes_recvd += bytes_recvd;
        printf("Received %ld B, remaining data = %ld B\n", bytes_recvd, (file_size - all_bytes_recvd));

        if((bytes_written = fwrite(buffer, sizeof(char), bytes_recvd, aFile)) < 0)
            DieWithError("fwrite() failed!\n");
    }

    // Close file stream
    fclose(aFile);  
}

void DownloadFile(int clntSock, char *sbuffer)
{
    int percent_sent;
    long bytes_sent, bytes_read, file_size, all_bytes_sent, bytes_left;
    char buffer[BUFFSIZE], *file_name, *header[BUFFSIZE];
    FILE *aFile;

    // Parse buffer for file name 
    parseARGS(header, sbuffer);
    file_name = header[1];

    // Open file stream in read bin mode
    aFile = fopen(file_name, "rb");
    if(aFile == NULL)
        DieWithError("failed to open the file!\n");

    // Shift file stream indicatore to get file size
    fseek(aFile, 0, SEEK_END);
    file_size = ftell(aFile);
    rewind(aFile);

    bytes_left = file_size;
    // Prepare DOWNLOAD stat msg with file name and size    
    memset(buffer, 0, BUFFSIZE);
    sprintf(buffer, "DOWNLOAD:%s:%ld\r\n", file_name, file_size);

    // Send DOWNLOAD stat msg via socket.
    // Repeat until amount of all sent Bytes equals 4096 Bytes. 
    all_bytes_sent = 0;
    while(all_bytes_sent != BUFFSIZE){
        bytes_sent = send(clntSock, (buffer + all_bytes_sent), (BUFFSIZE - all_bytes_sent), 0);
        if(bytes_sent < 0)
            DieWithError("send STAT msg failed!\n");        

        all_bytes_sent += bytes_sent;
        printf("STAT: Sent %ld B, remaining data = %ld B\n", bytes_sent, (BUFFSIZE - all_bytes_sent));
    }

    // Loop until all Bytes of the file will be send
    while(1){
        memset(buffer, 0, BUFFSIZE);

        // Read file into buffer
        if((bytes_read = fread(buffer, sizeof(char), BUFFSIZE, aFile)) < 0)
            DieWithError("failed to read the file!\n"); 

        // Send file over socket
        if(bytes_read > 0){
            if((bytes_sent = send(clntSock, buffer, bytes_read, 0)) < 0)
                DieWithError("fialed to send the file!\n");

            // Calc percentage and display status
            bytes_left -= bytes_sent;
            percent_sent = ((file_size - bytes_left) * 100) / file_size;
            printf("Sent %d%% (%ld B), remaining = %ld B\n", percent_sent, bytes_sent, bytes_left);
        }

        // Check the end of the file
        // if it is End - break.
        if(bytes_read < BUFFSIZE){
            if(feof(aFile))
                printf("End of file.\n");
            if(ferror(aFile))
                printf("Error reading!\n");
            break;
        }
    }

    // Close file stream
    fclose(aFile);  

}

void SysCmd(int clntSock, char *buffer)
{
    char local_buffer[BUFFSIZE], *cmd_name, *cmd_args, *file_name, *header[BUFFSIZE];
    long bytes_sent, all_bytes_sent;
    DIR *dir;
    struct dirent *dp;

    printf("%s\n", buffer);

    memset(local_buffer, 0, sizeof(local_buffer));

    // Parse buffer to get EXEC commands
    parseARGS(header, buffer);
    cmd_name = header[1];
    cmd_args = header[2];

    printf("Args: %s\n", cmd_args);

    // Execute command "dir"
    if(!strcmp(cmd_name, "dir")){

        // Open directory
        dir = opendir(cmd_args);
        if(!dir)
            DieWithError("opendir() failed!\n");
        else {
            // Read the directory and copy all file/dir names to local_buffer array
            while((dp = readdir(dir)) != NULL) {
                if (!strcmp(dp->d_name, ".") || !strcmp(dp->d_name, "..")){
                    // do nothing (straight logic)
                } else {
                   file_name = dp->d_name; // use it
                        strcat(local_buffer, file_name);
                        strcat(local_buffer, ":");
                }   
            }

            // Server status msg
            printf("%s\n", local_buffer);

            // Send info about files/dirs to client.
            // Repeat while amount of all sent Bytes equals BUFFSIZE. 
            all_bytes_sent = 0;
            while(all_bytes_sent != sizeof(local_buffer)){
                if((bytes_sent = send(clntSock, (local_buffer + all_bytes_sent), (sizeof(local_buffer) - all_bytes_sent), 0)) < 0)  
                    DieWithError("send() failed!\n");

                all_bytes_sent += bytes_sent;   
            }
        }

        // Close directory
        closedir(dir);
    }
}

Answer 1

Buffer overflow

The first thing I checked was for buffer overflow and I found it immediately. On the client side in SysCmd():

char buffer[BUFFSIZE], *header[BUFFSIZE];

// Create status message for EXEC command
memset(buffer, 0, sizeof(buffer));
sprintf(buffer, "EXEC:%s:%s:1\r\n", myCommand, myArgs); // 1 - to delimit args from \r\n

Here, myArgs came from argv[4] and could be anything. So if myArgs is longer than BUFFSIZE (4 KB), you will overflow your buffer. You need to either use snprintf or check the length of your arguments before constructing the buffer.

I spotted several calls to sprintf in your code and all of them have the same problem.

Server side processing

On the server side, you have problems when you parse the incoming buffer sent from the client. Remember, you have no idea what the contents of the buffer are because it could have been sent by a malicious program masquerading as a client. So for example, you have this code in SysCmd() on the server side:

    // Parse buffer to get EXEC commands
    parseARGS(header, buffer);
    cmd_name = header[1];
    cmd_args = header[2];

and this is the code for parseARGS():

int parseARGS(char **args, char *line)
{
   int tmp=0;
    // Parse line to get args[] elements by ":" delimeter
   args[tmp] = strtok( line, ":" );
   while ( (args[++tmp] = strtok(NULL, ":" ) ) != NULL );
   return tmp - 1;
}

1. If the client sent a buffer with too few arguments (something with no colons, for example), the calling function will end up dereferencing an argument string that doesn't exist. In other words header[1] and header[2] in the example above may be NULL, and your server will crash.
2. If the client sent a buffer with no null terminating character (for example, 4KB of all 'a'), then parseARGS will keep using strtok past the end of the buffer. This can possibly cause it to write a 0 to memory if it finds a ':' character somewhere. It might also cause your program to crash if it manages to read past the end of the stack.

Handling uploads on the server side

On the server side, when you handle an upload, I saw that you parse the file size passed by the client, but you don't actually use that size later on. Your code for downloading the file is this:

file_size = atoi(header[2]);  // <-- This isn't used!
//
// ... more code ...
//
while((bytes_recvd = recv(clntSock, buffer, BUFFSIZE, 0)) > 0){
    all_bytes_recvd += bytes_recvd;
    printf("Received %ld B, remaining data = %ld B\n", bytes_recvd, (file_size - all_bytes_recvd));

    if((bytes_written = fwrite(buffer, sizeof(char), bytes_recvd, aFile)) < 0)
        DieWithError("fwrite() failed!\n");
}

The problem here is that the client could send the file, and then immediately send another command. But because you just keep reading BUFFSIZE chunks of data from the client, you might read a chunk that includes the end of the file plus the next command. For example, the server might read this:

This is the end of the file.EXEC:dir:/tmp

and think that the whole thing is part of the file being uploaded instead of the end of the file plus another command. You could use the file_size variable to make sure you don't read past the end of the file.

4 KB Messages

Right now you use 4 KB fixed length messages. On the one hand, having fixed length messages makes your code simpler in many respects. On the other hand, you have to send 4 KB even if your command is "EXEC:dir:/tmp" or something small like that.

It wouldn't be too hard switch to variable length messages by having all your messages begin with a 4 byte length followed by the rest of the message. You could still enforce a maximum of 4 KB per message so that you could keep using fixed length buffers in your program.

Answer 2

SysCmd():

memset(header, 0, BUFFSIZE);

is wrong, as header has the size sizeof(char*) * BUFFSIZE.

UploadFile():

sprintf(buffer, "UPLOAD:%s:%ld\r\n", rfile, file_size);

you are uploading lfile, not rfile.

memset(file_buffer, 0, sizeof(file_buffer));

can be removed, you only sent what was read and therefore written in file_buffer.

DownloadFile():

percent_sent = ((file_size - bytes_left) * 100) / file_size;

will fail for files larger than 41 MB, which isn't that huge these days.

DieWithError():

The server doesn't handle rather normal situations, like the other side closing the TCP connection during DownloadFile(). That thread might degrade the performance of the whole system then, because the loop is going to do a system call as fast as it can. No need to send malicious data to make the server crash, if it does so freely already.